-
Notifications
You must be signed in to change notification settings - Fork 288
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
Reduce the amount of llvm IR instantiated #205
Conversation
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.
Could you try running the benchmarks (cargo bench
) to see if any of the changes have a performance impact?
src/raw/mod.rs
Outdated
} | ||
} | ||
|
||
impl RawTableInner { |
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.
All methods here should be #[inline]
since they are non-generic.
Didn't bother to inline the private functions since I think it is redundant within the same compilation unit(?) But I guess they are used from generic functions which causes them to be used from another compilation unit. That seemed to be enough to restore performance though.
|
The changes mostly look good. However they are quite invasive so I would feel more comfortable if we could test the impact of this change in compilation performance. Could you try creating a PR in rust-lang/rust which switches libstd to use your branch of hashbrown so that we can do a perf run? |
☔ The latest upstream changes (presumably #208) made this pull request unmergeable. Please resolve the merge conflicts. Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:
|
3faab35
to
502f9fe
Compare
adb8127
to
0ec09ce
Compare
Includes rust-lang/hashbrown#204 and rust-lang/hashbrown#205 (not yet merged) which both server to reduce the amount of IR generated for hashmaps. Inspired by the llvm-lines data gathered in rust-lang#76680
feat: Update hashbrown to instantiate less llvm IR Includes rust-lang/hashbrown#204 and rust-lang/hashbrown#205 (not yet merged) which both serve to reduce the amount of IR generated for hashmaps. Inspired by the llvm-lines data gathered in rust-lang#76680 (cc `@Julian-Wollersberger)`
This works to improve the generated code in a similar way to #204 , however it is entirely orthogonal to it but also far more invasive.
The main change here is the introduction of
RawTableInner
which is contains all the fields thatRawTable
has but without being parameterized byT
. Methods have been moved to this new type in parts or their entirety andRawTable
forwards to these methods.For this small test case with 4 different maps there is a reduction of the number of llvm lines generated by -17% (18088 / 21873 =0.82695560737) .
The commits are almost entirely orthogonal (except the first which does the main refactoring to support the rest) and if some are not desired they can be removed. If it helps, this PR could also be split up into multiple.
For most commitst I don't expect any performance degradation (unless LLVM stops inlining some function as it is now called more), but there are a couple of commits that does slow parts down however these should only be in the cold parts of the code (for instance, panic handling).