Make CrateNum allocation more thread-safe. #50538

michaelwoerister · 2018-05-08T15:21:18Z

This PR makes sure that we can't have race conditions when assigning CrateNums. It's a slight improvement but a larger refactoring of the CrateStore/CrateLoader infrastructure would be good, I think.

r? @Zoxc

rust-highfive · 2018-05-08T15:39:17Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.


[00:04:45] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:46] tidy error: /checkout/src/librustc_metadata/cstore.rs:145: line longer than 100 chars
[00:04:47] some tidy checks failed
[00:04:47] 
[00:04:47] 
[00:04:47] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:47] 
[00:04:47] 
[00:04:47] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:47] Build completed unsuccessfully in 0:02:03
[00:04:47] Build completed unsuccessfully in 0:02:03
[00:04:47] make: *** [tidy] Error 1
[00:04:47] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:1d68d3bb
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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 @TimNN. (Feature Requests)

Zoxc · 2018-05-09T10:53:04Z

I would use the length of the CStore.metas field as the next CrateNum instead of using a separate counter. alloc_new_crate_num would then push None to metas before returning and set_crate_data should be changed to no longer expand metas.

michaelwoerister · 2018-05-09T11:34:05Z

Good idea, I'll do that.

michaelwoerister · 2018-05-09T16:09:36Z

Updated with the new approach. Much nicer this way.

Zoxc · 2018-05-09T17:40:31Z

src/librustc_metadata/cstore.rs

@@ -96,32 +96,30 @@ pub struct CStore {
 impl CStore {
    pub fn new(metadata_loader: Box<MetadataLoader + Sync>) -> CStore {
        CStore {
-            metas: RwLock::new(IndexVec::new()),
+            metas: RwLock::new(IndexVec::from_elem_n(None, 1)),


The first entry is for LOCAL_CRATE here right? Do we call set_crate_data for that?

Yes, that's for the local crate. The entry will always be None. It's just there to make array indices match CrateNums.

Could you add that as a comment?

Zoxc · 2018-05-09T18:55:46Z

@bors r+

bors · 2018-05-09T18:55:47Z

📌 Commit 0837e2a has been approved by Zoxc

rust-highfive · 2018-05-09T19:06:59Z

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux]
travis_fold:end:system_info

Network availability confirmed.
Running apt-get update by default has been disabled.
You can opt into running apt-get update by setting this in your .travis.yml file:
  apt:
travis_fold:start:git.checkout
travis_time:start:0d43a906
$ git clone --depth=2 https://github.com/rust-lang/rust.git rust-lang/rust
---

[00:05:11] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:05:11] tidy error: /checkout/src/librustc_metadata/cstore.rs:100: trailing whitespace
[00:05:12] some tidy checks failed
[00:05:12] 
[00:05:12] 
[00:05:12] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:05:12] 
[00:05:12] 
[00:05:12] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:05:12] Build completed unsuccessfully in 0:02:04
[00:05:12] Build completed unsuccessfully in 0:02:04
[00:05:12] Makefile:79: recipe for target 'tidy' failed
[00:05:12] make: *** [tidy] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01781892
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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 @TimNN. (Feature Requests)

michaelwoerister · 2018-05-09T19:34:04Z

Tidy fixed.

@bors r=Zoxc

bors · 2018-05-09T19:34:04Z

📌 Commit 4537025 has been approved by Zoxc

michaelwoerister · 2018-05-10T13:25:40Z

@bors rollup

@Zoxc

Make CrateNum allocation more thread-safe. This PR makes sure that we can't have race conditions when assigning CrateNums. It's a slight improvement but a larger refactoring of the CrateStore/CrateLoader infrastructure would be good, I think. r? @Zoxc

@Zoxc

Make CrateNum allocation more thread-safe. This PR makes sure that we can't have race conditions when assigning CrateNums. It's a slight improvement but a larger refactoring of the CrateStore/CrateLoader infrastructure would be good, I think. r? @Zoxc

