Feed query values under state lock

[aerooneqq]

Fixes #162316.

The issue did not reproduced during ~100k launches locally. No significant perf changes on random benchmarks locally, but worth perf run anyway. Explanation can be found in comment.

No llms used.

[rustbot]

r? @mejrs

rustbot has assigned @mejrs.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, query-system
• compiler, query-system expanded to 76 candidates
• Random selection from 22 candidates

[petrochenkov]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 176a6a0 (176a6a0804d5fc03409482239ff18cbbb04c3eee)
Base parent: 1edd55d (1edd55dcfcd573872c727fa3e086369a71661ee0)

[rust-timer]

Finished benchmarking commit (176a6a0): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@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.2%	[0.1%, 0.3%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 0.5%, secondary -1.3%)

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

	mean	range	count
Regressions ❌ (primary)	1.9%	[0.6%, 3.1%]	2
Regressions ❌ (secondary)	1.1%	[1.1%, 1.1%]	2
Improvements ✅ (primary)	-0.9%	[-1.2%, -0.5%]	2
Improvements ✅ (secondary)	-3.6%	[-3.8%, -3.4%]	2
All ❌✅ (primary)	0.5%	[-1.2%, 3.1%]	4

Cycles

Results (primary -0.6%, secondary -1.1%)

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

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.7%]	3
Regressions ❌ (secondary)	4.3%	[3.7%, 5.1%]	4
Improvements ✅ (primary)	-0.9%	[-2.6%, -0.4%]	14
Improvements ✅ (secondary)	-3.8%	[-6.6%, -2.3%]	8
All ❌✅ (primary)	-0.6%	[-2.6%, 0.7%]	17

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 481.679s -> 480.794s (-0.18%)
Artifact size: 403.98 MiB -> 403.35 MiB (-0.16%)

[Zoxc]

We should indeed check the cache under the lock. Not sure about the with_feed_task call inside the lock, normally we don't hold these locks during tasks.

[aerooneqq]

Not sure about the with_feed_task call inside the lock, normally we don't hold these locks during tasks.

I put it inside the lock so it won't be executed more than once if several threads will be feeding the same query and key. As far as I understand when executing query in incremental mode, in execute_job_incr we access with_task without lock, but query system guarantees that there will be only one execution for query and key, however we don't have this guarantee while feeding.

[Zoxc]

I looked into it a bit more and there's probably too much code being executed in the lock.

We probably want to instead create a QueryJob to hold while executing, like try_execute_query does for regular queries. That may have some interactions with query cycle detection. We could also skip the first try_get_cached check and only check inside the look, as I expect the first one to not be hot enough to bother.

[mejrs]

r? petrochenkov

[petrochenkov]

Waiting on author to respond to #162515 (comment).
@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.