Refactor trans some more to pave way for incremental compilation #41355

nikomatsakis · 2017-04-18T01:24:00Z

Various refactorings paving the way for the newer approach to incremental compilation (And, in particular, to "query-ifying" trans). My partial goal is to remove SharedCrateContext; this PR gets about as far as I can easily get before starting to really want the red/green algorithm.

r? @eddyb
cc @michaelwoerister

eddyb

LGTM modulo nits

eddyb · 2017-04-18T05:09:19Z

src/librustc/traits/trans/mod.rs

+use ty::{self, Ty};
+
+/// Specializes caches used in trans -- in particular, they assume all
+/// types are fully monomorphized and that free regions can be erased.


The monomorphic condition was fixed a while back I think, at least in terms of specialization - and I assume the regions wouldn't matter after we get lazy normalization? Still, can't these caches be chosen dynamically, if all conditions are met?

Yeah, probably it's true that these caches aren't so important, though I'm not sure I'm inclined to change anything here in this particular PR. (I did however intend to move a lot of the routines that manipulate said caches out of trans and into this file, and I forgot to do so.)

I more or less want to kill all "trans" terminology and just have backends that use Reveal::All and do the lifetime erasing themselves, in the long term.

I more or less want to kill all "trans" terminology and just have backends that use Reveal::All and do the lifetime erasing themselves, in the long term.

I basically agree, but I'm not sure I want to do that now. In particular, I want to remove these caches eventually and just sort of roll this into a more general caching scheme. Therefore, I tried to just shuffle things around for now rather than worry too much about the details, so that we can address them in a more uniform fashion.

eddyb · 2017-04-18T05:10:44Z

src/librustc_metadata/cstore_impl.rs

@@ -111,6 +111,7 @@ provide! { <'tcx> tcx, def_id, cdata
    closure_kind => { cdata.closure_kind(def_id.index) }
    closure_type => { cdata.closure_ty(def_id.index, tcx) }
    inherent_impls => { Rc::new(cdata.get_inherent_implementations_for_type(def_id.index)) }
+    is_foreign_item => { cdata.is_foreign_item(def_id.index) }


Is there no similar method to remove on the CrateStore trait?

Hmm, there is such a method, it gets called quite a few times. I can work on refactoring those away.

eddyb · 2017-04-18T05:15:52Z

src/librustc_trans/context.rs

-
-    translation_items: RefCell<FxHashSet<TransItem<'tcx>>>,
-    trait_cache: RefCell<DepTrackingMap<TraitSelectionCache<'tcx>>>,
-    project_cache: RefCell<DepTrackingMap<ProjectionCache<'tcx>>>,
 }


This reminds me, the design of SharedCrateContext was a mistake IMO - its fields should be public and CrateContext should Deref to it so we get e.g. ccx.tcx and also no duplication of methods.

I'm basically aiming to kill it.

eddyb · 2017-04-18T05:18:57Z

src/librustc_trans/symbol_cache.rs

+
+pub struct SymbolCache<'a, 'tcx: 'a> {
+    tcx: TyCtxt<'a, 'tcx, 'tcx>,
+    index: RefCell<FxHashMap<TransItem<'tcx>, Rc<String>>>,


Could this use the interned Symbol instead?

i.e., I should do Symbol::intern() for each string? Seems ok, sure.

This works out to be pretty annoying in practice. Every caller of get() winds up calling as_str() on the result almost immediately.

Just add a convenience method?

What's the point of interning this?

I guess it reduces memory usage when multiple CGUs have the same serialization?

It's just that we already have something akin to Rc<String> and it's Symbol.

bors · 2017-04-18T06:46:46Z

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

This may seem like overkill, but it's exactly what we want/need for incremental compilation I think. In particular, while generating code for some codegen unit X, we can wind up querying about any number of external items, and we only want to be forced to rebuild X is some of those changed from a foreign item to otherwise. Factoring this into a query means we would re-run only if some `false` became `true` (or vice versa).

If we are going to hash `SharedCrateContext`, we don't want a list of things that pertain to **every CGU** in there.

The symbol map is not good for incremental: it has inputs from every fn in existence, and it will change if anything changes. One could imagine cheating with the symbol-map and exempting it from the usual dependency tracking, since the results are fully deterministic. Instead, I opted to just add a per-CGU cache, on the premise that recomputing some symbol names is not going to be so very expensive.

