-
-
Notifications
You must be signed in to change notification settings - Fork 668
dmd.root.array: add any and all predicates to dmd.root.array #22284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for your pull request and interest in making D better, @gorsing! 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. Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#22284" |
57f90a4 to
98f258c
Compare
|
Can you include some examples of where you want to use these functions? |
98f258c to
63d8497
Compare
|
Hi @dkorpel, thanks for the question. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the examples. Unfortunately I don't think they demonstrate benefits. It would be helpful if it collapsed complex loop logic into something simpler, but replacing a foreach loop body with a function literal passed to a call to all or any only makes the code harder to read in my opinion, as it's now harder to recognize that a loop is being performed.
compiler/src/dmd/semantic3.d
Outdated
| //fens = null; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example adds a return statement, which is discarded.
compiler/src/dmd/semantic3.d
Outdated
| const allDefined = funcdecl.labtab.tab.asRange.all!(kv => | ||
| (cast(LabelDsymbol)kv.value).statement || | ||
| ((cast(LabelDsymbol)kv.value).deleted && !(cast(LabelDsymbol)kv.value).iasm) | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds more code, doing an additional loop which is redundant because the original loop is unchanged.
compiler/src/dmd/semantic3.d
Outdated
| return true; | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an improvement relative to for, but relative to the more obvious refactor to foreach it only adds another discarded return value.
compiler/src/dmd/semantic3.d
Outdated
| if (f2 == f) | ||
| break LcheckAncestorsOfANestedRef; | ||
| } | ||
| if (a[].any!(f2 => f2 == f)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good loop replacement, but can already be done better with a.contains(f).
|
Thanks for the feedback, @dkorpel. |
Not really. I can think of many places where a loop is used to find an element in a list.
If you can formulate a piece of loop code as a mathematical question (like the aforementioned "does this list contain this element?") then the loop could be seen as an implementation detail, justifying the use of range functions. In dmd loops often have side effects and do more than produce a boolean result (computing types, printing errors, rewriting elements), in which case trying to hide the |
63d8497 to
be73bef
Compare
|
Hi @dkorpel Thanks for the previous review I have updated our base Array!T container with several high-performance methods O(N). These are the modern practices found in Rust, LLVM, and modern C++ that were missing from our implementation. Instant Deletion (swapRemove):
Efficient Filtering (removeIf):
Memory Management (shrinkToFit):
Cleaner Code (any, all):
|
Thanks for doing that! |
Do you have benchmarks showing how much this improves compilation times? Note that the dmd.root.array module is not a complete general purpose array implementation, it is only used for dmd to make bootstrapping easier. Hence additions to that module need to be motivated through dmd's use cases. |
4ce58ec to
f4853eb
Compare
I am currently working on a profiler report to demonstrate the specific performance gains. Regarding total compilation time: measuring a macro-level improvement right now is non-trivial. Since I extracted the refactoring of semantic3.d into a separate PR to keep this contribution minimal, these new methods are not yet being utilized in the compiler's hot paths. Consequently, any immediate gains are currently lost in the noise of a full build. However, the report I am preparing compares the current O(N²) patterns often found in DMD (manual loops with remove) against the O(N) removeIf implementation. My goal is to show that while dmd.root.array is indeed specialized, adding these algorithmic primitives provides the necessary infrastructure to optimize. I will post the benchmark results shortly. |
It is indeed hard, but important to measure because expectations and reality can wildly differ when trying to optimize something.
Great! If you need any help benchmarking let me know. |
Description:
Adds high-performance, inlined any and all predicates to Array in dmd.root.array. These templates use alias predicates to ensure zero-overhead execution, facilitating the refactoring of manual for loops in semantic analysis stages.