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.

Sign up for GitHub

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

Jump to bottom

Use sharded maps for interning #61779

Merged

bors merged 1 commit into rust-lang:master from Zoxc:sharded

Jul 23, 2019

Contributor

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.

rust-highfive assigned eddyb

rust-highfive added the S-waiting-on-review label

Zoxc force-pushed the sharded branch from 1abf914 to c82b67a Compare

June 12, 2019 16:24

Contributor

bors commented Jun 12, 2019

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

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 reviewed

View reviewed changes

src/librustc_data_structures/sharded.rs Outdated Show resolved Hide resolved

Zoxc force-pushed the sharded branch from c82b67a to 3101ccc Compare

June 12, 2019 17:31

Gankra reviewed

View reviewed changes

Contributor

Gankra left a comment

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 Show resolved Hide resolved

src/librustc_data_structures/sharded.rs Outdated Show resolved Hide resolved

src/librustc_data_structures/sharded.rs

    
                  }

              }

              impl<K: Eq + Hash + Copy> ShardedHashMap<K, ()> {

Contributor

Gankra Jun 12, 2019

Absent intent to actually use this type as a map, you might as well just make this ShardedHashSet?

Contributor Author

Zoxc Jun 12, 2019

It doesn't have set operations, so I'd find that misleading.

src/librustc_data_structures/sharded.rs

    
                      where K: Borrow<Q>,

                            Q: Hash + Eq

                  {

                      let hash = make_hash(value);

Contributor

Gankra Jun 12, 2019

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?

Contributor Author

Zoxc Jun 12, 2019

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 Show resolved Hide resolved

src/librustc_data_structures/sharded.rs

    
                      match entry {

                          RawEntryMut::Occupied(e) => *e.key(),

                          RawEntryMut::Vacant(e) => {

                              let v = make();

Contributor

Gankra Jun 12, 2019

it's a bit off to call this v when it's a key (same for other function)

Contributor Author

Zoxc Jun 12, 2019

It's also the interned value we return =P

Zoxc force-pushed the sharded branch 2 times, most recently from dbefa63 to 6a88dbe Compare

June 13, 2019 18:42

Zoxc mentioned this pull request

[WIP] Use a sharded dep node to dep node index map #61845

Closed

Contributor

bors commented Jun 14, 2019

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

Zoxc force-pushed the sharded branch from 6a88dbe to 537c1d1 Compare

June 14, 2019 19:22

Contributor

bors commented Jun 18, 2019

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

Zoxc mentioned this pull request

Make dep node indices persistent between sessions #62038

Closed

bors added a commit that referenced this pull request


          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 force-pushed the sharded branch from 537c1d1 to 3132775 Compare

June 22, 2019 20:38

Contributor Author

Zoxc commented Jun 22, 2019

Is anything blocking this now?

Contributor

bors commented Jun 23, 2019

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

Zoxc force-pushed the sharded branch from 3132775 to 30239ab Compare

June 23, 2019 03:38

Contributor Author

Zoxc commented Jun 23, 2019

Contributor

bors commented Jun 23, 2019

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

Contributor

bors commented Jun 23, 2019

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

Contributor Author

Zoxc commented Jun 23, 2019

@rust-timer build 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab

Collaborator

rust-timer commented Jun 23, 2019

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

Collaborator

rust-timer commented Jun 23, 2019

Finished benchmarking try commit 62b9c2cb74e80279d8090fafc17fb5bae1fd6fab, comparison URL.

Zoxc changed the title ~~Use sharded maps for interning and queries~~ [WIP] Use sharded maps for interning and queries


          Use sharded maps for interning

0e73386

Zoxc force-pushed the sharded branch from c5e7d05 to 0e73386 Compare

July 19, 2019 21:38

Contributor

oli-obk commented Jul 20, 2019

Contributor

bors commented Jul 20, 2019

📌 Commit 0e73386 has been approved by oli-obk

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Contributor

bors commented Jul 20, 2019

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

bors added a commit that referenced this pull request


          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

Contributor

bors commented Jul 20, 2019

💥 Test timed out

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Contributor

oli-obk commented Jul 22, 2019

@bors retry

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Centril added a commit to Centril/rust that referenced this pull request


          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 mentioned this pull request

Rollup of 12 pull requests #62882

Closed

Contributor

bors commented Jul 23, 2019

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

bors added a commit that referenced this pull request


          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 mentioned this pull request

Update RLS and Rustfmt #62805

Merged

Contributor

bors commented Jul 23, 2019

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

bors added the merged-by-bors label

bors merged commit 0e73386 into rust-lang:master

Zoxc deleted the sharded branch

July 24, 2019 02:05

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request


          Rollup merge of rust-lang#62108 - Zoxc:sharded-queries, r=oli-obk

Use sharded maps for queries

Based on rust-lang#61779.

r? @gankro

Centril added a commit to Centril/rust that referenced this pull request


          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


          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 mentioned this pull request

Avoid query cache sharding code in single-threaded mode #94084

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged-by-bors S-waiting-on-bors