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.