Cache the result of FuncDeclaration.isTypeIsolated()...

[JohanEngelen]

...because it is potentially a very expensive check.

This resulted in a large compile-time improvement on the large codebase at Weka.io (11min --> 3.5min).

Patch by @EyalIO !

[dlang-bot]

Thanks for your pull request and interest in making D better, @JohanEngelen! 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#12930"

[dkorpel]

I don't like the addition of global variables. Have you tried caching based on .deco (interned string of the type's mangle) instead? Also, isTypeIsolated has a terrible time complexity currently. Can you try out how #12885 affects compile time of the large codebase at Weka.io?

[JohanEngelen]

I don't like the addition of global variables.

This is not an objective critism that I can remedy.

Have you tried caching based on .deco (interned string of the type's mangle) instead?

This won't work without string copying in Type copy(). A Type's mangle can be very large, so it's not fast to index an AA based on these strings.

Also, isTypeIsolated has a terrible time complexity currently. Can you try out how #12885 affects compile time of the large codebase at Weka.io?

I will try it, give me a few days.

(Nice to see you are using --ftime-trace and that you are putting it to good use! :-) hope you like the improvements of ldc-developers/ldc#3797 !)

[thewilsonator]

dmd/mtype.d(254:31)[warn]: Variable declaration for `nextUniqueId` has redundant attributes (`static`, `__gshared`).

[dkorpel]

This won't work without string copying in Type copy(). A Type's mangle can be very large, so it's not fast to index an AA based on these strings.

Why do you need to copy it? It's interned, meaning it's immutable and two decos are the same if and only if their pointers are the same. The pointer value of .deco is already a unique id AFAIK.

(Nice to see you are using --ftime-trace and that you are putting it to good use! :-) hope you like the improvements of ldc-developers/ldc#3797 !)

It's a useful feature, thanks for adding it!

[RazvanN7]

@JohanEngelen : @dkorpel is right, the entire point of deco is to provide fast comparison of types. This makes it the faster alternative and gets rid of the need to add a new field.

[kinke]

11min --> 3.5min

!!!

[JohanEngelen]

@JohanEngelen : @dkorpel is right, the entire point of deco is to provide fast comparison of types. This makes it the faster alternative and gets rid of the need to add a new field.

How true is this?
Are Type objects never deleted? If they are deleted, then deco memory is freed, and could be used by another type's deco somewhere down the line leading to spurious miscompilation. (This is the reason why we did not use Type object's pointer)
Due to Type's copy() returning a new type with the same deco, I'm worried that there are cases where it's not uniqued but still a subtly different type.

[JohanEngelen]

11min --> 3.5min

!!!

You forgot one more exclam! Yeah, the impact is huge.

[RazvanN7]

Due to Type's copy() returning a new type with the same deco, I'm worried that there are cases where it's not uniqued but still a subtly different type.

I'm fairly certain they are unique. The copy() functions returns a new type that has not gone through semantic analysis. During semantic analysis of the type itself, the uniqness is assured.

[JohanEngelen]

isTypeIsolated has a terrible time complexity currently. Can you try out how #12885 affects compile time of the large codebase at Weka.io?

Looks like we are down to 1m49 currently (just sema, no codegen).
With this PR: 1m49
With this PR + #12885: 1m49 (no change)
With only #12885: 2m07
Without any of this: 2m28 (apparently a lot of other compiletime improvements have landed)

Due to Type's copy() returning a new type with the same deco, I'm worried that there are cases where it's not uniqued but still a subtly different type.

I'm fairly certain they are unique. The copy() functions returns a new type that has not gone through semantic analysis. During semantic analysis of the type itself, the uniqness is assured.

I don't understand why this is being debated. We need a unique identifier for Type, that's what this PR adds in a clear way. Using deco (an unintelligible variable name) as a unique identifier might work, but that's not the way to write source code of a robust compiler.

[JohanEngelen]

With this PR + #12885: 1m49 (no change)

That's not to say the #12885 does not have value. If it reduces complexity, so much the better!

[dkorpel]

I don't understand why this is being debated. We need a unique identifier for Type, that's what this PR adds in a clear way. Using deco (an unintelligible variable name) as a unique identifier might work, but that's not the way to write source code of a robust compiler.

It's debated because re-inventing an existing mechanism in the compiler with a global variable adds complexity to the code. I agree that deco is poorly named and it should be documented, but adding new redundant code because the existing code is poorly understood is not the way to write a robust compiler either.

[dkorpel]

Looks like we are down to 1m49 currently (just sema, no codegen).
With this PR: 1m49
With this PR + #12885: 1m49 (no change)
With only #12885: 2m07
Without any of this: 2m28 (apparently a lot of other compiletime improvements have landed)

Thank you! This is very interesting, I expected the codebase would contain a few complex base classes that would take >1s for each isTypeIsolated check similar to the 800ms of dmd's Dsymbol, but considering it doesn't even give a second of improvement with caching, it suggests that your codebase is doing a few relatively small (1 - 100ms?) checks very often instead.

[JohanEngelen]

Looks like we are down to 1m49 currently (just sema, no codegen).
With this PR: 1m49
With this PR + #12885: 1m49 (no change)
With only #12885: 2m07
Without any of this: 2m28 (apparently a lot of other compiletime improvements have landed)

Thank you! This is very interesting, I expected the codebase would contain a few complex base classes that would take >1s for each isTypeIsolated check similar to the 800ms of dmd's Dsymbol, but considering it doesn't even give a second of improvement with caching, it suggests that your codebase is doing a few relatively small (1 - 100ms?) checks very often instead.

