Skip to content

Use sharded maps for interning#61779

Merged
bors merged 1 commit intorust-lang:masterfrom
Zoxc:sharded
Jul 23, 2019
Merged

Use sharded maps for interning#61779
bors merged 1 commit intorust-lang:masterfrom
Zoxc:sharded

Conversation

@Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jun 12, 2019

Cuts down runtime from 5.5s to 3.8s for non-incremental syntex_syntax check builds with 16 threads / 8 cores.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2019
@bors
Copy link
Collaborator

bors commented Jun 12, 2019

☔ The latest upstream changes (presumably #61722) made this pull request unmergeable. Please resolve the merge conflicts.

@eddyb
Copy link
Member

eddyb commented Jun 12, 2019

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.
(There are, of course, more drastic things one can do, such as replacing pointer equality with hash equality, that would allow threads to cooperate despite duplication, but those are less clearly overall wins)

I'm now excited about being able to turn on "parallel rustc" by default this year!

cc @rust-lang/compiler

Gankra
Gankra reviewed Jun 12, 2019
View reviewed changes
Gankra
Gankra reviewed Jun 12, 2019
View reviewed changes
Copy link
Contributor

@Gankra Gankra left a 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).

src/librustc_data_structures/sharded.rs Outdated
Copy link
Contributor

@Gankra Gankra Jun 12, 2019

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?

Copy link
Contributor Author

@Zoxc Zoxc Jun 12, 2019

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.

src/librustc_data_structures/sharded.rs Outdated
Copy link
Contributor

@Gankra Gankra Jun 12, 2019

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?

Copy link
Contributor Author

@Zoxc Zoxc Jun 12, 2019

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.

src/librustc_data_structures/sharded.rs Outdated
Copy link
Contributor

@Gankra Gankra Jun 12, 2019

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)

Copy link
Contributor Author

@Zoxc Zoxc Jun 12, 2019

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

@Zoxc Zoxc force-pushed the sharded branch 2 times, most recently from dbefa63 to 6a88dbe Compare June 13, 2019 18:42
@Zoxc Zoxc mentioned this pull request Jun 14, 2019
Closed
@bors
Copy link
Collaborator

bors commented Jun 14, 2019

☔ The latest upstream changes (presumably #61817) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Jun 18, 2019

☔ The latest upstream changes (presumably #61891) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc Zoxc mentioned this pull request Jun 21, 2019
Closed
bors added a commit that referenced this pull request Jun 22, 2019
@bors
Auto merge of #62038 - Zoxc:pre-gc-dep-graph, r=<try>
b397c11 
[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.
@Zoxc
Copy link
Contributor Author

Zoxc commented Jun 22, 2019

Is anything blocking this now?

@bors
Copy link
Collaborator

bors commented Jun 23, 2019

☔ The latest upstream changes (presumably #62069) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc
Copy link
Contributor Author

Zoxc commented Jun 23, 2019

@bors try

@bors
Copy link
Collaborator

bors commented Jun 23, 2019

⌛ Trying commit 30239ab615197a5f7d6f388f41f952661dcbd1db with merge 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab...

@bors
Copy link
Collaborator

bors commented Jun 23, 2019

☀️ Try build successful - checks-travis
Build commit: 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab

@Zoxc
Copy link
Contributor Author

Zoxc commented Jun 23, 2019

@rust-timer build 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 23, 2019

Success: Queued 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab with parent a96ba96, comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 23, 2019

Finished benchmarking try commit 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab, comparison URL.

@Zoxc Zoxc changed the title Use sharded maps for interning and queries [WIP] Use sharded maps for interning and queries Jun 24, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jul 20, 2019

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 20, 2019

📌 Commit 0e73386 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 20, 2019
@bors
Copy link
Collaborator

bors commented Jul 20, 2019

⌛ Testing commit 0e73386 with merge 3fe8285...

bors added a commit that referenced this pull request Jul 20, 2019
@bors
Auto merge of #61779 - Zoxc:sharded, r=oli-obk
3fe8285 
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
@bors
Copy link
Collaborator

bors commented Jul 20, 2019

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 20, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jul 22, 2019

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 22, 2019
@Centril
Rollup merge of rust-lang#61779 - Zoxc:sharded, r=oli-obk
c3aa3d8 
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
@Centril Centril mentioned this pull request Jul 22, 2019
Closed
@bors
Copy link
Collaborator

bors commented Jul 23, 2019

⌛ Testing commit 0e73386 with merge 3ebca72...

bors added a commit that referenced this pull request Jul 23, 2019
@bors
Auto merge of #61779 - Zoxc:sharded, r=oli-obk
3ebca72 
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
@bors bors mentioned this pull request Jul 23, 2019
Merged
@bors
Copy link
Collaborator

bors commented Jul 23, 2019

☀️ Test successful - checks-azure
Approved by: oli-obk
Pushing 3ebca72 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 23, 2019
@bors bors merged commit 0e73386 into rust-lang:master Jul 23, 2019
@Zoxc Zoxc deleted the sharded branch July 24, 2019 02:05
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 11, 2019
@Mark-Simulacrum
Rollup merge of rust-lang#62108 - Zoxc:sharded-queries, r=oli-obk
6381937 
Use sharded maps for queries

Based on rust-lang#61779.

r? @gankro
Centril added a commit to Centril/rust that referenced this pull request Aug 12, 2019
@Centril
Rollup merge of rust-lang#62108 - Zoxc:sharded-queries, r=oli-obk
4f1cc83 
Use sharded maps for queries

Based on rust-lang#61779.

r? @gankro
Centril added a commit to Centril/rust that referenced this pull request Aug 12, 2019
@Centril
Rollup merge of rust-lang#62108 - Zoxc:sharded-queries, r=oli-obk
0b4823d 
Use sharded maps for queries

Based on rust-lang#61779.

r? @gankro
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Feb 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oli-obk oli-obk oli-obk left review comments

+1 more reviewer

@Gankra Gankra Gankra left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@oli-obk oli-obk

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Zoxc @bors @eddyb @rust-timer @oli-obk @Gankra @rust-highfive