-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Speed up Vec::clear()
.
#95664
Speed up Vec::clear()
.
#95664
Conversation
Currently it just calls `truncate(0)`. `truncate()` is (a) not marked as `#[inline]`, and (b) more general than needed for `clear()`. This commit changes `clear()` to do the work itself. This modest change was first proposed in rust-lang#74172, where the reviewer rejected it because there was insufficient evidence that `Vec::clear()`'s performance mattered enough to justify the change. Recent changes within rustc have made `Vec::clear()` hot within `macro_parser.rs`, so the change is now clearly worthwhile. Note that this will also benefit `String::clear()`, because it just calls `Vec::clear()`.
r? @m-ou-se (rust-highfive has picked a reviewer for you, use r? to override) |
Here are some local measurements of instruction counts for
|
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 27bddcb with merge 7e5a405eec6891b9459ca262ea526e966fae0fa4... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
Queued 7e5a405eec6891b9459ca262ea526e966fae0fa4 with parent 60e50fc, future comparison URL. |
The failing codegen test (vec-clear.rs) was added in #52908. From before then until now, |
Finished benchmarking commit (7e5a405eec6891b9459ca262ea526e966fae0fa4): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
Huh... my local perf run showed nice wins, but the CI perf run showed basically nothing. CI builds do PGO, I wonder if that explains the difference. Anyway, not worth pursuing this further. |
Currently it just calls
truncate(0)
.truncate()
is (a) not marked as#[inline]
, and (b) more general than needed forclear()
.This commit changes
clear()
to do the work itself. This modest changewas first proposed in #74172, where the reviewer rejected it because
there was insufficient evidence that
Vec::clear()
's performancemattered enough to justify the change. Recent changes within rustc have
made
Vec::clear()
hot withinmacro_parser.rs
, so the change is nowclearly worthwhile.
Note that this will also benefit
String::clear()
, because it justcalls
Vec::clear()
.