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

15% regression in compiler memory usage for inflate crate #43834

Closed
alexcrichton opened this issue Aug 13, 2017 · 9 comments
Closed

15% regression in compiler memory usage for inflate crate #43834

alexcrichton opened this issue Aug 13, 2017 · 9 comments
Assignees
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. P-medium Medium priority 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

@alexcrichton
Copy link
Member

According to perf.rust-lang.org the inflate-0.1.0 benchmark regressed 15% in memory usage over the past few weeks.

There was an initial big regression due to #39409 and #43576 recovered most of it but unfortunately there's still a 15% overall regression from before :(

@alexcrichton alexcrichton added I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. 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 Aug 13, 2017
@alexcrichton
Copy link
Member Author

A similar, but not as large regression can be seen for the deep vector benchmark

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Aug 13, 2017
@arielb1
Copy link
Contributor

arielb1 commented Aug 17, 2017

Looks expected, but someone should run a profiler to confirm it's what it looks like.

@nikomatsakis
Copy link
Contributor

Believed to be fallout from EndRegion. In that case, we can probably just close as "expected", if unfortunate. Those are however likely to go away with the migration to NLL, so hopefully we will recover eventually. It'd be nice to do a quick profile to double check.

@alexcrichton alexcrichton modified the milestone: 1.21 Aug 23, 2017
@pnkfelix pnkfelix self-assigned this Aug 24, 2017
@nikomatsakis
Copy link
Contributor

triage: P-medium

@rust-highfive rust-highfive added the P-medium Medium priority label Aug 24, 2017
@alexcrichton alexcrichton added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Aug 28, 2017
@alexcrichton
Copy link
Member Author

1.21 is now in beta

@nikomatsakis should this not be on the milestone for 1.21?

@nikomatsakis
Copy link
Contributor

@alexcrichton we were hoping to verify the results and make sure we at least know the cause. @pnkfelix has done some of the legwork here but I think we're still not clear that EndRegion is to blame, yet.

jonhoo added a commit to jonhoo/fantoccini that referenced this issue Sep 1, 2017
@pnkfelix
Copy link
Member

pnkfelix commented Sep 1, 2017

Just some quick data to supply here: I switched to extern crate alloc_system; inrustc in order to get massif working.

When using valgrind --tool=massif, I was getting peak memory numbers for inflate like this:

Skip EndRegion Emit EndRegion ratio
198.1 MB 207.2 MB 1.046

This led me to think that there might be some other issue causing the regression being reported.

But after @nikomatsakis pushed back a little, I switched to using /bin/time, which on Linux includes peak memory usage in its report. The numbers then bounced around some, but on average they looked like this:

Skip EndRegion Emit EndRegion ratio
259.4 MB 329.4 MB 1.27

That was a lot scarier, and thus led me to file PR #44249.

(I don't know why the massif report is so different from the one I get from /bin/time. I have found myself repeating the phrase "'tis an ill workman that quarrels with his own tools" a lot over the past few days...)

@alexcrichton
Copy link
Member Author

Hm that is indeed very odd! I would have thought that massif wouldn't tamper too much with the output... I wonder if thats something like really bad heap fragmentation where massif sums up literal allocation sizes whereas /bin/time sums up the actual pages being used?

@nikomatsakis
Copy link
Contributor

Looking at perf, I see that the regression has been fixed by #44249. We'll have to wrangle this when it comes to switch to MIR borrowck, but we'll worry about then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC I-compilemem Issue: Problems and improvements with respect to memory usage during compilation. P-medium Medium priority 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

6 participants