Rollup of 18 pull requests Successful merges: - #49423 (Extend tests for RFC1598 (GAT)) - #50010 (Give SliceIndex impls a test suite of girth befitting the implementation (and fix a UTF8 boundary check)) - #50447 (Fix update-references for tests within subdirectories.) - #50514 (Pull in a wasm fix from LLVM upstream) - #50524 (Make DepGraph::previous_work_products immutable) - #50532 (Don't use Lock for heavily accessed CrateMetadata::cnum_map.) - #50538 ( Make CrateNum allocation more thread-safe. ) - #50564 (Inline `Span` methods.) - #50565 (Use SmallVec for DepNodeIndex within dep_graph.) - #50569 (Allow for specifying a linker plugin for cross-language LTO) - #50572 (Clarify in the docs that `mul_add` is not always faster.) - #50574 (add fn `into_inner(self) -> (Idx, Idx)` to RangeInclusive (#49022)) - #50575 (std: Avoid `ptr::copy` if unnecessary in `vec::Drain`) - #50588 (Move "See also" disambiguation links for primitive types to top) - #50590 (Fix tuple struct field spans) - #50591 (Restore RawVec::reserve* documentation) - #50598 (Remove unnecessary mutable borrow and resizing in DepGraph::serialize) - #50606 (Retry when downloading the Docker cache.) Failed merges: - #50161 (added missing implementation hint) - #50558 (Remove all reference to DepGraph::work_products)

rust-highfive assigned Zoxc May 8, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 8, 2018

Adapt some method visibilities in librustc_metadata::cstore.

31d2012

michaelwoerister force-pushed the atomic-cnums branch from 3402230 to 3cb76c3 Compare May 9, 2018 08:34

Make CrateNum allocation more thread-safe.

b8b957d

michaelwoerister force-pushed the atomic-cnums branch from 3cb76c3 to b8b957d Compare May 9, 2018 14:06

Zoxc reviewed May 9, 2018

View reviewed changes

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 May 9, 2018

Add comment about first element in CStore::metas.

4537025

michaelwoerister force-pushed the atomic-cnums branch from 0837e2a to 4537025 Compare May 9, 2018 19:33

alexcrichton mentioned this pull request May 10, 2018

Rollup of 19 pull requests #50608

Closed

alexcrichton mentioned this pull request May 10, 2018

Rollup of 18 pull requests #50611

Merged

bors merged commit 4537025 into rust-lang:master May 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CrateNum allocation more thread-safe. #50538

Make CrateNum allocation more thread-safe. #50538

michaelwoerister commented May 8, 2018

rust-highfive commented May 8, 2018

Zoxc commented May 9, 2018

michaelwoerister commented May 9, 2018

michaelwoerister commented May 9, 2018

Zoxc May 9, 2018

michaelwoerister May 9, 2018

Zoxc May 9, 2018

michaelwoerister May 9, 2018

Zoxc commented May 9, 2018

bors commented May 9, 2018

rust-highfive commented May 9, 2018

michaelwoerister commented May 9, 2018

bors commented May 9, 2018

michaelwoerister commented May 10, 2018

Make CrateNum allocation more thread-safe. #50538

Make CrateNum allocation more thread-safe. #50538

Conversation

michaelwoerister commented May 8, 2018

rust-highfive commented May 8, 2018

Zoxc commented May 9, 2018

michaelwoerister commented May 9, 2018

michaelwoerister commented May 9, 2018

Zoxc May 9, 2018

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

Zoxc May 9, 2018

Choose a reason for hiding this comment

michaelwoerister May 9, 2018

Choose a reason for hiding this comment

Zoxc commented May 9, 2018

bors commented May 9, 2018

rust-highfive commented May 9, 2018

michaelwoerister commented May 9, 2018

bors commented May 9, 2018

michaelwoerister commented May 10, 2018