Skip to content

Refactor execute_job_incr#154279

Draft
nnethercote wants to merge 8 commits intorust-lang:mainfrom
nnethercote:refactor-execute_job_incr
Draft

Refactor execute_job_incr#154279
nnethercote wants to merge 8 commits intorust-lang:mainfrom
nnethercote:refactor-execute_job_incr

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Mar 23, 2026

This PR improves some clumsy control flow in execute_job_incr, and also fixes some inconsistencies the the scopes of start_query and query profiling.

@nnethercote
[From rust-lang#154076] Remove some dep-graph assertions.
9c77169 
The call chain for a non-incremental query includes the following
functions:

- execute_query_non_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_non_incr (assert!)

And likewise for an incremental query:

- execute_query_incr_inner (assert!)
- try_execute_query (assert!)
- execute_job_incr (expect)

That is five distinct functions. Every one of them has an `assert!` or
`expect` call that checks that the dep-graph is/is not enabled as
expected. Three cheers for defensive programming but this feels like
overkill, particularly when `execute_job{,_non_incr,_incr}` each have a
single call site.

This commit removes the assertions in `execute_query_*` and
`try_execute_query`, leaving a check in each of the `execute_job_*`
functions.
@nnethercote
[From rust-lang#154076] Compute DepNode for incremental queries.
4e05cdb 
Prior to rust-lang#154122 it wasn't used on all paths, so we only computed it
when necessary -- sometimes in `check_if_ensure_can_skip_execution`,
sometimes in one of two places in `execute_job_incr` -- and pass around
`Option<DepNode>` to allow this.

But now it's always used, so this commit makes us compute it earlier,
which simplifies things.
- `check_if_ensure_can_skip_execution` can be made simpler, returning a
  bool and eliminating the need for `EnsureCanSkip`.
- `execute_job_incr` no longer uses two slightly different methods to
  create a `DepNode` (`get_or_insert_with` vs `unwrap_or_else`).
@nnethercote
Shrink start_query scope.
9e2971f 
There are three `start_query` calls. Two of them have very small scopes,
just an `invoke_provider_fn` call and very little else. But the one in
`execute_job_incr` has a huge scope that covers a `try_mark_green` call
and everything done by `load_from_disk_or_invoke_provider_green`: a disk
load attempt, `prof_timer` start/stop; an `invoke_provider_fn` call, and
hash verification.

This commit moves the `start_query` call into
`load_from_disk_or_invoke_provider_green` to make the scope match the
other two cases.
@nnethercote
Move the cache_on_disk check out of try_load_from_disk_fn.
ec28a9b 
It doesn't need to be in there. Removing it makes `define_queries`
smaller, which is always good, and also enables the next commit which
tidies up profiling.

Also change how `value` and `verify` are initialized, because I just
don't like the current way of doing it after the declaration.
@nnethercote
Move profiling of query loading outwards.
25eb7f0 
From `plumbing.rs` to `execution.rs`, which is where most of the other
query profiling occurs. It also avoids eliminates some fn parameters.

This means the `provided_to_erased` call in `try_from_load_disk_fn` is
now included in the profiling when previously it wasn't. This is
good because `provided_to_erased` is included in other profiling calls
(e.g. calls to `invoke_provider_fn`).
@nnethercote
Inline and remove load_from_disk_or_invoke_provider_green.
454045f 
It has a single call site. After inlining we can refactor
`execute_job_incr` quite a bit and the overall result is simpler and
clearer, especially the control flow -- no `try` block in an `if`
condition(!), no early returns, just vanilla `if`s and `match`es and
about -40 lines of code.
@nnethercote
Comment fiddling.
26417d7 
Remove a low-value comment, and add a comment about hashing explaining
something that I found non-obvious.
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 23, 2026
@nnethercote
Pass QueryVTable to two functions instead of two fn pointers.
745e144 
Specifically, `DepGraph::with_feed_task` and `incremental_verify_ich`
This makes some things shorter, at the cost of making module `dep_graph`
depend directly on module `query`. (It already depends indirectly via
module `verify_ich`.)
@Zalathar
Copy link
Member

Zalathar commented Mar 24, 2026

I feel very uneasy about the start_query change, because the commit message seems to be making an argument on aesthetic/consistency grounds, but doesn't have any discussion of what the change is actually doing, or whether it's correct.

@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 24, 2026

Ok, so what does start_query do?

  • Maybe checks the query depth limit.
  • Sets up ImplicitCtxt with the given query job id and a (maybe) new depth-limit. These values won't be used unless we end up calling try_execute_query within the given closure.

Let's think about the things that were within start_query and no longer are.

  • Loading a value from disk, query profiling start/stop, and hash verification: these are unaffected because they don't interact with ImplicitCtxt's query job id and depth-limit.
  • try_mark_green: this is the interesting one.
    • It can (eventually) call force_query, which calls try_execute_query, which uses ImplicitCtxt and calls start_query again, and then invoke_provider_fn.
    • So the old code could call start_query twice on the way to a single invoke_provider_fn. This will result in ImplicitCtxt being set for a second time, albeit with the same values. This would mess up the depth limit check, except that false is passed as the second argument to the start_query call in execute_job_incr, so the check is skipped. (This might also explain the comment "The diagnostics for this query will be promoted to the current session during try_mark_green(), so we can ignore them here." Not sure.)
    • So I think the new code is better: we avoid calling start_query twice on the try_mark_green path, avoid the redundant setting of ImplicitCtxt. And that false argument to start_query can be changed to query.depth_limit for another consistency win.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nnethercote @Zalathar @rustbot