Check that a query has not completed and is not executing before starting it #108845
Conversation
rustbot
added
A-query-system
| // For the parallel compiler we need to check both the query cache and query state structures | ||
| // while holding the state lock to ensure that 1) the query has not yet completed and 2) the | ||
| // query is not still executing. | ||
| if cfg!(parallel_compiler) && qcx.is_sync() { | ||
| if let Some((value, index)) = Q::query_cache(qcx).lookup(&key) { | ||
| qcx.dep_context().profiler().query_cache_hit(index.into()); | ||
| return (value, Some(index)); | ||
| } | ||
| } |
There was a problem hiding this comment.
This check should happen in JobOwner::try_start which should return a JobCompleted.
There was a problem hiding this comment.
try_start isn't generic over query values, presumably for compile time reasons. Also JobCompleted is currently used as "job completed after blocking". It might make sense to refactor try_execute_query and try_start, but I'll wait for some PRs to merge before trying that.
|
I added a more extended explanation.
That sounds like an expensive way to avoid this check. It is very likely hot in cache for the common |
| // re-executing the query since `try_start` only checks that the query is not currently | ||
| // executing, but another thread may have already completed the query and stores it result | ||
| // in the query cache. | ||
| if cfg!(parallel_compiler) && qcx.is_sync() { |
There was a problem hiding this comment.
The session is already accessible, no need for an extra method.
| if cfg!(parallel_compiler) && qcx.is_sync() { | |
| if cfg!(parallel_compiler) && qcx.dep_context().sess().threads() > 1 { |
r=me with the is_sync method removed.
cjgillot
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #108167) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@bors r+ rollup=iffy |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (938afba): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. |
This fixes a race in the query system where we only checked if the query was currently executing, but not if it was already completed, causing queries to re-execute.
r? @cjgillot