Migrate a slew of metadata methods to queries #44142

alexcrichton · 2017-08-29T01:34:40Z

This PR intends to make more progress on #41417, knocking off some low-hanging fruit.

Closes #44190
cc #44137

alexcrichton · 2017-08-29T01:34:49Z

r? @nikomatsakis

rust-highfive · 2017-08-29T01:34:50Z

r? @pnkfelix

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

kennytm · 2017-08-29T06:59:31Z

CI timed out after 3 hours, with run-pass/rustc-rust-log.rs failed with signal 6 i.e. SIGABRT. The error log is >4 MB consisting of DEBUG logs. It is legit last time this happens (#43028 (comment)).

[00:46:45] ---- [run-pass] run-pass/rustc-rust-log.rs stdout ----
[00:46:45] 	
[00:46:45] error: compilation failed!
[00:46:45] status: signal: 6
[00:46:45] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/run-pass/rustc-rust-log.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/rustc-rust-log.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-pass/rustc-rust-log.stage2-x86_64-unknown-linux-gnu.run-pass.libaux"
[00:46:45] stdout:
[00:46:45] ------------------------------------------
[00:46:45] 
[00:46:45] ------------------------------------------
[00:46:45] stderr:
[00:46:45] ------------------------------------------

eddyb · 2017-08-29T11:21:46Z

src/librustc/dep_graph/dep_node.rs

+    [] DeriveRegistrarFn(DefId),
+    [] ForeignCrateDisambiguator(DefId),
+    [] CrateHash(DefId),
+    [] ForeignOriginalCrateName(DefId),


@nikomatsakis @michaelwoerister Is there not a better way to deal with this?

The problem being the duplication? I'd like to combine this macro with the query macro, I'm not sure if something is blocking that.

eddyb · 2017-08-29T11:22:27Z

src/librustc_metadata/cstore_impl.rs

@@ -140,7 +138,27 @@ provide! { <'tcx> tcx, def_id, cdata,
    is_panic_runtime => { cdata.is_panic_runtime(&tcx.dep_graph) }
    is_compiler_builtins => { cdata.is_compiler_builtins(&tcx.dep_graph) }
    has_global_allocator => { cdata.has_global_allocator(&tcx.dep_graph) }
+    is_sanitizer_runtime => { cdata.is_sanitizer_runtime(&tcx.dep_graph) }
+    is_profiler_runtime => { cdata.is_profiler_runtime(&tcx.dep_graph) }
+    panic_strategy => { cdata.panic_strategy(&tcx.dep_graph) }


Aren't a bunch of these just reading crate-level attributes? Why can't we do that directly through tcx.get_attrs?

We could, but it would mean that if we allow "free" access, then any change to crate attributes will require recompiling everything. Not sure if that's a problem.

Hmm but that could be solved by putting a Symbol (for the name of the attribute) in the query, which is what you want most of the time anyway.

Not all of these are crate attributes as well, some of them, like the panic strategy, can be affected by command line flags as well. The others, sure, but perhaps that's a refactoring for another time?

Hmm if there aren't many of them then this PR is probably fine.

nikomatsakis

This all seems ok to me. Regarding @eddyb's comments, my take is that:

we could (and should) revisit the question of how to autogenerate dep-node variants and query variants (I'd prefer if you did not have to duplicate), but it seems orthogonal from this PR;
we could consider accessing the crate attributes directly, but I don't see a strong reason to do so.

In particular, accessing crate attributes directly would imply that we would have more coarse-grained incremental. This is not in and of itself a problem, as I don't know that modifying crate attributes was ever intended to be a "supported" incremental use case, but I also don't see much reason to actively make it not work, now that @alexcrichton already did the work.

If the concern is that we are making too many hashmaps or something like that, I think I'd prefer to keep the queries, but add some flags to make a query "direct" or something, in which it is not cached (and presumably then also doesn't help much with incremental). That way we can control this trade-off with minimal effort. (Or does that feel like overkill?)

eddyb · 2017-08-29T14:41:34Z

@nikomatsakis The thing is that there's a handful of methods that were never required, all they do is they read the crate attributes and IMO having the extra method in between muddles that.

alexcrichton · 2017-08-29T18:00:03Z

I tracked down some incremental test failures with the help of @nikomatsakis and the conclusion is that this PR will regress some current incremental tests due to new dependency nodes getting introduced. We decided, though, that the regression is ok for now. The red/green system will fix the regressions and so for now I'll just tag them with a FIXME

alexcrichton · 2017-08-29T22:52:08Z

Ok I've updated queries to work with CrateNum instead of DefId and ignored the failing tests.

re-r? @nikomatsakis

nikomatsakis · 2017-08-30T15:02:14Z

@eddyb

The thing is that there's a handful of methods that were never required, all they do is they read the crate attributes and IMO having the extra method in between muddles that.

It appears I never responded to this. I know I started one... anyway, I actually think the queries do serve (potentially) a purpose. First off, they are clearly (to me) better documentation of the intent than "open-coding" some logic that trawls through the attributes on the crate. That is, I'd prefer that most of the compiler doesn't directly deal in attributes, but uses a helper method to encapsulate that logic (that doesn't necessarily have to be a query).

Second, in terms of incremental logic, having queries does serve a real purpose since it allows us to control change propagation. This is what I was trying to say: having a query means that, once red-green is working, if the user changes the crate attributes, we will be able to avoid recompiling everything (since many things consult these crate attributes, e.g. symbol creation), unless they happen to have changed the result of one of those queries.

Now, it may be that we could just have helper methods instead of the queries: I wouldn't be opposed to that, but it will result in coarser grained incremental support.

alexcrichton · 2017-08-30T18:55:13Z

I've pushed up a fix for the broken test and one more query translated

bors · 2017-08-31T03:57:06Z

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

alexcrichton · 2017-08-31T19:51:52Z

src/librustc_metadata/cstore_impl.rs

+        },
+        extern_mod_stmt_cnum: |tcx, id| {
+            let id = tcx.hir.definitions().find_node_for_hir_id(id);
+            tcx.sess.cstore.extern_mod_stmt_cnum_untracked(id)


This method and postorder_cnums below are something I wanted to ask about here. So what's happening here is that in this query it's reaching into the cstore via the CrateStore trait into various untracked methods to actually perform the computation.

Is that ok? Does that mean that these aren't tracked properly? Should the dep nodes be manually created here? Just wanted to make sure to flag this!

I think what should happen is that we should treat these as "input" queries -- and hence always re-execute them. Really, I think that robably all the metadata-poking queries should be this way, and we should just lean on red-green to let us only invalidate things that are truly invalidated by changing metadata.

(Cc @michaelwoerister)

cc #44234 as well, where I believe we're discussing this basically

alexcrichton · 2017-08-31T19:54:04Z

src/librustc/ty/context.rs

+        // Once red/green incremental compilation lands we should be able to
+        // remove this because while the crate changes often the lint level map
+        // will change rarely.
+        self.dep_graph.with_ignore(|| {


I wanted to make sure I called this out as well. This with_ignore was necessary to not instantly break all of the incremental tests!

I imagine though that with red/green this'll always generate the same value, so the node's always green even though it needs to be recomputed?

I think that's the idea, yeah

eddyb · 2017-08-31T19:56:22Z

src/librustc_metadata/cstore_impl.rs

+    item_body => {
+        if let Some(cached) = tcx.hir.get_inlined_body_untracked(def_id) {
+            return cached;
+        }


You can remove this if and the get_inlined_body_untracked method, as the result will be cached anyway.

eddyb · 2017-08-31T19:57:12Z

src/librustc/ty/maps.rs

@@ -1313,6 +1343,14 @@ define_maps! { <'tcx>
    [] get_lang_items: get_lang_items_node(CrateNum) -> Rc<LanguageItems>,
    [] defined_lang_items: DefinedLangItems(CrateNum) -> Rc<Vec<(DefIndex, usize)>>,
    [] missing_lang_items: MissingLangItems(CrateNum) -> Rc<Vec<LangItem>>,
+    [] item_body: ItemBody(DefId) -> &'tcx hir::Body,


Can this be named extern_const_body?

bors · 2017-09-01T07:31:39Z

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

bors · 2017-09-07T20:13:01Z

💔 Test failed - status-travis

kennytm · 2017-09-07T20:36:07Z

Same error as #43742 (comment)? Something's wrong with the sanitizers.

@bors retry

bors · 2017-09-08T09:48:05Z

⌛ Testing commit fd0aa64 with merge dead08c...

Migrate a slew of metadata methods to queries This PR intends to make more progress on #41417, knocking off some low-hanging fruit. Closes #44190 cc #44137

alexcrichton · 2017-09-08T14:41:39Z

Everything passed except two builders on OSX which are timing out due to travis issues. I'm going to merge and deal with fallout if there's any (which seems highly unlikely)

alexcrichton · 2017-09-08T14:42:54Z

Travis and AppVeyor

This commit alters the `query` function in the dep graph module to preallocate memory using `with_capacity` instead of relying on automatic growth. Discovered in rust-lang#44576 it was found that for the syntex_syntax clean incremental benchmark the peak memory usage was found when the dep graph was being saved, particularly the `DepGraphQuery` data structure itself. PRs like rust-lang#44142 which add more queries end up just making this much larger! I didn't see an immediately obvious way to reduce the size of the `DepGraphQuery` object, but it turns out that `with_capacity` helps quite a bit! Locally 831 MB was used [before] this commit, and 770 MB is in use at the peak of the compiler [after] this commit. That's a nice 7.5% improvement! This won't quite make up for the losses in rust-lang#44142 but I figured it's a good start. [before]: https://gist.github.com/alexcrichton/2d2b9c7a65503761925c5a0bcfeb0d1e [before]: https://gist.github.com/alexcrichton/6da51f2a6184bfb81694cc44f06deb5b

…elwoerister rustc: Preallocate when building the dep graph This commit alters the `query` function in the dep graph module to preallocate memory using `with_capacity` instead of relying on automatic growth. Discovered in rust-lang#44576 it was found that for the syntex_syntax clean incremental benchmark the peak memory usage was found when the dep graph was being saved, particularly the `DepGraphQuery` data structure itself. PRs like rust-lang#44142 which add more queries end up just making this much larger! I didn't see an immediately obvious way to reduce the size of the `DepGraphQuery` object, but it turns out that `with_capacity` helps quite a bit! Locally 831 MB was used [before] this commit, and 770 MB is in use at the peak of the compiler [after] this commit. That's a nice 7.5% improvement! This won't quite make up for the losses in rust-lang#44142 but I figured it's a good start. [before]: https://gist.github.com/alexcrichton/2d2b9c7a65503761925c5a0bcfeb0d1e [before]: https://gist.github.com/alexcrichton/6da51f2a6184bfb81694cc44f06deb5b

rust-highfive assigned pnkfelix Aug 29, 2017

rust-highfive assigned nikomatsakis and unassigned pnkfelix Aug 29, 2017

eddyb reviewed Aug 29, 2017

View reviewed changes

nikomatsakis approved these changes Aug 29, 2017

View reviewed changes

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

nikomatsakis mentioned this pull request Aug 29, 2017

incr.comp.: Crate Metadata not corresponding to a DefId is untracked. #41417

Closed

alexcrichton force-pushed the dllimport-query branch from 40831eb to 30ea918 Compare August 29, 2017 21:59

alexcrichton force-pushed the dllimport-query branch from 94fd925 to a520552 Compare August 30, 2017 19:04

alexcrichton force-pushed the dllimport-query branch from 668d624 to 7279bec Compare August 31, 2017 14:50

nikomatsakis mentioned this pull request Aug 31, 2017

incremental: refactor out non-query data from tcx #44137

Closed

13 tasks

alexcrichton force-pushed the dllimport-query branch from 7279bec to 9892072 Compare August 31, 2017 18:07

alexcrichton commented Aug 31, 2017

View reviewed changes

eddyb reviewed Aug 31, 2017

View reviewed changes

alexcrichton force-pushed the dllimport-query branch from 5de8ffc to 1485ed4 Compare September 1, 2017 00:38

alexcrichton mentioned this pull request Sep 1, 2017

incremental: Initial "always dirty" queries #44234

Closed

alexcrichton force-pushed the dllimport-query branch from 1485ed4 to d7d5c59 Compare September 1, 2017 16:28

nikomatsakis 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 Sep 8, 2017

alexcrichton merged commit fd0aa64 into rust-lang:master Sep 8, 2017

bors mentioned this pull request Sep 8, 2017

Use NodeId/HirId instead of DefId for local variables. #44316

Merged

alexcrichton deleted the dllimport-query branch September 8, 2017 14:52

oli-obk mentioned this pull request Sep 8, 2017

incoming breakage rust-lang/rust-clippy#2032

Closed

kennytm mentioned this pull request Sep 10, 2017

Implement RFC 1861: Extern types #44295

Merged

arielb1 mentioned this pull request Sep 14, 2017

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

Closed

alexcrichton mentioned this pull request Sep 15, 2017

rustc: Preallocate when building the dep graph #44586

Merged

kennytm mentioned this pull request Oct 31, 2017

typeck: suggest use of match_default_bindings feature #45409

Merged

shepmaster mentioned this pull request Jul 4, 2018

Unknown feature flags should trigger a warning or error #52053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate a slew of metadata methods to queries #44142

Migrate a slew of metadata methods to queries #44142

alexcrichton commented Aug 29, 2017 •

edited

Loading

alexcrichton commented Aug 29, 2017

rust-highfive commented Aug 29, 2017

kennytm commented Aug 29, 2017 •

edited

Loading

eddyb Aug 29, 2017

nikomatsakis Aug 29, 2017

eddyb Aug 29, 2017

nikomatsakis Aug 29, 2017

eddyb Aug 29, 2017

alexcrichton Aug 29, 2017

eddyb Aug 29, 2017

nikomatsakis left a comment

eddyb commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

nikomatsakis commented Aug 30, 2017

alexcrichton commented Aug 30, 2017

bors commented Aug 31, 2017

alexcrichton Aug 31, 2017

nikomatsakis Sep 5, 2017

alexcrichton Sep 5, 2017

alexcrichton Aug 31, 2017

nikomatsakis Sep 5, 2017

eddyb Aug 31, 2017

eddyb Aug 31, 2017

bors commented Sep 1, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017

bors commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

Migrate a slew of metadata methods to queries #44142

Migrate a slew of metadata methods to queries #44142

Conversation

alexcrichton commented Aug 29, 2017 • edited Loading

alexcrichton commented Aug 29, 2017

rust-highfive commented Aug 29, 2017

kennytm commented Aug 29, 2017 • edited Loading

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

nikomatsakis left a comment

Choose a reason for hiding this comment

eddyb commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

alexcrichton commented Aug 29, 2017

nikomatsakis commented Aug 30, 2017

alexcrichton commented Aug 30, 2017

bors commented Aug 31, 2017

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 Sep 1, 2017

bors commented Sep 7, 2017

kennytm commented Sep 7, 2017

bors commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

alexcrichton commented Sep 8, 2017

alexcrichton commented Aug 29, 2017 •

edited

Loading

kennytm commented Aug 29, 2017 •

edited

Loading