Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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!

# for free to join this conversation on GitHub. Already have an account? # 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