-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Add TrustedRandomAccess
specialization for Vec::extend()
#83770
Conversation
@rustbot label +T-libs-impl |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit d3b03191639102db5aa13f0f525f1f68d11550ac with merge e808b7b361f6311b62118983e044a5f05634690d... |
☀️ Try build successful - checks-actions |
Queued e808b7b361f6311b62118983e044a5f05634690d with parent d1065e6, future comparison URL. |
Finished benchmarking try commit (e808b7b361f6311b62118983e044a5f05634690d): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
@Dylan-DPC this is missing an assignee |
r? @dtolnay |
This should do roughly the same as the TrustedLen specialization but result in less IR by using __iterator_get_unchecked instead of iterator.for_each.
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 PR! The implementation looks good to me.
Would you be able to provide an example program where this produces better codegen after optimizations? Is there a simple thing I could measure with cargo bench
or cargo run --release
to see the improvement? (Or is that not the point of the PR?)
The goal here was compile time improvements, not runtime. Although runtime might benefit in some cases too, but I haven't looked at the assembly output for this PR. When available I can run the |
Ah okay, makes sense! I am not familiar enough with reading the rustc performance comparisons to make a call whether this is worthwhile. WDYT @Mark-Simulacrum? |
Performance looks promising but I have a couple questions on the implementation (not necessarily blockers, just want to hear your thoughts). |
@bors r+ rollup=never |
📌 Commit 0202875 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000. This pull request will be tested once the tree is reopened. |
☀️ Test successful - checks-actions |
Current perf run actually shows bigger regression, than in previous run https://perf.rust-lang.org/compare.html?start=9a700d2947f2d7f97a2c0dfca3117a8dcc255bdd&end=9111b8ae9793f18179a1336417618fc07a9cac85&stat=instructions%3Au |
Those results are quite surprising, especially since the sign of the deltas changed in some cases. But I guess we can revert this and I redo those changes together with #84255 which needs rebasing anyway and touches some of the same code paths, then they can be measured together. |
Revert "Auto merge of rust-lang#83770 - the8472:tra-extend, r=Mark-Simulacrum" Due to a performance regression that didn't show up in the original perf run this reverts commit 9111b8a (rust-lang#83770), reversing changes made to 9a700d2. Since since is expected to have the inverse impact it should probably be rollup=never. r? `@Mark-Simulacrum`
This should do roughly the same as the
TrustedLen
specialization but result in less IR by using__iterator_get_unchecked
instead of
Iterator::for_each
Conflicting specializations are manually prioritized by grouping them under yet another helper trait.