Skip to content
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

Make CrateMetadata and CStore thread-safe #48936

Merged
merged 3 commits into from
Mar 17, 2018
Merged

Make CrateMetadata and CStore thread-safe #48936

merged 3 commits into from
Mar 17, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Mar 11, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2018
@Zoxc Zoxc force-pushed the cstore branch 3 times, most recently from 0d9b6ee to 2de21be Compare March 12, 2018 04:42
Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @Zoxc! Looks good to me.

What do you think of adding an InitOnce<T> primitive to rustc_data_structures::sync? Especially things like codemap_import_info would profit from this, I think. I haven't done any performance testing but even if it's not significantly faster, it makes the semantics much clearer which is always a win.

@@ -335,7 +334,9 @@ impl<'a> CrateLoader<'a> {
if data.root.macro_derive_registrar.is_some() {
dep_kind = DepKind::UnexportedMacrosOnly;
}
data.dep_kind.set(cmp::max(data.dep_kind.get(), dep_kind));
data.dep_kind.with_lock(|data_dep_kind| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a separate primitive for state that is initialized once and then never modified again. There must be more efficient ways to do this than a mutex.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parking_lot::Once looks like a good building block for such a thing.

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 12, 2018

I did think about adding something like InitOnce, but I think we'll end up using either queries (which cannot deadlock) or just refactoring away the internal mutability.

metadata_loader,
}
}

/// You cannot use this function to allocate a CrateNum in a thread-safe manner.
/// It is currently only used in CrateLoader which is single-threaded code.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment about this function. next_crate_num is used once in CrateLoader to get the next free CrateNum. CrateLoader may then create multiple CrateNums based on that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@michaelwoerister
Copy link
Member

I did think about adding something like InitOnce, but I think we'll end up using either queries (which cannot deadlock) or just refactoring away the internal mutability.

At least for codemap_import_info we won't want to use a query, I think.

@michaelwoerister
Copy link
Member

@bors r+

Let's merge this as is, if you don't mind, @Zoxc. I'll be looking into the whole crate store infrastructure a bit more in detail over the next few days.

@bors
Copy link
Contributor

bors commented Mar 14, 2018

📌 Commit 5b8f9c5 has been approved by michaelwoerister

@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 Mar 14, 2018
@bors
Copy link
Contributor

bors commented Mar 17, 2018

⌛ Testing commit 5b8f9c5 with merge adf2135...

bors added a commit that referenced this pull request Mar 17, 2018
Make CrateMetadata and CStore thread-safe

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Mar 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing adf2135 to master...

@bors bors merged commit 5b8f9c5 into rust-lang:master Mar 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants