Copy link

Contributor

jackh726 commented 14 days ago

rust-timer commented 14 days ago

Awaiting bors try build completion.

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

bors commented 14 days ago

Trying commit 362320b with merge 045cf79...

bors commented 14 days ago

Try build successful - checks-actions
Build commit: 045cf79 (045cf79edfbfad98d41a0fefbb824ac4306f2dbd)

rust-timer commented 14 days ago

rust-timer commented 14 days ago

Finished benchmarking commit (045cf79): comparison url.

Summary: This change led to large relevant mixed results in compiler performance.

• Moderate improvement in instruction counts (up to -1.3% on incr-unchanged builds of wg-grammar)
• Large regression in instruction counts (up to 1.0% on full builds of diesel)

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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

jackh726 commented 14 days ago

Perf is better than last time, but not neutral. It would be nice to look into it a bit, but I also think this is "landable" as-is.

jackh726 commented 14 days ago

(I still have to do an actual review)

eggyal commented 13 days ago

Perf is better than last time, but not neutral. It would be nice to look into it a bit, but I also think this is "landable" as-is.

cachegrind diff for the worst scenario (diesel full check)

I haven't yet made sense of this result, but from a first glance I'm wondering if maybe a cache is no longer being used to the same degree. For example, the early/short-circuiting return in the event of error that is introduced just before this cache insertion:

That said, I'm not convinced this is the actual source of the regression, so I'll keep digging... unless anything obvious jumps out at a more seasoned mind in the interim?

eggyal commented 13 days ago

Per discussion on Zulip the perf regression here is suspected to be due LLVM's PGO decisions.

@rustbot label: +perf-regression-triaged

Marking ready for review.

eggyal commented 12 days ago •

it does introduce quite a bit of boilerplate for the few fallible cases. I wonder if there is some way to encode this into the trait to make it nicer?

I pushed 9f714ef, which delegates from IdFunctor::map_id to IdFunctor::try_map_id—is this what you meant? Otherwise not sure what you have in mind: do you mean to expose an infallible TypeFolder API that (similar now with this commit to IdFunctor) delegates to the fallible version?

In fact, I wonder whether map_id is even needed anymore (all usages have been replaced with try_map_id): does the presence of IdFunctor in rustc_data_structures (aside: it isn't a data structure!) mandate any stability or generality that forces us to keep the infallible version?

jackh726 commented 11 days ago

I do mean altering the TypeFolder api, yes. But definitely for followup, if at all.

AFAIK, rustc_data_structures has no stability requirements. But removing map_id could be done in a followup PR.

This LGTM. @bors r+

bors commented 11 days ago

Commit afa6f92 has been approved by jackh726

LeSeulArtichaut commented 11 days ago

Thanks for picking this up @eggyal!

bors commented 11 days ago

Testing commit afa6f92 with merge e6d2de9...

bors commented 11 days ago

Test successful - checks-actions
Approved by: jackh726
Pushing e6d2de9 to master...

rust-timer commented 11 days ago

Finished benchmarking commit (e6d2de9): comparison url.

Summary: This change led to large relevant mixed results in compiler performance.

• Large improvement in instruction counts (up to -1.0% on incr-full builds of deeply-nested-async)
• Large regression in instruction counts (up to 4.0% on incr-unchanged builds of deep-vector)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

eggyal commented 6 days ago •

It's curious how the post-merge perf result is so different to the pre-merge result at #91230 (comment) (that was put down to LLVM/PGO optimisations). I don't think anything material changed in this PR between the two runs, so I guess something material was merged in the interim. Nevertheless:

cachegrind diff for the worst scenario (deep-vector incr-unchanged opt)

Some material differences there in hashing, perhaps indicating a difference in cache use—possibly around resolving consts?

Because I don't think this is due to PGO:
@rustbot label: -perf-regression-triaged

rustbot commented 6 days ago

Error: Label because can only be set by Rust team members

Please let @rust-lang/release know if you're having trouble with this bot.