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

incr.comp.: Implement query result cache and use it to cache type checking tables. #46004

Merged
merged 21 commits into from Nov 17, 2017

Conversation

@michaelwoerister
Copy link
Contributor

commented Nov 15, 2017

This is a spike implementation of caching more than LLVM IR and object files when doing incremental compilation. At the moment, only the typeck_tables_of query is cached but MIR and borrow-check will follow shortly. The feature is activated by running with -Zincremental-queries in addition to -Zincremental, it is not yet active by default.

r? @nikomatsakis

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2017

Just found a bug in this: The new_type_index!() macro generates Encodable and Decodable implementations for DefIndex, so the specialized impls are never really called. This leads to some "vanilla" DefIndices being decoded without being rebased (e.g. in ty::UpVarId) and also without triggering the intended ICE.

Please do not merge until this is fixed.

let body = Body::decode(&mut decoder).unwrap();
body.diagnostics.into_iter().collect()

let prev_diagnostics: FxHashMap<_, _> = {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 15, 2017

Contributor

Why do we have this new variable here? It doesn't serve an obvious purpose. =)

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Nov 16, 2017

Author Contributor

It does in later commits where this block is used to limit the scope of the CacheDecoder.

where E: 'enc + ty_codec::TyEncoder
{
fn specialized_encode(&mut self, node_id: &NodeId) -> Result<(), Self::Error> {
let hir_id = self.definitions.node_to_hir_id(*node_id);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 15, 2017

Contributor

Is this basically copy-n-paste from the other encoders? (Not criticizing, just trying to understand.)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 15, 2017

Contributor

Answer: this impl, anyway, is not.

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Nov 15, 2017

Author Contributor

Yeah, I thought about writing a macro in ty::codec that would instantiate these implementations. Upfront, I wasn't sure though how many of them would end up just being copies.

}
}

impl<'a, 'tcx, 'x> SpecializedDecoder<&'tcx Substs<'tcx>> for CacheDecoder<'a, 'tcx, 'x> {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 15, 2017

Contributor

If we forget one of these impls, what happens? I think it panics? (These impls are copy-n-paste, I assume.)

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Nov 15, 2017

Author Contributor

Yeah, most of them should panic.

@@ -49,6 +49,7 @@ pub struct OnDiskCache<'sess> {

prev_cnums: Vec<(u32, String, CrateDisambiguator)>,
cnum_map: RefCell<Option<IndexVec<CrateNum, Option<CrateNum>>>>,
prev_def_path_tables: Vec<DefPathTable>,

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 15, 2017

Contributor

Does this signal a shift away from the "reverse lookup by hash" strategy?

This comment has been minimized.

Copy link
@michaelwoerister

michaelwoerister Nov 16, 2017

Author Contributor

No, the DefPathTables are stored for facilitating the "reverse lookup by hash". The implementation in this PR does the following:

  • Store a DefId unmodified in the cache.
  • When loading, we need to
    1. map the serialized DefId to the session-independent DefPathHash and then
    2. look up the new DefId by this DefPathHash.

In order to be able to do step (i), we need the old DefPathTable because it maps DefIndices to DefPathHashes.

But,... I'm not surprised that this caught your eye. There is an alternative approach that is simpler to implement and maybe more robust: Map DefIds and DefIndices to DefPathHashes at serialization time and thus get rid of the requirement to store the old DefPathTables. I'm not sure if this affects performance and space requirements in a good or in a bad way. But I'd like to implement it soonish and measure the impact.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

OK, I see.

@nikomatsakis
Copy link
Contributor

left a comment

Did a first pass. Left some drive-by questions. The main "red flag" of sorts was that there seemed to be a bit more code duplication than I expected, though mostly it's just impls calling into common helpers, so perhaps it's fine. Not sure how best to avoid it, perhaps using a macro -- or there might be some specialization tricks we could use with a helper trait.

michaelwoerister added some commits Nov 16, 2017

incr.comp.: Remove default serialization implementations for things i…
…n rustc::hir::def_id so that we get an ICE instead of silently doing the wrong thing.
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

OK, this should be up-to-date now. I went ahead and implemented the "map to DefPathHash at serialization-time" strategy because it made a few things easier around fixing stray DefIndices.

@michaelwoerister michaelwoerister referenced this pull request Nov 16, 2017
17 of 17 tasks complete
@nikomatsakis
Copy link
Contributor

left a comment

Lovely.



#[derive(Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct LocalDefId(DefIndex);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

I've wanted to add this type for so long. But can you please add a doc-comment explaining its role and why to use it, and also why it is bad to just use a raw DefIndex.

self.krate == LOCAL_CRATE
}

#[inline]

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

Nit: extra space.

prev_cnums: Vec<(u32, String, CrateDisambiguator)>,
cnum_map: RefCell<Option<IndexVec<CrateNum, Option<CrateNum>>>>,
prev_def_path_tables: Vec<DefPathTable>,

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

Nice.

ty_codec::decode_const(self)
}
}
implement_ty_decoder!( DecodeContext<'a, 'tcx> );

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 16, 2017

Contributor

Also nice.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

📌 Commit 4c4f7a3 has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

Er wait, I wanted something.

@bors r-

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

@michaelwoerister michaelwoerister force-pushed the michaelwoerister:cached-mir-wip-3 branch from 5f4d5d3 to 0a1f6dd Nov 16, 2017

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

@bors r=nikomatsakis

@bors

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2017

📌 Commit 0a1f6dd has been approved by nikomatsakis

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

@bors p=1 (we'd like to get this tested on rust-icci asap)

@bors

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

⌛️ Testing commit 0a1f6dd with merge 02eed2e...

bors added a commit that referenced this pull request Nov 17, 2017

Auto merge of #46004 - michaelwoerister:cached-mir-wip-3, r=nikomatsakis
incr.comp.: Implement query result cache and use it to cache type checking tables.

This is a spike implementation of caching more than LLVM IR and object files when doing incremental compilation. At the moment, only the `typeck_tables_of` query is cached but MIR and borrow-check will follow shortly. The feature is activated by running with `-Zincremental-queries` in addition to `-Zincremental`, it is not yet active by default.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

commented Nov 17, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 02eed2e to master...

@bors bors merged commit 0a1f6dd into rust-lang:master Nov 17, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2017

Note, this is not expected to show up in any benchmarks yet since it's behind the -Zincremental-queries flag.

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