Skip to content
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

why did rust#96214 get tagged as regression? #1302

Closed
pnkfelix opened this issue Apr 21, 2022 · 2 comments
Closed

why did rust#96214 get tagged as regression? #1302

pnkfelix opened this issue Apr 21, 2022 · 2 comments

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Apr 21, 2022

Spawned off of rust-lang/rust#96214 (comment)

Basically, I'm trying to understand how the PR sequence of rust-lang/rust#95379; rust-lang/rust#96020; rust-lang/rust#96214 ended up with both PR rust-lang/rust#96020 and PR rust-lang/rust#96214 being flagged as regressions.

From my perspective, the data next to no change between rust-lang/rust#96020 and rust-lang/rust#96214 on this benchmark, at least from eyeballing the graph:

image

(but it could be that I'm very tired and overlooking something)

@nnethercote
Copy link
Contributor

Here's the changes for #96020:
d1

Here's the changes for #96214:
d2

The profiles/scenarios only have partial overlap. The graph you showed above is for one profile/scenario combination where the first commit made a significant regression but the second commit didn't. So it's a case where looking at one graph is misleading, and the text on the "compare" page is more instructive.

Also, the raw data values match up like you'd expect. E.g. look at the check full results in both tables.

In conclusion, I think the perf bot did the right thing here.

@pnkfelix
Copy link
Member Author

pnkfelix commented Apr 22, 2022

Okay, so the mistake I made was in focusing in on the incr-full line while looking into rust-lang/rust#96214, when I should have been looking at the incr-unchanged line, right?

Hmm: https://perf.rust-lang.org/index.html?start=2022-03-20&end=4ca19e09d302a4cbde14f9cb1bc109179dc824cd&benchmark=deeply-nested-multi&profile=opt&stat=instructions:u

and zooming in:

image

I can at least now see how rust-lang/rust#96214 got tagged.

(I had to zoom in a lot to see this picture, though.)

The difference for this profile/scenario are dwarfed by the differences observed in the other profiles/scenarios. I'm still musing about whether its "right" that this got tagged as a regression, but it was a good thing to have to dig into, I guess...

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

No branches or pull requests

2 participants