[perf-check] Revert "Rollup merge of #88860 - nbdd0121:panic, r=m-ou-se"

[JohnTitor]

This reverts commit f702499, reversing
changes made to 84fe598.

r? @ghost cc #90067 (comment)

[JohnTitor]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit d751558 with merge cbe7d0ec97e0f4f2e1d300b556ff8bbf2e355f4b...

[bors]

☀️ Try build successful - checks-actions
Build commit: cbe7d0ec97e0f4f2e1d300b556ff8bbf2e355f4b (cbe7d0ec97e0f4f2e1d300b556ff8bbf2e355f4b)

[rust-timer]

Queued cbe7d0ec97e0f4f2e1d300b556ff8bbf2e355f4b with parent 47aeac6, future comparison URL.

[rust-timer]

Finished benchmarking commit (cbe7d0ec97e0f4f2e1d300b556ff8bbf2e355f4b): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

• Small improvement in instruction counts (up to -1.0% on incr-patched: b9b3e592dd cherry picked builds of style-servo)

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.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

[JohnTitor]

@Mark-Simulacrum this does seem to cause the style-servo's regression.

[Mark-Simulacrum]

Hm. OK. I think these performance results look close to neutral (+/- about the same amount) so we may wish to investigate further but it's also possible we just accept this regression.

cc @m-ou-se -- do you think the cleanup is worth the partial regressions and improvements here?

[jackh726]

I think this is more a T-libs PR than T-compiler.

[JohnCSimon]

@JohnTitor Can you please address the merge conflicts?

[JohnTitor]

I'd like to see @m-ou-se's (or someone from t-libs) thoughts if this is worth merging before resolving the conflicts.

[m-ou-se]

Do we know why and how this change affects performance? The perf results don't show a lot of difference.

[JohnTitor]

Do we know why and how this change affects performance? The perf results don't show a lot of difference.

The regression we found is #90067 (comment), according to the bot: Moderate regression in instruction counts (up to 1.1% on incr-patched: b9b3e592dd cherry picked builds of style-servo)
And the perf result here is: Small improvement in instruction counts (up to -1.0% on incr-patched: b9b3e592dd cherry picked builds of style-servo)
So we could assume the regression came from that PR, I think.

[m-ou-se]

So we could assume the regression came from that PR, I think.

Sure, but do we have any idea how it caused it? The difference is small enough that it could be something unrelated.

[JohnTitor]

So we could assume the regression came from that PR, I think.

Sure, but do we have any idea how it caused it? The difference is small enough that it could be something unrelated.

Not really, because this PR was created on 27th Oct 2021 and I don't remember the time I investigated. I think you're more familiar with the changes as you're the reviewer of it. Looking at the benchmark code (https://github.com/rust-lang/rustc-perf/tree/master/collector/benchmarks/style-servo) would be helpful. And the point here is if it's worth reverting, I guess.

[Dylan-DPC]

@JohnTitor do we still need this pr?

[JohnTitor]

It depends on which is more important, cleanup or performance regression here. I think this is up to @m-ou-se.

[m-ou-se]

Since the impact is minimal and we don't even know how that PR caused the small regression, I think we should just close this.