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

Turn crate store into a resolver output #65625

Merged
merged 10 commits into from Oct 25, 2019

Conversation

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 20, 2019

Crate store (CStore) is a vector of data (CrateMetadata) associated with extern crates loaded during the current compilation session.

All crates are loaded in the resolver when resolving either paths pointing to extern prelude or extern crate items. (There are also a couple of crates like panic runtime that are loaded kind of like implicit extern crates, but that also happens in resolve.)

The use of CStore from rustc_plugin (which is outside of the resolver) was unnecessary because legacy plugins are not added to the crate store and don't use CrateNums.

So, CStore can be produced by the resolver instead of being kept in some really global data (rustc_interface::Compiler) like now.

As a result of crate store being more "local" we can now remove some locks and Lrcs.

pub struct CStore {
metas: RwLock<IndexVec<CrateNum, Option<Lrc<CrateMetadata>>>>,
crate metadata_loader: Box<dyn MetadataLoader + Sync>,
metas: IndexVec<CrateNum, Option<Lrc<CrateMetadata>>>,

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Oct 20, 2019

Author Contributor

Unfortunately we cannot remove this Lrc because CStore has to be cloneable.
EDIT: The reason - #65625 (comment).

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 20, 2019

Member

Wow, I didn't even realize that was a thing. Do you know where the clones are coming from?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

I guess we could get rid of it by having the expansion query produce the CStore structure and redirect later uses to that instead. That would make the structure more like before this PR. It doesn't seem like a huge win right now though. It might be worth waiting on moving things into TyCtxt. Maybe we'd replace it by a crate_metadata(CrateNum) -> &'tcx CrateMetadata query or something.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Oct 21, 2019

Author Contributor

Yeah, this certainly can be done later if "later" is a better time due to other changes (like TyCtxt).
The clone itself (the primary effect) is cheap - only a vector of Lrcs is cloned.
The locks around CrateMetadata::{dependencies,dep_kind,extern_crate} (the secondary effect) are also unlikely to be accessed often and cause performance issues.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 20, 2019

Regarding locks in CrateMetadata:

