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.

Sign up for GitHub

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

Jump to bottom

A perf regression between 2018-12-20 and 2018-12-21 #57028

Open
ljedrz opened this issue Dec 21, 2018 · 5 comments · Fixed by #57033
Open

A perf regression between 2018-12-20 and 2018-12-21 #57028

ljedrz opened this issue Dec 21, 2018 · 5 comments · Fixed by #57033
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@ljedrz
Copy link
Contributor

ljedrz commented Dec 21, 2018 •
edited
Loading

Instructions and wall-time took a noticeable hit due to one of these changes.

instructions:

style-servo-check
	avg: 43.0%	min: 23.4%	max: 55.9%
style-servo-opt
	avg: 14.4%	min: 2.9%	max: 49.5%
style-servo-debug
	avg: 20.5%	min: 7.1%	max: 44.0%
sentry-cli-check
	avg: 29.6%	min: 14.9%	max: 41.4%
inflate-check
	avg: 10.6%	min: 3.4%	max: 33.6%
syn-check
	avg: 23.9%	min: 13.3%	max: 28.6%
inflate-opt
	avg: 7.3%?	min: 0.8%?	max: 26.0%?
inflate-debug
	avg: 9.2%	min: 3.5%	max: 25.6%
syn-debug
	avg: 12.7%	min: 6.9%	max: 20.4%
syn-opt
	avg: 6.6%?	min: 1.9%?	max: 20.3%?
sentry-cli-opt
	avg: 5.4%	min: 1.1%	max: 18.4%
ripgrep-check
	avg: 12.0%	min: 6.4%	max: 16.1%

wall-time:

style-servo-check
	avg: 21.8%	min: 12.3%	max: 29.1%
style-servo-opt
	avg: 11.5%	min: 5.3%	max: 22.2%
sentry-cli-check
	avg: 15.9%	min: 7.9%	max: 21.3%
syn-check
	avg: 13.6%	min: 7.6%	max: 18.6%
style-servo-debug
	avg: 14.2%	min: 7.8%	max: 18.4%
inflate-check
	avg: 10.1%	min: 7.2%	max: 17.0%
inflate-opt
	avg: 4.3%?	min: 0.3%?	max: 14.4%?
inflate-debug
	avg: 7.3%	min: 4.9%	max: 12.7%
syn-opt
	avg: 5.5%?	min: 3.4%?	max: 10.2%?

Cc @varkor (I narrowed it down)
@nikic
Copy link
Contributor

nikic commented Dec 21, 2018

This is likely due to #51085 (comment). The inhabitedness code is now used in a lot more places, and it's extremely slow.

@nikic nikic added the I-slow Issue: Problems and improvements with respect to performance of generated code. label Dec 21, 2018
@nikic nikic mentioned this issue Dec 21, 2018
Merged
@nikic nikic added I-compiletime Issue: Problems and improvements with respect to compile times. and removed I-slow Issue: Problems and improvements with respect to performance of generated code. labels Dec 21, 2018
bors added a commit that referenced this issue Dec 21, 2018
@bors
Auto merge of #57033 - nikic:inhabitedness-perf, r=varkor
a9ff135 
Remove "visited" set from inhabitedness checking (fix perf regression)

Now that references are no longer recursively checked, this should no longer be necessary, and it's a major performance bottleneck.

This should fix #57028.

r? @varkor
@estebank estebank added I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 22, 2018
@ljedrz
Copy link
Contributor Author

ljedrz commented Dec 22, 2018 •
edited
Loading

It was an improvement, but it doesn't look like the issue is solved.

I'd reopen, but I can't override bors.

@nikic nikic reopened this Dec 22, 2018
@nikic
Copy link
Contributor

nikic commented Dec 22, 2018

Yeah, clearly that wasn't (all of) it. I remember another perf issue in this code (set intersections are not lazy -- they continue even if the intersection is already empty), but will have to profile to make sure this is really the right issue this time...

@nikic nikic mentioned this issue Dec 22, 2018
Merged
bors added a commit that referenced this issue Dec 22, 2018
@bors
Auto merge of #57060 - nikic:inhabit-perf-2, r=varkor
fa922ab 
Short-circuit DefIdForest::intersection()

If the forest is already empty, there is no point in intersecting further.

Also handle the first element separately, so we don't compute an unnecessary intersection between the full forest and the first element, which is always equal to the first element.

This is the second try at fixing #57028, as the previous attempt only recovered part of the regression. I checked locally that this drops time spent in ty::inhabitedness for syn-check a lot, though not to zero.

r? @varkor
@nikic
Copy link
Contributor

nikic commented Dec 22, 2018

With the second fix the really large regression is gone, but we're still not at parity: https://perf.rust-lang.org/compare.html?start=2018-12-20&end=fa922ab876cb3140de2ead9c8ad88a75982c167c&stat=instructions%3Au

This code will need some more optimization, or possibly needs to be turned into a query so it can be cached.

@steveklabnik
Copy link
Member

Triage: almost two years later, it's not clear to me exactly what this would be tracking these days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-compiletime Issue: Problems and improvements with respect to compile times. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove "visited" set from inhabitedness checking (fix perf regression) nikic/rust
4 participants
@steveklabnik @nikic @estebank @ljedrz