-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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
Use a sharded symbol interner #89073
Conversation
This should reduce contention when accessing the symbol interner from multiple threads when using parallel rustc.
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 4304f75 with merge 33591cc45aff24a0a9dd4810862f7df559993f1e... |
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.
Are you planning to benchmark parallel rustc yourself? perf.r-l.o does not measure parallel performance currently; you'd need to do the "double try" game with & without your changes to test on perf.rlo.
string.hash(&mut state); | ||
let hash = state.finish(); | ||
|
||
let mut shard = self.0[(hash & 0xff) as usize].lock(); |
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.
Shouldn't you hash >> 8
or similar here? Otherwise the hashmap is going to be colliding constantly since the low bits are used to index into the backing storage, IIRC.
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.
The symbol integer is created as index_within_shard << 8 | shard_num
. The shard num is determined by hash & 0xff
.
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.
Maybe I'm missing something, but shard.names.raw_entry_mut().from_key_hashed_nocheck(hash, string)
looks like it's using just the plain hash, not shifting based on the sharding. I think the low bits there will always be the same, which seems like a problem.
The job Click to see the possible cause of the failure (guessed by this bot)
|
rust/compiler/rustc_span/src/symbol.rs Lines 1795 to 1809 in d6cd2c6
Oops, didn't notice this. This of course isn't going to work with sharded symbol lists. |
The job Click to see the possible cause of the failure (guessed by this bot)
|
💔 Test failed - checks-actions |
My laptop is unfortunately not suitable for benchmarking. Wall time easily differs several percents between runs, likely shadowing any improvements made by this PR. Instruction count is probably accurate, but reducing lock contention should mostly improve wall time and not instruction count by very much. |
You could replace this with an
How would you measure locally? If measuring the wall time of building stage 2 with your and master's stage 1 is a reasonable approximation, then I have a cloud desktop that I can use that builds relatively quickly that I can through this PR on. |
☔ The latest upstream changes (presumably #91469) made this pull request unmergeable. Please resolve the merge conflicts. |
I won't have time to work on this in the near future. If anyone else wants to pick this back up, feel free. |
This should reduce contention when accessing the symbol interner from multiple threads when using parallel rustc.