The more we can things dependent on just tcx, the easier it will be to make queries etc later on.

Once it is computed, no need to deep clone the set.

Arguably these could become custom queries, but I chose not to do that because the relationship of queries and trait system is not yet fleshed out enough. For now it seems fine to have them be `DepTrackingMap` using the memoize pattern.

This makes these routines more readily available for other bits of code. It also will help when refactoring.

nikomatsakis · 2017-04-22T09:47:51Z

@bors r=eddyb

bors · 2017-04-22T09:47:52Z

📌 Commit 6d86f81 has been approved by eddyb

@eddyb

…ns-2, r=eddyb Refactor trans some more to pave way for incremental compilation Various refactorings paving the way for the newer approach to incremental compilation (And, in particular, to "query-ifying" trans). My partial goal is to remove `SharedCrateContext`; this PR gets about as far as I can easily get before starting to really want the red/green algorithm. r? @eddyb cc @michaelwoerister

Rollup of 3 pull requests - Successful merges: #41077, #41355, #41450 - Failed merges:

bors · 2017-04-22T16:46:55Z

⌛ Testing commit 6d86f81 with merge 9cc77d7...

eddyb · 2017-04-22T17:35:39Z

@bors r- retry

rust-highfive assigned eddyb Apr 18, 2017

eddyb approved these changes Apr 18, 2017

View reviewed changes

aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2017

nikomatsakis added 9 commits April 21, 2017 17:26

remove translation_items from SharedCrateContext

4c31750

If we are going to hash `SharedCrateContext`, we don't want a list of things that pertain to **every CGU** in there.

just take tcx where we can

c446cb0

The more we can things dependent on just tcx, the easier it will be to make queries etc later on.

make reachable_set ref-counted

264f237

Once it is computed, no need to deep clone the set.

sort provide

e48a759

move the trans trait caches into tcx

f4c183b

Arguably these could become custom queries, but I chose not to do that because the relationship of queries and trait system is not yet fleshed out enough. For now it seems fine to have them be `DepTrackingMap` using the memoize pattern.

use Symbol/InternedString in the cache

8552745

move the uses of the trans caches into rustc::traits

6d86f81

This makes these routines more readily available for other bits of code. It also will help when refactoring.

nikomatsakis force-pushed the incr-comp-refactor-trans-2 branch from e3c3b80 to 6d86f81 Compare April 22, 2017 01:03

frewsxcv mentioned this pull request Apr 22, 2017

Rollup of 3 pull requests #41464

Merged

bors added a commit that referenced this pull request Apr 22, 2017

Auto merge of #41464 - frewsxcv:rollup, r=frewsxcv

9cc77d7

Rollup of 3 pull requests - Successful merges: #41077, #41355, #41450 - Failed merges:

bors merged commit 6d86f81 into rust-lang:master Apr 22, 2017

dwrensha mentioned this pull request Aug 26, 2017

use rustc version of fulfill_obligation now that we can rust-lang/miri#314

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor trans some more to pave way for incremental compilation #41355

Refactor trans some more to pave way for incremental compilation #41355

nikomatsakis commented Apr 18, 2017

eddyb left a comment

eddyb Apr 18, 2017

nikomatsakis Apr 18, 2017

eddyb Apr 18, 2017

nikomatsakis Apr 21, 2017

eddyb Apr 18, 2017

nikomatsakis Apr 18, 2017

eddyb Apr 18, 2017

nikomatsakis Apr 18, 2017

eddyb Apr 18, 2017

nikomatsakis Apr 18, 2017

nikomatsakis Apr 21, 2017

eddyb Apr 21, 2017

nikomatsakis Apr 21, 2017

nikomatsakis Apr 21, 2017

eddyb Apr 21, 2017

bors commented Apr 18, 2017

nikomatsakis commented Apr 22, 2017

bors commented Apr 22, 2017

bors commented Apr 22, 2017

eddyb commented Apr 22, 2017

Refactor trans some more to pave way for incremental compilation #41355

Refactor trans some more to pave way for incremental compilation #41355

Conversation

nikomatsakis commented Apr 18, 2017

eddyb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 18, 2017

nikomatsakis commented Apr 22, 2017

bors commented Apr 22, 2017

bors commented Apr 22, 2017

eddyb commented Apr 22, 2017