-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Make FileMap::{lines, multibyte_chars, non_narrow_chars} non-mutable. #50997
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
Conversation
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
@bors try |
Make FileMap::{lines, multibyte_chars, non_narrow_chars} non-mutable.
This PR removes most of the interior mutability from `FileMap`, which should be beneficial, especially in a multithreaded setting. This is achieved by initializing the state in question when the filemap is constructed instead of during lexing. Hopefully this doesn't degrade performance.
cc @wesleywiser
|
Nice job @michaelwoerister! It didn't even occur to me to try this kind of approach. IMO, this code is much easier to follow than the previous version since all of the special character handling is consolidated into one place. 👍 |
|
Yes, let's hope performance doesn't throw a wrench into the works |
|
Hm, UI tests also seem to have some trouble... |
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum, could I get a perf-run, please? |
|
|
||
| if self.ch.unwrap() == '\n' { | ||
| if self.save_new_lines_and_multibyte { | ||
| self.filemap.next_line(self.next_pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks if the current character is a newline, but then it passes next_pos into next_line
src/libsyntax_pos/lib.rs
Outdated
| if byte.is_ascii() { | ||
| match byte { | ||
| b'\n' => { | ||
| lines.push(byte_pos); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checks if the current character is a newline and then records the current position. Is this where the off-by one on the failing UI tests is coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's the problem. Good catch!
fbd1750 to
8c8d733
Compare
|
Should be fixed now. The perf run should still be valid, despite the change. |
|
Perf queued! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Thanks, @Mark-Simulacrum! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
c80e82b to
9a1b838
Compare
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
9a1b838 to
2874a87
Compare
|
I think something might have gone wrong with that previous perf run. Let's do another one. @bors try |
Make FileMap::{lines, multibyte_chars, non_narrow_chars} non-mutable.
This PR removes most of the interior mutability from `FileMap`, which should be beneficial, especially in a multithreaded setting. This is achieved by initializing the state in question when the filemap is constructed instead of during lexing. Hopefully this doesn't degrade performance.
cc @wesleywiser
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum, can we give this another try? |
|
Rebased. @bors r=Mark-Simulacrum |
|
📌 Commit ba30c1d has been approved by |
|
⌛ Testing commit ba30c1d with merge a9880eb97bcb8fac55e8b0b34aac9f42ee11a0f7... |
|
💔 Test failed - status-appveyor |
|
I can reproduce locally, will investigate. |
The method relied on the FileMap still being under construction in order for it to do what the name promises. It's now independent of the current state.
|
Fixed @bors r=Mark-Simulacrum |
|
📌 Commit a1f8a6c has been approved by |
…Simulacrum
Make FileMap::{lines, multibyte_chars, non_narrow_chars} non-mutable.
This PR removes most of the interior mutability from `FileMap`, which should be beneficial, especially in a multithreaded setting. This is achieved by initializing the state in question when the filemap is constructed instead of during lexing. Hopefully this doesn't degrade performance.
cc @wesleywiser
|
☀️ Test successful - status-appveyor, status-travis |
This PR removes most of the interior mutability from
FileMap, which should be beneficial, especially in a multithreaded setting. This is achieved by initializing the state in question when the filemap is constructed instead of during lexing. Hopefully this doesn't degrade performance.cc @wesleywiser