Skip to content

Improve the summary message both in GitHub and in the triage report #1156

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

Merged
merged 2 commits into from
Jan 25, 2022

Conversation

rylev
Copy link
Member

@rylev rylev commented Jan 21, 2022

This improves the summaries generated for both the perf bot on GitHub as well as the weekly triage report.

Note: I've not tested this locally as I don't have a local database in a useable form anymore.

Perf Bot

before

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 2.2% on incr-patched: reverse builds of regex check)

after

Summary: This benchmark run shows 10 relevant regressions 😿 to instruction counts.

  • Average relevant regression: 1.7%
  • Largest relevant regression in instruction count: 2.2% on incr-patched: reverse builds of regex check

Triage Report

before

after

Other Changes

Additionally, this PR changes the message slightly when there are no "relevant" changes (and thus nothing needs to be done) but there are still some (small) statistically significant changes. The following is added in the perf bot message.

"N results were found to be statistically significant but too small to be relevant."

@rylev rylev requested a review from Mark-Simulacrum January 21, 2022 16:17
@Mark-Simulacrum
Copy link
Member

Feel free to merge with nit fixed

@nnethercote
Copy link
Contributor

Oh! I've been thinking about this stuff, and have been meaning to make some changes myself.

  • I would like the message to say how many relevant improvements/unchangeds/regressions there were.
  • I would like finer classification of "mixed results" cases. Sometimes they are genuinely mixed, e.g. half improvements and half regressions. But sometimes they are like 90% improvements and 10% regressions, or vice versa. Adding a "mostly positive" or "mostly negative" classification might help here? Though perhaps the previous point would reduce the need for this.

What do you think?

@rylev
Copy link
Member Author

rylev commented Jan 25, 2022

  • I would like the message to say how many relevant improvements/unchangeds/regressions there were.

I believe this is what I already have (it shows the number of improvements and regressions) except it doesn't say the number of "unchanged". Do you want the number of unchanged to be the number of test cases where there was no significant change - this is essentially just all other test cases that aren't improvements or regressions.

  • I would like finer classification of "mixed results" cases. Sometimes they are genuinely mixed, e.g. half improvements and half regressions. But sometimes they are like 90% improvements and 10% regressions, or vice versa. Adding a "mostly positive" or "mostly negative" classification might help here? Though perhaps the previous point would reduce the need for this.

We could do this, but it might be useful to first see how these changes look in practice before figuring out the line between "genuinely mixed" and "mixed but leaning one way predominantly". What do you think?

@rylev rylev merged commit f5af99b into rust-lang:master Jan 25, 2022
@rylev rylev deleted the improve-summary branch January 25, 2022 19:14
@camelid
Copy link
Member

camelid commented Jan 25, 2022

Maybe "important" would be a better word than "relevant"? The distinction between "significant" and "relevant" seems a bit fuzzy, but "significant" and "important" seem better separated.

@nnethercote
Copy link
Contributor

Do you want the number of unchanged to be the number of test cases where there was no significant change - this is essentially just all other test cases that aren't improvements or regressions.

I want this in the summary, basically:

Screen Shot 2022-01-26 at 10 02 23 am

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants