Fix 1037 (rebased against master)#1377
Conversation
|
Thanks for your pull request and interest in making D better, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
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. |
s-ludwig
left a comment
There was a problem hiding this comment.
Finally looked through this line by line. Looks good apart from the added GC allocation and ConfigEntry should be called PackageConfigs or similar. I can make those changes separately, though, as to not further delay this PR unnecessarily.
| sizediff_t childidx = package_indices[basepack]; | ||
| if (all_configs[childidx].length == 1 && all_configs[childidx][0] == CONFIG.invalid) { | ||
| auto child = configs[childidx]; | ||
| if (child.allConfigs == [CONFIG.invalid]) { |
There was a problem hiding this comment.
Produces a GC allocation in the inner loop.
There was a problem hiding this comment.
did you measure whether it actually had a performance impact?
also, if it does, StaticArray(Config.invalid) could be used
There was a problem hiding this comment.
oh wow that actually uses the GC... who would have thought if you could simply put that array in a separate line as static immutable to not do that.
There was a problem hiding this comment.
Yes, in the sense that I got huge speedups with all allocations removed from that loop. Pretty stupid that this is not optimized away by the compiler automatically.
There was a problem hiding this comment.
dlang/druntime#2093 [WIP] core.array.staticArray static array litteral: staticArray(1, 2, 3) #2093
There was a problem hiding this comment.
Definitely an improvement, although I still think that the compiler should be smarter about array literals. It could for example simply infer the possible constness of the literal, and if it is not required to be mutable, it can safely allocate a static array instead of issuing a GC allocation (marking it as @nogc, too).
just a rebase of #1154 from @WebFreak001 against master