-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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 sharded maps for interning #61779
Conversation
☔ The latest upstream changes (presumably #61722) made this pull request unmergeable. Please resolve the merge conflicts. |
LGTM, modulo the concurrent data structure, which I don't know who should be reviewing that (@gankro?) I should mention that my uncertainties around "parallel rustc" in the past were mostly around using locks instead of concurrent data structures, and this is the kind of approach I was hoping to see. I'm now excited about being able to turn on "parallel rustc" by default this year! cc @rust-lang/compiler |
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.
just minor style nits on the map, it's not exactly subtle or interesting, afaict (literally just an array of Mutex<Map>
s where one is "randomly" selected for each value).
} | ||
} | ||
|
||
impl<K: Eq + Hash + Copy> ShardedHashMap<K, ()> { |
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.
Absent intent to actually use this type as a map, you might as well just make this ShardedHashSet?
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.
It doesn't have set operations, so I'd find that misleading.
where K: Borrow<Q>, | ||
Q: Hash + Eq | ||
{ | ||
let hash = make_hash(value); |
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.
Worth putting a comment here explaining that we can't use the map's hasher because we need the hash to find the map?
Also you could arguably do something silly like make a HashMap::default here just so this code is easier to change but... meh?
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 hash map's hasher is never used, so there isn't really a reason to access it.
match entry { | ||
RawEntryMut::Occupied(e) => *e.key(), | ||
RawEntryMut::Vacant(e) => { | ||
let v = make(); |
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.
it's a bit off to call this v when it's a key (same for other function)
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.
It's also the interned value we return =P
dbefa63
to
6a88dbe
Compare
☔ The latest upstream changes (presumably #61817) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably #61891) made this pull request unmergeable. Please resolve the merge conflicts. |
[WIP] Make dep node indices persistent between sessions This makes marking dep nodes green faster (and lock free in the case with no diagnostics). This change is split out from #60035. Unlike #60035 this makes loading the dep graph slower because it loads 2 copies of the dep graph, one immutable and one mutable. Based on #61845, #61779 and #61923.
Is anything blocking this now? |
☔ The latest upstream changes (presumably #62069) made this pull request unmergeable. Please resolve the merge conflicts. |
@bors try |
⌛ Trying commit 30239ab615197a5f7d6f388f41f952661dcbd1db with merge 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab... |
☀️ Try build successful - checks-travis |
@rust-timer build 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab |
Success: Queued 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab with parent a96ba96, comparison URL. |
Finished benchmarking try commit 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab, comparison URL. |
@bors r+ |
📌 Commit 0e73386 has been approved by |
Use sharded maps for interning Cuts down runtime from 5.5s to 3.8s for non-incremental `syntex_syntax` check builds with 16 threads / 8 cores. r? @eddyb
💥 Test timed out |
@bors retry |
Use sharded maps for interning Cuts down runtime from 5.5s to 3.8s for non-incremental `syntex_syntax` check builds with 16 threads / 8 cores. r? @eddyb
Use sharded maps for interning Cuts down runtime from 5.5s to 3.8s for non-incremental `syntex_syntax` check builds with 16 threads / 8 cores. r? @eddyb
☀️ Test successful - checks-azure |
Use sharded maps for queries Based on rust-lang#61779. r? @gankro
Use sharded maps for queries Based on rust-lang#61779. r? @gankro
Use sharded maps for queries Based on rust-lang#61779. r? @gankro
Cuts down runtime from 5.5s to 3.8s for non-incremental
syntex_syntax
check builds with 16 threads / 8 cores.r? @eddyb