Improve time complexity of traverseIndirections

[dkorpel]

I was profiling the build time of DMD and noticed the host compiler spent a very long time on StructDeclaration.finalizeSize.

It turns out that 300 ms of semantic analysis time is spent on the following line:

bool isunion = isUnionDeclaration() !is null;

When using my self-built dmd instead of an official release as host compiler, it's even 800 ms.

So I walked down the call stack of the function that analysed that line, looking for the culprit:

expressionsem.visit(IdentityExp exp)
expressionsem.typeCombine(exp, sc)
dcast.typeMerge()
dcast.implicitConvTo(Expression e, Type t)
FuncDeclaration.isReturnIsolated()
FuncDeclaration.isTypeIsolatedIndirect()
traverseIndirections()

And there you had it: traverseIndirections(inout(UnionDeclaration), inout(Dsymbol)) took all the time. What's happening is that all fields of Dsymbol are recursively walked looking for UnionDeclaration, which can take a while, since Dsymbol is huge and UnionDeclaration is pretty deep in the hierarchy:

UnionDeclaration : StructDeclaration : AggregateDeclaration : ScopeDsymbol : Dsymbol

But the worst thing is that there is no memoization. There is a simple cycle checker that walks back a linked list, but if Dsymbol can reach a field of e.g. type Symbol* in 50 different ways, it will walk down all fields of Symbol 50 times, and do that recursively in the same inefficient way.

This PR tries to solve it by inserting fields it visits into a DsymbolTable, and quitting when it's already there. This not only replaces the old cycle checker, but also prunes redundant branches. The amount of calls to traverse went down from 1145127 to 1041. The frontend build time of dmd (no codegen) went down from 3.4 to 2.6 seconds, measured with this command:

time dmd -version=MARS -Jdmd/res -J.. -i -o- dmd/mars.d

I have yet to test it on other codebases, I'm open for suggestions of codebases that you suspect have super complex aggregate definitions like Dsymbol.

I hope the algorithm is still correct, I assumed type.toDsymbol() and DsymbolTable handle type modifiers (inout in this case) like I expect, but I have yet to check.

[dlang-bot]

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#12885"

[dkorpel]

I hope the algorithm is still correct, I assumed type.toDsymbol() and DsymbolTable handle type modifiers (inout in this case) like I expect, but I have yet to check.

This error is missing now in fail_compilation/testInference.d:

Error: cannot implicitly convert expression `this.a` of type `inout(A8998)` to `immutable(A8998)`

So I guess not.

[dkorpel]

DsymbolTable only looks at identifiers, and identifiers are interned strings. dmd.root.aav only hashes pointers, so making a map of types is actually difficult to do in dmd. I could use some assistance here. Maybe I can put the type's mangle in a string table?

[dkorpel]

I learned that Type.deco is an interned C-string of the type's mangle, and it seems we can use that for the hashmap.

[thewilsonator]

This is green, is it good to go?

[dkorpel]

One thing left to do is check whether my assert(tb.deco) is correct, e.g. is traverseIndirections only called on completed types with mangle or are there exceptions?

[RazvanN7]

It would be interesting to see what is the memory footprint of this. I suspect that it should be negligible.