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

incremental: Initial "always dirty" queries #44234

Closed
alexcrichton opened this issue Sep 1, 2017 · 9 comments
Closed

incremental: Initial "always dirty" queries #44234

alexcrichton opened this issue Sep 1, 2017 · 9 comments
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

In working on #44142 I've come across the need/desire a few times to have "always dirty" nodes that are always recomputed at the base of the dependency graph. One example of this is handling today's extern_mod_stmt_cnum method.

Today there's a method CrateStore::extern_mod_stmt_cnum, but as part of #41417 we want to move this to a query. That's relatively simple but what's actually happening here I think is a bit more subtle in terms of dependencies. This query will take the ID of an extern crate statement and return the CrateNum that it loaded. This happens very early in the compiler when we're loading crates and it's basically "however CrateStore is implemented picks these numbers".

Right now, on my currently unmerged branch, the implementation looks like this:

        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)
        },

So in other words this is just defining a query that doesn't actually have any dependencies! The extern_mod_stmt_cnum_untracked method just reaches into the internals of CStore and plucks a CrateNum seemingly out of thin air, returning it.

Ideally I think what we'll want here is a way of saying that queries like this need to be computed 100% of the time to determine if they're red or green. Typically they'll instantly turn green again which'll allow us to have lots of cache hits, but I'm worried about assumign they're always green because they have no inputs.

I believe this is a similar-ish issue to many of the maps in #44137 as well. For all the maps calculated in resolve then then hidden behind a query in TyCtxt those nodes don't actually have any dependencies, they're just reading internal tables. We should always rerun the query though to ensure that it is properly tracked!

cc @nikomatsakis, @michaelwoerister

@alexcrichton alexcrichton added the A-incr-comp Area: Incremental compilation label Sep 1, 2017
@shepmaster shepmaster added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 1, 2017
@michaelwoerister
Copy link
Member

Yes, I agree. I think what @nikomatsakis calls "constant data" in #44137 is really "input data" as far as the query system is concerned. Inputs have no dependencies, they always need to be read and checked for changes.

@michaelwoerister
Copy link
Member

Right now a DepNode can be marked as [anon]. One way to handle this would be to add a [input] modifier that informs the query system that it must treat the dependency differently.

@michaelwoerister
Copy link
Member

cc @eddyb

@nikomatsakis
Copy link
Contributor

Yes so I'm in favor of this. I think roughly speaking the idea of an "input" query would just be that it's a query where we can never re-use the results. Seems like a simple thing for us to add and it will no doubt remain useful until such time as we push the query system all the way throughout the compiler (and maybe even then...).

(For example, it could eventually subsume the existing handling of HIR nodes -- i.e., we could just have a hir(def-id) query that returns the data, and let the natural hashing take care of it.)

@michaelwoerister this does suggest that perhaps moving towards a lazy model of deciding when (e.g.) an input HIR node has changed might be better? i.e., closer to this scheme?

@michaelwoerister
Copy link
Member

this does suggest that perhaps moving towards a lazy model of deciding when (e.g.) an input HIR node has changed might be better? i.e., closer to this scheme?

I think so, yes.

@alexcrichton
Copy link
Member Author

Niko just had an excellent idea. Let's add an always red query called "Input", and then any other query that wants to always run will simply depend on this.

@eddyb
Copy link
Member

eddyb commented Sep 13, 2017

Just to be clear: having the compiler arguments and files read during parsing/macro expansion would serve the same purpose, if we can sneak all of that into queries? If so I'm fine with "Input".

@michaelwoerister
Copy link
Member

Yet, they are not always red :) An input can be green too, it's just that we cannot rely on their (non-existing) dependencies for determining if they are. It would rather have to be a "always re-compute" query.

@michaelwoerister
Copy link
Member

❤️ This has been implement in #45353 by @wesleywiser

pietroalbini added a commit to pietroalbini/rust that referenced this issue Jan 7, 2019
…=michaelwoerister

remove outdated comment

rust-lang#44234 was closed, apparently solved by rust-lang#45353

r? @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants