rustc: Remove HirId from queries #44435

alexcrichton · 2017-09-08T21:03:12Z

This'll allow us to reconstruct query parameters purely from the DepNode
they're associated with.

Closes #44414

rust-highfive · 2017-09-08T21:03:15Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

alexcrichton · 2017-09-08T21:04:04Z

r? @michaelwoerister

eddyb · 2017-09-09T08:38:11Z

src/librustc/ty/context.rs

@@ -836,11 +836,11 @@ pub struct GlobalCtxt<'tcx> {
    // Records the free variables refrenced by every closure
    // expression. Do not track deps for this, just recompute it from
    // scratch every time.
-    freevars: FxHashMap<HirId, Rc<Vec<hir::Freevar>>>,
+    freevars: FxHashMap<DefId, Rc<Vec<hir::Freevar>>>,


I believe there's DefIdMap and DefIdSet aliases which could be used for these changes.

eddyb · 2017-09-09T08:39:28Z

src/librustc/ty/mod.rs

-        let hir_id = self.hir.node_to_hir_id(fid);
-        match self.freevars(hir_id) {
+        let def_id = self.hir.local_def_id(fid);
+        match self.freevars(def_id) {


So with_freevars used to be a thing because of a RefCell. Can it be removed in favor of tcx.freevars(def_id)? All callers should, ironically, already have a closure DefId around.

kennytm · 2017-09-09T09:05:19Z

Could not compile core

[00:17:57] error: internal compiler error: unexpected panic
[00:17:57] 
[00:17:57] note: the compiler unexpectedly panicked. this is a bug.
[00:17:57] 
[00:17:57] note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
[00:17:57] 
[00:17:57] note: rustc 1.22.0-dev running on x86_64-unknown-linux-gnu
[00:17:57] 
[00:17:57] thread 'rustc' panicked at '/checkout/src/librustc/hir/map/mod.rs:330: local_def_id: no entry for `291005`, which has a map of `Some(EntryLifetime(NodeId(192109), DepNodeIndex { index: 4294967295 }, lifetime(291005: )))`', /checkout/src/librustc/session/mod.rs:889:25
[00:17:57] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:17:57] 
[00:17:57] error: Could not compile `core`.

bors · 2017-09-09T09:18:35Z

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

michaelwoerister · 2017-09-09T15:48:08Z

Thanks, @alexcrichton!

Unfortunately it won't be as easy as just replacing HirId with DefId here because not every HirId has a corresponding DefId. For example, HIR expressions don't have a DefId, so moving in_scope_traits to DefId also involves storing things in a two-level map, so that it can look something like this:

impl TyCtxt {
    fn in_scope_traits_of_expr(self, hir_id: HirId) -> Option<Rc<Vec<TraitCandidate>>> {
        // Get the map for the item containing the HirId. This is a query:
        let map_for_containing_item = self.in_scope_traits_of_item(hir_id.owner);
        // From that item-local map get actual return value
        map_for_containing_item.get(hir_id.local_id).cloned()
    }
}

alexcrichton · 2017-09-09T20:59:51Z

Heh yeah the panics figured that out pretty quickly here! I'll work on making these sub-maps.

alexcrichton · 2017-09-09T21:46:18Z

Alright I believe the necessary maps have been transitioned, re-r? @michaelwoerister

eddyb · 2017-09-10T03:07:41Z

src/librustc/dep_graph/dep_node.rs

@@ -570,8 +570,8 @@ define_dep_nodes!( <'tcx>
    [] UsedCrateSource(CrateNum),
    [] PostorderCnums,

-    [] Freevars(HirId),
-    [] MaybeUnusedTraitImport(HirId),
+    [] Freevars(DefIndex),


Why not use DefId here? Closures have DefIds.

Indeed! Got a bit overzealous with DefIndex

michaelwoerister

Looks good to me except for that one performance concern.

michaelwoerister · 2017-09-11T09:21:11Z

src/librustc_metadata/cstore_impl.rs

-            let id = tcx.hir.definitions().find_node_for_hir_id(id);
+            assert_eq!(id.krate, LOCAL_CRATE);
+            let hir_id = tcx.hir.definitions().def_index_to_hir_id(id.index);
+            let id = tcx.hir.definitions().find_node_for_hir_id(hir_id);


Whoops, I should better have listened to @arielb1 and marked find_node_for_hir_id as super-slow somehow. It does a linear search over all NodeIds 😰

But it seems that what you're doing here is equivalent to tcx.hir.as_local_node_id(id).unwrap()?

Why does find_node_for_hir_id exist? I've introduced hir_to_node_id. (EDIT: I generated a reverse HashMap for that purpose)

Oh, I see. It does exist because I wanted to avoid building that reverse mapping because it was only needed in error reporting. It's probably cheap to build though, especially now that locals don't have DefIds anymore.

alexcrichton · 2017-09-11T14:07:03Z

@bors: r=michaelwoerister

bors · 2017-09-11T14:07:04Z

📌 Commit 772ca85 has been approved by michaelwoerister

This'll allow us to reconstruct query parameters purely from the `DepNode` they're associated with. Some queries could move straight to `HirId` but others that don't always have a correspondance between `HirId` and `DefId` moved to two-level maps where the query operates over a `DefIndex`, returning a map, which is then keyed off `ItemLocalId`. Closes rust-lang#44414

alexcrichton · 2017-09-11T14:54:03Z

@bors: r=michaelwoerister

bors · 2017-09-11T14:54:04Z

📌 Commit caaf365 has been approved by michaelwoerister

bors · 2017-09-11T15:35:41Z

⌛ Testing commit caaf365 with merge efa3ec6...

rustc: Remove HirId from queries This'll allow us to reconstruct query parameters purely from the `DepNode` they're associated with. Closes #44414

bors · 2017-09-11T18:16:13Z

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

rust-highfive assigned arielb1 Sep 8, 2017

alexcrichton mentioned this pull request Sep 8, 2017

incr.comp.: Refactor in_scope_traits query to take DefId instead of HirId as query-key #44414

Closed

rust-highfive assigned michaelwoerister and unassigned arielb1 Sep 8, 2017

eddyb reviewed Sep 9, 2017

View reviewed changes

alexcrichton force-pushed the in-scope branch 2 times, most recently from 14c68ee to 7e49951 Compare September 9, 2017 21:45

eddyb reviewed Sep 10, 2017

View reviewed changes

alexcrichton force-pushed the in-scope branch from 7e49951 to 7666583 Compare September 10, 2017 16:19

michaelwoerister reviewed Sep 11, 2017

View reviewed changes

alexcrichton force-pushed the in-scope branch from 7666583 to 772ca85 Compare September 11, 2017 14:06

carols10cents added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 11, 2017

alexcrichton force-pushed the in-scope branch from 772ca85 to caaf365 Compare September 11, 2017 14:53

bors added a commit that referenced this pull request Sep 11, 2017

Auto merge of #44435 - alexcrichton:in-scope, r=michaelwoerister

efa3ec6

rustc: Remove HirId from queries This'll allow us to reconstruct query parameters purely from the `DepNode` they're associated with. Closes #44414

bors merged commit caaf365 into rust-lang:master Sep 11, 2017

bors mentioned this pull request Sep 11, 2017

rustc: Make CrateStore private to TyCtxt #44420

Merged

alexcrichton deleted the in-scope branch September 11, 2017 19:13

oli-obk mentioned this pull request Sep 12, 2017

Clippy v0.0.158 fails to build on nightly (eba374fb2 2017-09-11) rust-lang/rust-clippy#2044

Closed

nikomatsakis mentioned this pull request Sep 14, 2017

10% regression in the issue-43572-unused-uses test case #44575

Closed

alexcrichton mentioned this pull request Sep 15, 2017

Up to 8% RSS regression because of #44142 #44576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: Remove HirId from queries #44435

rustc: Remove HirId from queries #44435

alexcrichton commented Sep 8, 2017

rust-highfive commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

eddyb Sep 9, 2017

eddyb Sep 9, 2017

kennytm commented Sep 9, 2017

bors commented Sep 9, 2017

michaelwoerister commented Sep 9, 2017

alexcrichton commented Sep 9, 2017

alexcrichton commented Sep 9, 2017

eddyb Sep 10, 2017

alexcrichton Sep 10, 2017

michaelwoerister left a comment

michaelwoerister Sep 11, 2017

eddyb Sep 11, 2017 •

edited

Loading

michaelwoerister Sep 11, 2017

alexcrichton commented Sep 11, 2017

bors commented Sep 11, 2017

alexcrichton commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

rustc: Remove HirId from queries #44435

rustc: Remove HirId from queries #44435

Conversation

alexcrichton commented Sep 8, 2017

rust-highfive commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

eddyb Sep 9, 2017

Choose a reason for hiding this comment

eddyb Sep 9, 2017

Choose a reason for hiding this comment

kennytm commented Sep 9, 2017

bors commented Sep 9, 2017

michaelwoerister commented Sep 9, 2017

alexcrichton commented Sep 9, 2017

alexcrichton commented Sep 9, 2017

eddyb Sep 10, 2017

Choose a reason for hiding this comment

alexcrichton Sep 10, 2017

Choose a reason for hiding this comment

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Sep 11, 2017

Choose a reason for hiding this comment

eddyb Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

michaelwoerister Sep 11, 2017

Choose a reason for hiding this comment

alexcrichton commented Sep 11, 2017

bors commented Sep 11, 2017

alexcrichton commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

bors commented Sep 11, 2017

eddyb Sep 11, 2017 •

edited

Loading