Skip to content

Add ability to filter out 'very small' changes #1023

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
Sep 21, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Sep 21, 2021

Fixes #1006.

This introduces a new filter (on by default) which filters out any changes that are deemed to have a "very small" magnitude. You can find the definition for magnitude here.

@Mark-Simulacrum
Copy link
Member

I think we should make sure that the comparison view always show any regressions (or improvemnts) we report in the summary comments / perf triage reports by default. Otherwise I think people will be confused. AFAICT at a quick skim, those comments currently do show 'very small' changes, so this would be a step away from that.

@Mark-Simulacrum
Copy link
Member

Not sure how we should fix that, though -- I lean towards not showing very small changes in perf summaries, under the assumption that they are likely to be irrelevant in practice (same reason we're hiding them in comparison pages).

@rylev
Copy link
Member Author

rylev commented Sep 21, 2021

So we only show the largest individual changes for either improvements or regressions in the triage and PR summaries. Because of this, those changes are only going to be "very small" if there are a decent number of very small changes. So it is true that it's possible that the comparison page doesn't show the change but the summary does, but only in specific situations.

There's only one way to fully fix this IMO: Don't use very small changes at all in determining perf categorizations. Thoughts on this?

@Mark-Simulacrum
Copy link
Member

Yeah, I think that's the way to go.

@rylev
Copy link
Member Author

rylev commented Sep 21, 2021

@Mark-Simulacrum I've gone ahead and filtered out very small changes from the categorization determination.

Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like CI is failing but otherwise looks good to me.

Let's merge this in and flip the default for new significance in a few days (I'll be going through the untriaged regression PRs and get a sense for how things look).

@rylev rylev force-pushed the filter-by-magnitude branch from dafc454 to 5a2ec92 Compare September 21, 2021 17:05
@rylev rylev merged commit 24e25c6 into rust-lang:master Sep 21, 2021
@rylev rylev deleted the filter-by-magnitude branch September 21, 2021 18:06
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.

Add filtering by "magnitude" to the comparison page
2 participants