Speed up Vec::clear(). #96002
Speed up Vec::clear(). #96002
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. Although it doesn't show wins on CI perf runs, this seems to be because they use PGO. But not all platforms currently use PGO. Also, local builds don't use PGO, and `truncate` sometimes shows up in an over-represented fashion in local profiles. So local profiling will be made easier by this change. Note that this will also benefit `String::clear()`, because it just calls `Vec::clear()`. Finally, the commit removes the `vec-clear.rs` codegen test. It was added in rust-lang#52908. From before then until now, `Vec::clear()` just called `Vec::truncate()` with a zero length. The body of Vec::truncate() has changed a lot since then. Now that `Vec::clear()` is doing actual work itself, and not just calling `Vec::truncate()`, it's not surprising that its generated code includes a load and an icmp. I think it's reasonable to remove this test.
rustbot
added
the
T-libs
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
rust-highfive
added
the
S-waiting-on-review
|
I first tried this in #95664, then close the PR because PGO rendered it ineffective. I have now changed my mind again because not all builds benefit from PGO. |
|
Thanks! @bors r+ |
|
📌 Commit 9c59d04 has been approved by |
bors
added
S-waiting-on-bors
|
Maybe rollup=never, if it optimization in some way? |
|
@bors rollup=never |
|
Hm, does it mean that if this change get merged, we can try to merge #78884 once again and see if it hurts performance this time? |
We've seen for CI perf runs in #95664 that this doesn't have any effect. (The benefits are on platforms/configurations other than the CI perf platform/configuration.) But I guess it doesn't hurt to be cautious.
My understanding of that PR is that changing the comparison within |
Codegen was worse precisely for cases |
|
⌛ Testing commit 9c59d04 with merge fcde805782d23f7175ad42994d3dd5ee63c470f6... |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (43a71dc): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
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.
Although it doesn't show wins on CI perf runs, this seems to be because they
use PGO. But not all platforms currently use PGO. Also, local builds don't use
PGO, and
truncatesometimes shows up in an over-represented fashion in localprofiles. So local profiling will be made easier by this change.
Note that this will also benefit
String::clear(), because it justcalls
Vec::clear().Finally, the commit removes the
vec-clear.rscodegen test. It wasadded in #52908. From before then until now,
Vec::clear()just calledVec::truncate()with a zero length. The body of Vec::truncate() haschanged a lot since then. Now that
Vec::clear()is doing actual workitself, and not just calling
Vec::truncate(), it's not surprising thatits generated code includes a load and an icmp. I think it's reasonable
to remove this test.
r? @m-ou-se