Skip to content

rust-timer contradicts itself about whether there were perf changes #1085

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

Closed
camelid opened this issue Oct 30, 2021 · 4 comments
Closed

rust-timer contradicts itself about whether there were perf changes #1085

camelid opened this issue Oct 30, 2021 · 4 comments
Labels
A-triage Issues related to the triage process C-improvement This isn't a feature or a bug fix but makes things better

Comments

@camelid
Copy link
Member

camelid commented Oct 30, 2021

Example: rust-lang/rust#90411 (comment)

rust-timer says both:

This benchmark run did not return any relevant changes.

and:

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.

(When I opened the comparison page, there were very slight changes, but as rust-timer itself said, nothing very significant.)

@Mark-Simulacrum
Copy link
Member

Hm, the latter message I think used to be less confident, but this still seems correct to me. The key point is that while there are no relevant changes (i.e., the PR author/review should likely not take any action to resolve regressions etc), there are still changes, and so we shouldn't roll this PR up as it may mask regressions (or, less importantly, improvements) in other PRs.

In general we take perf invocation as a proxy for "someone thought this perf sensitive", and that's usually enough that rolling up is probably at least =iffy, and =never is not that comparatively more expensive.

@Mark-Simulacrum
Copy link
Member

We've not yet found good wording for the "relevant changes" messaging though, suggestions welcome there -- ideally we'd concisely indicate that there potentially are changes but that you probably shouldn't care.

@camelid
Copy link
Member Author

camelid commented Oct 30, 2021

We've not yet found good wording for the "relevant changes" messaging though, suggestions welcome there -- ideally we'd concisely indicate that there potentially are changes but that you probably shouldn't care.

It'd be less concise, but something along these lines could work:

This benchmark run did not return any relevant changes. While some changes may have occurred, they are insignificant, although they could mask other changes if included in a rollup.

@rylev rylev added A-triage Issues related to the triage process C-improvement This isn't a feature or a bug fix but makes things better labels Feb 25, 2022
@Kobzol
Copy link
Contributor

Kobzol commented Jul 1, 2023

The messages have been quite overhauled since this issue has been created, and in this specific case, the text has been "softened":

While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this
PR may lead to changes in compiler perf.

So I think that this can be closed.

@Kobzol Kobzol closed this as completed Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-triage Issues related to the triage process C-improvement This isn't a feature or a bug fix but makes things better
Projects
None yet
Development

No branches or pull requests

4 participants