-
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
Revert performance-sensitive change in #82436 #83293
Conversation
This change was done in rust-lang#82436, as an "optimization". Unfortunately I missed that this code is not always executed, because of the "continue" in the conditional above it. This commit should solve the perf regressions introduced by rust-lang#82436 as I think there isn't anything else that could affect runtime performance in that PR. The `Pick` type grows only one word, which I doubt can cause up to 8.8% increase in RSS in some of the benchmarks.
r? @varkor (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit f925757 with merge cec4d3de4e29e63a5e96e00a156057ceef9aafad... |
☀️ Try build successful - checks-actions |
Queued cec4d3de4e29e63a5e96e00a156057ceef9aafad with parent eb95ace, future comparison URL. |
Finished benchmarking try commit (cec4d3de4e29e63a5e96e00a156057ceef9aafad): 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 |
Great, this seems to solve the perf issue introduced in #82436. Let's merge this. |
@bors r+ |
📌 Commit f925757 has been approved by |
⌛ Testing commit f925757 with merge a142e00ce2ccc52f7349c40b4cb927e9f3e7e0d9... |
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
@bors retry |
☀️ Test successful - checks-actions |
use a `SmallVec` in `impl_or_trait_item` rust-lang#83293 showed that this is fairly hot, slightly improves max-rss and cpu cycles, does not noticeably improve instruction counts
This change was done in #82436, as an "optimization". Unfortunately I
missed that this code is not always executed, because of the "continue"
in the conditional above it.
This commit should solve the perf regressions introduced by #82436 as I
think there isn't anything else that could affect runtime performance in
that PR. The
Pick
type grows only one word, which I doubt can cause upto 8.8% increase in RSS in some of the benchmarks.
Could someone with the rights start a perf job please?