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

Missing start commit for 90891 #1163

Closed
bjorn3 opened this issue Jan 31, 2022 · 6 comments · Fixed by #1164
Closed

Missing start commit for 90891 #1163

bjorn3 opened this issue Jan 31, 2022 · 6 comments · Fixed by #1164
Assignees

Comments

@bjorn3
Copy link
Member

bjorn3 commented Jan 31, 2022

rust-lang/rust#90891 (comment)

@Mark-Simulacrum
Copy link
Member

It's in the queue - https://perf.rust-lang.org/status.html - I'm not sure why we benchmarked in the wrong order yet. My suspicion is that the PR result was incorrectly queued as a try build, which would prioritize it over master commits, though it would typically have the parent commit still precede it.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 31, 2022

Should I close this issue then?

@klensy
Copy link
Contributor

klensy commented Jan 31, 2022

1/30/2022, 11:57:34 PM | 08df8b81d6e72 | #92711 - in progress
1/31/2022, 7:23:21 AM | bb549e5afe8f2 | #93270
1/31/2022, 11:12:10 AM | 415c9f95884ca | #93499

No, it's not in queue? I mean, 90891, as it already failed, so other queued after 92711 will fail too.

@Mark-Simulacrum
Copy link
Member

The parent commit (rust-lang/rust@08df8b8) of #90891's merge (rust-lang/rust@e58e7b1) is in the queue -- currently in progress.

@klensy
Copy link
Contributor

klensy commented Jan 31, 2022

I thought, that if no parent for perf run exist, no benchmarks will be run at all, but i was wrong. It will run, but simply don't do comparison.
Perf run results for #90891 exist, so yes, upcoming runs should work :-)

@Mark-Simulacrum
Copy link
Member

OK, I think I likely know what happened:

I think it should be a relatively straightforward thing to change the queue code to fix this -- might actually represent a simplification since I think it'll probably mean dropping the prioritization logic.

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 a pull request may close this issue.

3 participants