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

Parser performance regression #6355

Closed
dotdash opened this issue May 9, 2013 · 6 comments
Closed

Parser performance regression #6355

dotdash opened this issue May 9, 2013 · 6 comments

Comments

@dotdash
Copy link
Contributor

dotdash commented May 9, 2013

I noticed that compiling libcore now takes about 26s instead of the 20s it used to take. perf revealed that a large amount of time is spent in the bump() method of the lexer. The offending line is:

        let next = str::char_range_at(*rdr.src, current_byte_offset);

With rdr being of type &mut StringReader and rdr.src being @~str. str::char_range_at expects a &str as its first argument.

What happens now is that the compiler generates code that copies the whole src string, which is obviously bad.

The copy is triggered by the rdr being mutable. Without the mut, the compiler doesn't generate a copy. A benchmark to demonstrate that behaviour can be found here: https://gist.github.com/dotdash/5546866

@nikomatsakis said he wants to look into this.

@dotdash
Copy link
Contributor Author

dotdash commented May 9, 2013

FWIW, the function call is not mandatory for the problem to show up, assignment to a temporary of type &str is enough to trigger the copy. Updated the benchmark code accordingly (the with_tmp part).

@graydon
Copy link
Contributor

graydon commented May 9, 2013

I noticed this too! I've been trying to figure it out for a couple days as it's blocking my gc work (the program leaks so much memory my machine crashes)

@graydon
Copy link
Contributor

graydon commented May 9, 2013

I think it might be from vecs-implicitly-copyable

@dotdash
Copy link
Contributor Author

dotdash commented May 9, 2013

AFAICT this is because the borrow checker decides that is has to root the value to be safe. See https://github.com/mozilla/rust/blob/incoming/src/librustc/middle/trans/write_guard.rs#L108 for the code that does the copy.

And https://github.com/mozilla/rust/blob/incoming/src/librustc/middle/borrowck/gather_loans/lifetime.rs#L92 is the place where the optimization for the immutable case happens to avoid the copy.

@nikomatsakis
Copy link
Contributor

I believe this is due to #6272. I am in the processing of fixing it.

@nikomatsakis
Copy link
Contributor

I have verified that this is a dup of #6272. Closing in favor of that issue.

bors added a commit that referenced this issue May 9, 2013
…=graydon

Fix #6355 and #6272---we were not giving the correct index to the derefs that occur as part of the rooting process, resulting in extra copies and generally bogus behavior. Haven't quite produced the right test for this, but I thought I'd push the fix in the meantime. Test will follow shortly.

r? @graydon
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants