Refactor execute_job_incr

[nnethercote]

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.

[Zalathar]

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]

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.

[nnethercote]

@bors try @rust-timer queue

[Zalathar]

I had considered doing this in the past, and ultimately backed down because I wasn't sure if I wanted to introduce this C: QueryCache bound here or not.

But it's probably fine, and being able to simplify the signature of incremental_verify_ich is certainly a nice payoff, so +1 on this change.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 237f82c (237f82c3b1536e49cf903c29142efb83d1886e35, parent: 212b0d480f337082bbe1132d2b62be20e7e61f8a)

[rust-timer]

Finished benchmarking commit (237f82c): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.2%	[-0.2%, -0.2%]	2
Improvements ✅ (secondary)	-0.2%	[-0.3%, -0.1%]	13
All ❌✅ (primary)	-0.2%	[-0.2%, -0.2%]	2

Max RSS (memory usage)

Results (primary 2.5%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.5%	[1.6%, 3.5%]	2
Regressions ❌ (secondary)	3.0%	[1.9%, 3.4%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.6%	[-6.4%, -2.7%]	2
All ❌✅ (primary)	2.5%	[1.6%, 3.5%]	2

Cycles

Results (primary 0.3%, secondary -1.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.5%	[2.3%, 2.8%]	2
Regressions ❌ (secondary)	6.0%	[6.0%, 6.0%]	1
Improvements ✅ (primary)	-2.0%	[-2.5%, -1.4%]	2
Improvements ✅ (secondary)	-3.5%	[-4.0%, -2.8%]	3
All ❌✅ (primary)	0.3%	[-2.5%, 2.8%]	4

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 495.299s -> 485.474s (-1.98%)
Artifact size: 394.77 MiB -> 394.80 MiB (0.01%)

[Zalathar]

I was looking at the version of execute_job_incr produced by this PR, and I have to say that overall it does not feel like an improvement.

There are now several multi-level nested conditions, else arms that are very far away from their condition, multiple subtly different kinds of cryptic tuple, and no obvious logical flow overall.

The current version, while far from perfect, at least has a mostly-linear structure with a distinct green path followed by fallback to a red path. The new version makes me immediately want to look for ways to make things more manageable.

EDIT: Specifically, the commit that removes the green-path early return and inlines load_from_disk_or_invoke_provider_green feels like a step backwards. I do like trying to make improvements to the weird structure in that block, but I don't like this particular outcome.

[nnethercote]

Arguing over matters of taste is probably pointless but I'll defend my work in case it stops you from blocking what I think is a good change.

This code is complex because the underlying algorithm is complex. Breaking it in to smaller pieces doesn't change the complexity, it just obscures it. Splitting execute_job_incr gives you two shorter functions that are so tightly intertwined that you have to understand one to understand the other. I would much prefer one coherent 86 line function with all the closely related stuff in it than two awkwardly split functions spanning 122 lines.

Just look at load_from_disk_or_invoke_provider_green. The name alone is a giveaway. It's two different green paths masquerading as one. It has seven parameters. It's the kind of awkward structure you get when someone decides that functions should not exceed a particular number of lines.

[Zalathar]

Just look at load_from_disk_or_invoke_provider_green. The name alone is a giveaway. It's two different green paths masquerading as one. It has seven parameters. It's the kind of awkward structure you get when someone decides that functions should not exceed a particular number of lines.

That function is the green case path. It used to have a straightforward top-to-bottom special-case/fallback structure (much like execute_job_incr) until that structure was removed in #154207, which is already feeling like a big mistake. I trusted the diff and the motivation without properly looking at the end result in context, and here I'm trying not to repeat that error.

Inlining it turns execute_job_incr into ~80 lines of featureless control-flow sludge. I no longer have any frame of reference for where the green path is, or where the non-green path is, or what the try-mark-green is doing or why. No part of the function can be considered in isolation any more. There's just this huge chunk of deeply nested control flow structures, too large to even fit on the screen, and certainly far too complex to keep track of in my head now that all of the helpful hints and landmarks and abstractions have been removed. I can no longer grasp the control flow without constantly juggling multiple layers of nested context. I can no longer focus on any part of the function without having to constantly refer to the entire function as a whole, in all its complexity.

Inlining the green path feels like the result of following a rigid style guide that says all early returns are bad, and all single-call-site functions are bad, and all long parameter lists are bad. It's not simpler, and it's not clearer, and that was supposed to be the whole point.

[rust-bors]

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

[Zoxc]

The start_query change is incorrect as we need set ImplicitCtxt.query before calling try_mark_green in case it forces any queries. Failing to set it means we'll find the wrong edge when looking for query cycles and so we could miss a legitimate cycle.