My "without any of this" was actually including the change in dcast.d which puts the isTypeIsolated check later in a series of checks, that probably also helped.

[JohanEngelen]

I don't understand why this is being debated. We need a unique identifier for Type, that's what this PR adds in a clear way. Using deco (an unintelligible variable name) as a unique identifier might work, but that's not the way to write source code of a robust compiler.

It's debated because re-inventing an existing mechanism in the compiler with a global variable adds complexity to the code. I agree that deco is poorly named and it should be documented, but adding new redundant code because the existing code is poorly understood is not the way to write a robust compiler either.

deco is not meant to be a unique identifier, even if it could be used as such. You can even see in the comment by Razvan and the behavior of copy that it is going to change value during sema. Very bad for a unique identifier.
Adding a "global" is the only way I know of getting a unique id (I've now made it private btw); let me know if it should be made threadsafe. Show me how else to do it and I'll change the code to it. Memory allocation (which is done for the string of deco) is very much not a guaranteed way to get a unique value.
I strongly disagree here. DMD is already terribly unstable code, I'm not going to add more badness to it.

[dkorpel]

My "without any of this" was actually including the change in dcast.d which puts the isTypeIsolated check later in a series of checks, that probably also helped.

Is the codebase compiled with -preview=dip1000? In that case, the change to dcast.d will help a ton because few functions are strongly pure usually. Otherwise, it shouldn't matter.

[dkorpel]

Memory allocation (which is done for the string of deco) is very much not a guaranteed way to get a unique value.

I see your point now, you don't want to rely on the fact that dmd interns .deco strings in Type.stringtable and never frees them, so the code keeps working even when the memory allocation scheme is changed. That's fair. I don't think you need to make it thread safe, dmd really isn't. I would consider resetting the global state in Type._init() for dmd.frontend:initDMD, and adding an assert(!__ctfe) to isTypeIsolated with a comment that it doesn't work in CTFE because it relies on a global counter.

[WalterBright]

Each Type already gets a unique identifier - it's the .deco field.

[WalterBright]

deco is not meant to be a unique identifier, even if it could be used as such.

It absolutely is meant to be a unique identifier, and is used as such everywhere. For example, because these identifiers are unique, types can (and are) compared for identity by comparing the pointers to deco, never the contents of deco. deco also forms the basis of the type name mangling, which utterly relies on its uniqueness and the 1:1 mapping of types <=> deco. The compiler would fall apart if it wasn't unique.

[WalterBright]

I was looking at isTypeIsolated(), as I was unaware that it was in the compiler. Some problems with it:

1. Why isn't it part of the Type hierarchy? It doesn't seem to rely at all on being a member function of FuncDeclaration. It could be static!
2. It doesn't deal with TypeTuple or TypeDelegate parameters.
3. The name suggests TypeIsolated is a Type. It is not.

[WalterBright]

Looking at the implementation if isTypeIsolated(), there is:

const tName = t.toChars().toDString;

which is going to be disastrously slow, and consume lots and lots and lots of memory. It's then inserted into an associated array.
@JohanEngelen wrote: "A Type's mangle can be very large, so it's not fast to index an AA based on these strings." but that's exactly what is happening in isTypeIsolated(), and it's wasting time generating the string, too.

[WalterBright]

I suggest isIsolatedType instead of isTypeIsolated.

[RazvanN7]

@JohanEngelen Are you now convinced that deco can be used as a unique identifier? If so, could you update this PR so that we can move forward?

[maxhaton]

@RazvanN7 @JohanEngelen ping pong

[RazvanN7]

@maxhaton I will happily merge this once the comments have been addressed and the conflicts resolved.

[JohanEngelen]

haven;t gotten around to testing with the changes yet

[JohanEngelen]

@RazvanN7 @maxhaton I have tested using deco as a unique identifier and it resulted in the same object code binary on a large testcase. Also, compilation times were identical. I've rewritten the code to use deco as unique identifier and fixed the merge conflict with master.

[maxhaton]

@RazvanN7 @maxhaton I have tested using deco as a unique identifier and it resulted in the same object code binary on a large testcase. Also, compilation times were identical. I've rewritten the code to use deco as unique identifier and fixed the merge conflict with master.

What results are you getting now? This seems to yield about 5% on some informal testing for me, does that correlate? I haven't tried it on any internal company things but I imagine it'll be similar

[JohanEngelen]

@RazvanN7 @maxhaton I have tested using deco as a unique identifier and it resulted in the same object code binary on a large testcase. Also, compilation times were identical. I've rewritten the code to use deco as unique identifier and fixed the merge conflict with master.

What results are you getting now? This seems to yield about 5% on some informal testing for me, does that correlate? I haven't tried it on any internal company things but I imagine it'll be similar

I cannot test using dmd master (will take months before we get there, no joke).

Edit: and 5% is huge.

[nordlow]

This resulted in a large compile-time improvement on the large codebase at Weka.io (11min --> 3.5min).

Did you use any profiling tools to figure this out? If so, which?

[JohanEngelen]

This resulted in a large compile-time improvement on the large codebase at Weka.io (11min --> 3.5min).

Did you use any profiling tools to figure this out? If so, which?

The improvement is not so large anymore (codebase has evolved, see a later post in this thread here), but initially it was huge. I believe Eyal found the offending code using -ftime-trace (LDC), I don't know about the discovery of which exact function in dmd code took most time.

[JohanEngelen]

ping