Skip to content

Simplify artifact comparison relevance #1281

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
Apr 7, 2022

Conversation

rylev
Copy link
Member

@rylev rylev commented Apr 6, 2022

Based on discussion in #1277, this PR drastically simplifies how we think about relevance of an artifact comparison. If there are any relevant test result comparisons, then the artifact comparison as a whole is deemed relevant.

We should expect that more items will appear on the triage report.

@nnethercote - I like this change for the GitHub messages we post, but I'm less keen on this for the triage report. Perhaps for the triage report we can be a bit smarter. For example, if the primary summary has any comparisons OR the secondary summary has any medium or above test result comparison or 10 or more small ones. This would at least mean the triage filer doesn't need to filter out those that are just composed of small changes to secondary benchmarks. Thoughts?

@rylev rylev requested review from nnethercote and Kobzol April 6, 2022 12:03
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

👍 I think that it's a good idea to make this simpler first and only if we later find that there is some problem with it, we can introduce some additional logic. Deciding based on # and magnitude of relevant benchmarks in primary/secondary category sounds reasonable as a potential future step.

Co-authored-by: Jakub Beránek <[email protected]>
@rylev rylev force-pushed the simplify-relevance branch from d67f48a to 4e505cf Compare April 6, 2022 12:47
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

I agree with @Kobzol: let's try it and see how it goes, we can adjust if necessary.

It would be good to make sure the triagers know about this change.

@rylev
Copy link
Member Author

rylev commented Apr 7, 2022

cc @Mark-Simulacrum @pnkfelix - Just so you're aware, this change will cause many more PRs to show up on the triage report as perf improvements or regressions. I imagine we'll likely want to reinstate some logic that limits what shows up on the triage report but we're going back to a simple state (show anything that causes any relevant changes) and then rebuilding from there. I'll cc you on any additional changes.

@rylev rylev merged commit b0145e1 into rust-lang:master Apr 7, 2022
@rylev rylev deleted the simplify-relevance branch April 7, 2022 07:48
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.

3 participants