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

A big performance regression in benchmarks #70785

Closed
ljedrz opened this issue Apr 4, 2020 · 6 comments · Fixed by #70803
Closed

A big performance regression in benchmarks #70785

ljedrz opened this issue Apr 4, 2020 · 6 comments · Fixed by #70803
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ljedrz
Copy link
Contributor

ljedrz commented Apr 4, 2020

rustc performance data shows a pretty big regression between 74bd074e and 6050e52. I don't know if this is expected, but I'm putting this out there in case others see the dent in perf charts and wonder what had happened.


EDIT(@eddyb): we how have a single-PR perf comparison showing the regression for #69718.

#70156, the other PR in the range, is much more of a mixed bag (but only for a small subset of runs).

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. I-nominated regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 4, 2020
@jonas-schievink
Copy link
Contributor

cc @arlosi @eddyb

@eddyb

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

9e55101 is in the queue: https://perf.rust-lang.org/status.html.

@arlosi
Copy link
Contributor

arlosi commented Apr 5, 2020

It looks like this was caused by #69718. I'll start investigating why.

I timed several compilations of ripgrep locally in debug with and without #69718.

master (sec): 41 40 39 40 40
revert (sec): 34 34 34 36 34

@eddyb
Copy link
Member

eddyb commented Apr 5, 2020

For the record, this is the single-PR perf comparison showing the regression from #69718.
I'll update the issue description to minimize potential confusion.

@arlosi Two things I'm seeing:

  1. there are way worse regressions than ripgrep, you can focus on the worse ones to get better contrast in measurements (e.g. tokio-webpush-simple-debug even has "patched incremental" runs slowed down a lot, unlike many others, clap-rs-debug being another example I guess)
  2. the detailed profile (if you click the percentage for a run) shows a lot of LLVM differences, e.g. for tokio-webpush-simple-debug "clean", and most of the regressions are -debug, with effectively no -check ones, so I think we might be able to get away without a revert, just adding a flag controlling whether LLVM sees these hashes (assuming that's the source of the slowdown)

@eddyb
Copy link
Member

eddyb commented Apr 5, 2020

We have perf data for the fix (#70803), which at a glance looks right.

Before the regression to after the fix looks like noise, and so does after the regression to before the fix, so I believe this confirms the fix is complete.

For the cost of MD5 hashing compared to SipHash, based on the changes to -check runs in the original regression, it looks insignificant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants