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

Performance regression on pest in newest nightly #47356

Closed
dragostis opened this issue Jan 11, 2018 · 2 comments
Closed

Performance regression on pest in newest nightly #47356

dragostis opened this issue Jan 11, 2018 · 2 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@dragostis
Copy link

dragostis commented Jan 11, 2018

Running the benchmark in pest_grammars on current nightly vs nightly 2017-12-04 yields 56% performance regression.

Current:

test data ... bench:      72,455 ns/iter (+/- 13,575)

2017-12-04:

test data ... bench:      46,380 ns/iter (+/- 1,352)
@dragostis dragostis changed the title Performance regression on pest in newst nightly Performance regression on pest in newest nightly Jan 11, 2018
@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 11, 2018
@alexcrichton
Copy link
Member

I believe this is a result of #46910 where LLVM's making a different inlining decision in the ThinLTO phases than it previously did when everything was in one codegen unit. That PR was merged with prior knowledge that it would not yield a 100% across-the-board improvement in all cases.

That being said, it's not clear why an inlining decision wasn't made here! The difference can be verified by adding:

[profile.bench]
codegen-units = 1

to the Cargo.toml at the root, and cargo bench should go back to what it previously was.

@Mark-Simulacrum Mark-Simulacrum added this to the 1.24 milestone Jan 15, 2018
@nikomatsakis
Copy link
Contributor

OK, based on @alexcrichton's analysis I'm going to close this issue as being an expected byproduct of enabling ThinLTO. @dragostis you might want to consider adding #[inline] to some of your hot methods; hopefully profiling could help you find which ones. Thanks for the report!

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

No branches or pull requests

4 participants