  • source_map_import_info and dep_node_index are created lazily on the first use, so they need synchronization, but can probably use some of the Once primitives instead.
  • dependencies, dep_kind and extern_crate are only under locks because CrateMetadata is under Lrc (#65625 (comment)), they are used only from inside the crate loader and could just use mutable references otherwise.
@@ -211,7 +171,7 @@ impl BoxedResolver {
Err(resolver) => {
let resolver = &*resolver;
resolver.borrow_mut().access(|resolver| {
ExpansionResult::from_resolver_ref(resolver)
ExpansionResult::from_resolver_outputs(resolver.clone_outputs())

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Oct 20, 2019

Author Contributor

This is why we can't have nice things.
This case puts the cloneability requirement on resolver outputs (https://github.com/rust-lang/rust/pull/65625/files#r336780545), but it actually never happens in rustc, only in rustdoc and perhaps other tools using rustc_interface.

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 20, 2019

Member

Why would cloning be needed? Is this that support for resolving things after rustc_resolve finishes?
In that case I'm not sure why we need to clone the outputs - is it to be able to keep the original resolver around?

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Oct 20, 2019

Author Contributor

Not sure, rustc_interface is very hard to reason about. ¯_(ツ)_/¯
It needs some investigation, but I'm not sure I'll get to it until the next weekend.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

This clone only happens with rustdoc since it needs the resolver available at a later stage. It would be nicer if we could extract the information rustdoc needs instead.

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 20, 2019

Member

Can we split the resolver up so that we can pass a &CStore later to the parts of it that we need to keep around? (i.e. the local module tree)

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

We could make expansion query produce the final CStore structure and I said in my other comment. That won't help to get rid of the clone here though.

This comment has been minimized.

Copy link
@eddyb

eddyb Oct 20, 2019

Member

I'd prefer if rustdoc didn't cause that clone to happen but instead did its resolving in some other way.

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Oct 20, 2019

Member

It really feels like the new code is not worse, right? So we can definitely leave this for a future PR (filing an issue wouldn't hurt). I'd rather not block this PR on that cleanup.

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

This clone was there before this PR.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 20, 2019

cc @rust-lang/compiler @Mark-Simulacrum
Several people may be interested in this PR because it touches rustc_interface and removes locks.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 20, 2019

r? @Mark-Simulacrum or @Zoxc probably

Copy link
Member

Mark-Simulacrum left a comment

Overall looks great, excellent cleanup!

I agree with @eddyb that the cloning is unfortunate but ultimately seems 'fine' for now, we can clean it up in a future PR.

@@ -191,6 +191,8 @@ pub trait MetadataLoader {
-> Result<MetadataRef, String>;
}

pub type MetadataLoaderDyn = dyn MetadataLoader + Sync;

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Oct 20, 2019

Member

Do we need this to be Sync even in the non-parallel compiler case? Maybe we want sync::Sync here which is either the proper marker trait from core or the data structures one which is just auto for everything?

This comment has been minimized.

Copy link
@Zoxc

Zoxc Oct 20, 2019

Contributor

The metadata loaders have no fields, so it doesn't matter which Sync trait is used.

@@ -159,13 +156,12 @@ pub fn configure_and_expand(
}

pub struct ExpansionResult {

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Oct 20, 2019

Member

Is there a reason to re-wrap the resolve outputs here? I guess we do get Steal...

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Oct 21, 2019

Author Contributor

ExpansionResult is removed.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Oct 20, 2019

r=me unless you want to investigate my comments in this PR

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Oct 20, 2019

This PR looks good to me too.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 21, 2019

Let's run perf on this.
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 21, 2019

Awaiting bors try build completion

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 21, 2019

⌛️ Trying commit 2a329d2 with merge fb82105...

bors added a commit that referenced this pull request Oct 21, 2019
Turn crate store into a resolver output

Crate store (`CStore`) is a vector of data (`CrateMetadata`) associated with extern crates loaded during the current compilation session.

All crates are loaded in the resolver when resolving either paths pointing to extern prelude or `extern crate` items. (There are also a couple of crates like panic runtime that are loaded kind of like implicit `extern crate`s, but that also happens in resolve.)

The use of `CStore` from `rustc_plugin` (which is outside of the resolver) was unnecessary because legacy plugins are not added to the crate store and don't use `CrateNum`s.

So, `CStore` can be produced by the resolver instead of being kept in some really global data (`rustc_interface::Compiler`) like now.

As a result of crate store being more "local" we can now remove some locks and `Lrc`s.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2019

☀️ Try build successful - checks-azure
Build commit: fb82105 (fb82105b083af835f36cba113878eabd97ab8573)

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 22, 2019

Queued fb82105 with parent 10f12fe, future comparison URL.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 22, 2019

Let's run perf on this.

Not sure why I did that, there should be ~no changes for the non-parallel case.
@bors r=Mark-Simulacrum,Zoxc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2019

📌 Commit 2a329d2 has been approved by Mark-Simulacrum,Zoxc

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Oct 22, 2019

Finished benchmarking try commit fb82105, comparison URL.

bors added a commit that referenced this pull request Oct 23, 2019
Rollup of 9 pull requests

Successful merges:

 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65666 (Deprecated proc_macro doesn't trigger warning on build library)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

 - #65625 (Turn crate store into a resolver output)

r? @ghost
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 24, 2019

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

@petrochenkov petrochenkov force-pushed the petrochenkov:cstore branch from 2a329d2 to 94216ce Oct 24, 2019
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

petrochenkov commented Oct 24, 2019

@bors r=Mark-Simulacrum,Zoxc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 24, 2019

📌 Commit 94216ce has been approved by Mark-Simulacrum,Zoxc

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 24, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

Centril added a commit to Centril/rust that referenced this pull request Oct 24, 2019
…rum,Zoxc

Turn crate store into a resolver output

Crate store (`CStore`) is a vector of data (`CrateMetadata`) associated with extern crates loaded during the current compilation session.

All crates are loaded in the resolver when resolving either paths pointing to extern prelude or `extern crate` items. (There are also a couple of crates like panic runtime that are loaded kind of like implicit `extern crate`s, but that also happens in resolve.)

The use of `CStore` from `rustc_plugin` (which is outside of the resolver) was unnecessary because legacy plugins are not added to the crate store and don't use `CrateNum`s.

So, `CStore` can be produced by the resolver instead of being kept in some really global data (`rustc_interface::Compiler`) like now.

As a result of crate store being more "local" we can now remove some locks and `Lrc`s.
bors added a commit that referenced this pull request Oct 25, 2019
Rollup of 8 pull requests

Successful merges:

 - #65625 (Turn crate store into a resolver output)
 - #65627 (Forbid non-`structural_match` types in const generics)
 - #65710 (Update cargo)
 - #65729 (Update test cases for vxWorks)
 - #65746 (Tweak format string error to point at arguments always)
 - #65753 (Don't assert for different instance on impl trait alias)
 - #65755 (Avoid ICE when adjusting bad self ty)
 - #65766 (Update hashbrown to 0.6.2)

Failed merges:

r? @ghost
@bors bors merged commit 94216ce into rust-lang:master Oct 25, 2019
4 checks passed
4 checks passed
pr Build #20191024.65 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 25, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.