-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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 VecDeque::extend from TrustedLen specialization #98004
Add VecDeque::extend from TrustedLen specialization #98004
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
a121840
to
f6a0c4c
Compare
The new code path probably changes what's covered by existing tests, so it would be good to update them to make sure that both the default and the specialized paths are exercised. |
588e1d2
to
d7f449f
Compare
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.
Looks good except for a few minor style things
d7f449f
to
e1065b7
Compare
e1065b7
to
f06dac6
Compare
This comment has been minimized.
This comment has been minimized.
f06dac6
to
ce3b6f5
Compare
@bors r+ |
📌 Commit ce3b6f5 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (2cec687): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesThis benchmark run did not return any relevant results for this metric. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
Continuation of #95904
Inspired by how
VecDeque::copy_slice
works.Benchmarks
Before
After
Why do it this way
https://rust.godbolt.org/z/15qY1fMYh
The Compiler Explorer example shows how "just" removing the capacity check, like the
Vec
TrustedLen
specialization does, wouldn't have been enough forVecDeque
.wrap_add
would still have greatly limited what LLVM could do while optimizing.r? @the8472