Skip to content

fix timings order across browsers #847

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 1 commit into from
Mar 4, 2021

Conversation

klensy
Copy link
Contributor

@klensy klensy commented Mar 4, 2021

Currently order if timing differs in different browsers. For example, check this page in chrome and firefox https://perf.rust-lang.org/compare.html?start=15598a83db88ec7a32ea18a44dd6309f32edc07e&end=f288a8e816163b97425adb1baca86b573395e3ec , you can see that in chrome, bootstrap timings goes first, but in firefox - last.

Fixes by setting is_bootstrap=false for non bootstrap, and then sorting (as is_bootstrap became true\false always, instead being none sometimes).

@Mark-Simulacrum
Copy link
Member

Interesting! I was not aware of this difference. Thanks.

@Mark-Simulacrum Mark-Simulacrum merged commit aac84c1 into rust-lang:master Mar 4, 2021
@Mark-Simulacrum
Copy link
Member

Ah, bootstrap timings going last was actually the intent, so I think this swapped the ordering I was expecting -- I'll fix that up.

@klensy
Copy link
Contributor Author

klensy commented Mar 4, 2021

Hmm, this actually broke ordering on not bootstrap https://perf.rust-lang.org/compare.html?start=2021-02-01&end=2021-03-01&stat=instructions%3Au

@Mark-Simulacrum
Copy link
Member

I am not obviously seeing what you mean, but happy to see a PR if there's some additional breakage - I pushed up the ordering switch.

@klensy
Copy link
Contributor Author

klensy commented Mar 4, 2021

For bootstrap - sorted by right column, but for not bootstrap - should be sorted by abs(avg) ?

@Mark-Simulacrum
Copy link
Member

No, we should be sorting by abs(max change) I think, something like that? i.e., the farthest_pct field is right I believe

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.

2 participants