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

michaelwoerister · 2017-11-15T14:49:12Z

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

…he same byte offsets as when saving.

…decoder.

….comp. cache.

…session-CrateNums in OnDiskCaches.

…sults can be found in the cached.

…ternCrates

…queries is specified.

michaelwoerister · 2017-11-15T16:52:11Z

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.

nikomatsakis · 2017-11-15T17:36:53Z

src/librustc/ty/maps/on_disk_cache.rs

-            let body = Body::decode(&mut decoder).unwrap();
-            body.diagnostics.into_iter().collect()
+
+            let prev_diagnostics: FxHashMap<_, _> = {


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

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

nikomatsakis · 2017-11-15T17:42:57Z

src/librustc/ty/maps/on_disk_cache.rs

+    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);


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

Answer: this impl, anyway, is not.

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.

nikomatsakis · 2017-11-15T17:44:43Z

src/librustc/ty/maps/on_disk_cache.rs

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


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

Yeah, most of them should panic.

nikomatsakis · 2017-11-15T18:01:04Z

src/librustc/ty/maps/on_disk_cache.rs


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


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

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

map the serialized DefId to the session-independent DefPathHash and then

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.

nikomatsakis

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.

…efId is from the local crate.

…se during deserialization.

…g things into a macro.

…n rustc::hir::def_id so that we get an ICE instead of silently doing the wrong thing.

michaelwoerister · 2017-11-16T16:18:03Z

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.

nikomatsakis

Lovely.

nikomatsakis · 2017-11-16T16:37:52Z

src/librustc/hir/def_id.rs

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


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.

nikomatsakis · 2017-11-16T16:38:07Z

src/librustc/hir/def_id.rs

        self.krate == LOCAL_CRATE
    }
+
+     #[inline]


Nit: extra space.

nikomatsakis · 2017-11-16T16:54:05Z

src/librustc/ty/maps/on_disk_cache.rs

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


nikomatsakis · 2017-11-16T16:54:39Z

src/librustc_metadata/decoder.rs

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


nikomatsakis · 2017-11-16T16:55:19Z

@bors r+

bors · 2017-11-16T16:55:20Z

📌 Commit 4c4f7a3 has been approved by nikomatsakis

nikomatsakis · 2017-11-16T16:55:27Z

Er wait, I wanted something.

@bors r-

nikomatsakis · 2017-11-16T16:55:34Z

r=me with comment on LocalDefId explaining its role =)

michaelwoerister · 2017-11-16T17:10:10Z

@bors r=nikomatsakis

bors · 2017-11-16T17:10:10Z

📌 Commit 0a1f6dd has been approved by nikomatsakis

michaelwoerister · 2017-11-17T10:10:33Z

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

bors · 2017-11-17T10:12:27Z

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

@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 · 2017-11-17T12:57:01Z

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

michaelwoerister · 2017-11-17T13:02:59Z

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

michaelwoerister added 15 commits November 13, 2017 15:13

incr.comp.: Include header when loading cache files in order to get t…

8cbc022

…he same byte offsets as when saving.

incr.comp.: Add position() method to TyEncoder.

c08e03a

incr.comp.: Implement UseSpecializedXXcodable for DefIndex and DefId.

bc96d9d

incr.comp.: Properly use ty::codec::decode_cnum() in rustc_metadata::…

9ac1026

…decoder.

incr.comp.: Add CacheEncoder for encoding query results into the incr…

3bd333c

….comp. cache.

incr.comp.: Implement TyDecoder for on_disk_cache::CacheDecoder.

15db165

incr.comp.: Allow for mapping from prev-session-CrateNums to current-…

bedb44c

…session-CrateNums in OnDiskCaches.

incr.comp.: Encode DefPathTables for reconstructing DefIds.

de0317e

incr.comp.: Do some verification on data decoded from incr. comp. cache.

2087d5e

incr.comp.: Store the query result index which records where query re…

4bfab89

…sults can be found in the cached.

incr.comp.: Add 'tcx to QueryDescription.

0b14383

incr.comp.: Cache TypeckTables and add -Zincremental-queries flag.

2c1aedd

incr.comp.: Refactor query cache serialization to be more re-usable.

279b6df

incr.comp.: Add missing [input] annotation for DepNode::MaybeUnusedEx…

13582c6

…ternCrates

incr.comp.: Only save and load query result cache when -Zincremental-…

2f50e62

…queries is specified.

rust-highfive assigned nikomatsakis Nov 15, 2017

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2017

nikomatsakis reviewed Nov 15, 2017

View reviewed changes

michaelwoerister added 4 commits November 16, 2017 14:04

Introduce LocalDefId which provides a type-level guarantee that the D…

24e54dd

…efId is from the local crate.

incr.comp.: Encode DefIds as DefPathHashes instead of recomputing tho…

2f44ef2

…se during deserialization.

incr.comp.: Remove some code duplication around TyDecoder by factorin…

723028f

…g things into a macro.

incr.comp.: Remove default serialization implementations for things i…

cb1ff24

…n rustc::hir::def_id so that we get an ICE instead of silently doing the wrong thing.

Fix some tidy errors in ty::codec.

4c4f7a3

michaelwoerister mentioned this pull request Nov 16, 2017

Tracking issue for "Red/Green" Dependency Tracking #42293

Closed

17 tasks

nikomatsakis approved these changes Nov 16, 2017

View reviewed changes

Add doc comment for LocalDefId.

0a1f6dd

michaelwoerister force-pushed the cached-mir-wip-3 branch from 5f4d5d3 to 0a1f6dd Compare November 16, 2017 17:09

kennytm 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 Nov 16, 2017

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

bors mentioned this pull request Nov 17, 2017

Make GlobalCtxt implement Sync #45912

Closed

This was referenced Nov 17, 2017

incr.comp.: Make sure cargo check is compatible with incremental compilation. #46058

Open

Enable -Zincremental-queries nikomatsakis/cargo-incremental#53

Merged



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

Uh oh!

Conversation

michaelwoerister commented Nov 15, 2017

Uh oh!

michaelwoerister commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

michaelwoerister commented Nov 16, 2017

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Nov 16, 2017

Uh oh!

bors commented Nov 16, 2017

Uh oh!

nikomatsakis commented Nov 16, 2017

Uh oh!

nikomatsakis commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelwoerister commented Nov 16, 2017

Uh oh!

bors commented Nov 16, 2017

Uh oh!

michaelwoerister commented Nov 17, 2017

Uh oh!

bors commented Nov 17, 2017

Uh oh!

bors commented Nov 17, 2017

Uh oh!

michaelwoerister commented Nov 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nikomatsakis commented Nov 16, 2017 •

edited

Loading