Skip to content

try_execute_query tweaks#153766

Merged
rust-bors[bot] merged 3 commits intorust-lang:mainfrom
nnethercote:try_execute_query-tweaks
Mar 13, 2026
Merged

try_execute_query tweaks#153766
rust-bors[bot] merged 3 commits intorust-lang:mainfrom
nnethercote:try_execute_query-tweaks

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Mar 12, 2026 •
edited
Loading

Details in individual commits.

r? @cjgillot

@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 12, 2026
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 12, 2026

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rust-bors

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Mar 12, 2026
@bors
Auto merge of #153766 - nnethercote:try_execute_query-tweaks, r=<try>
6917c78 
`try_execute_query` tweaks
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2026
jyn514
jyn514 approved these changes Mar 12, 2026
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment •
edited by rustbot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if perf is good

View changes since this review

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 12, 2026

☀️ Try build successful (CI)
Build commit: 6917c78 (6917c783645aa45fb290a314b53f77a3f300d8ce, parent: d1ee5e59a964a419b84b760812a35075034f4861)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

rust-timer commented Mar 12, 2026

Finished benchmarking commit (6917c78): 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
Improvements ✅
(secondary)		 -0.0% [-0.0%, -0.0%] 2
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary -1.7%, secondary -0.4%)

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

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 4.0% [4.0%, 4.0%] 1
Improvements ✅
(primary)		 -1.7% [-1.7%, -1.7%] 1
Improvements ✅
(secondary)		 -4.8% [-4.8%, -4.8%] 1
All ❌✅ (primary) -1.7% [-1.7%, -1.7%] 1

Cycles

Results (secondary -4.3%)

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

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -4.3% [-4.4%, -4.1%] 2
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 480.488s -> 479.599s (-0.19%)
Artifact size: 394.87 MiB -> 396.93 MiB (0.52%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2026
@nnethercote nnethercote marked this pull request as ready for review March 12, 2026 20:30
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 12, 2026
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 12, 2026

Perf is neutral, should be good to go.

@cjgillot
Copy link
Contributor

cjgillot commented Mar 13, 2026

@bors r+

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 13, 2026

📌 Commit f70fb2c has been approved by cjgillot

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2026
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 13, 2026

@bors rollup=maybe

@Zalathar
Copy link
Member

Zalathar commented Mar 13, 2026

I have some nits with check_feedable, but I'd be fine with fixing them in a follow-up if we don't want to derail this approved PR:

  • check_feedable is a pretty confusing function name, since we're not checking that the query is feedable; we're doing some consistency checks that only apply if the query is known to be feedable.
  • The body of check_feedable consists of an if let Some that could be flattened into a let-else with an early return if the cache lookup doesn't find an entry.

Zalathar added a commit to Zalathar/rust that referenced this pull request Mar 13, 2026
@Zalathar
Rollup merge of rust-lang#153766 - nnethercote:try_execute_query-twea…
43e4472 
…ks, r=cjgillot

`try_execute_query` tweaks

Details in individual commits.

r? @cjgillot
@Zalathar Zalathar mentioned this pull request Mar 13, 2026
Closed
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 13, 2026

@bors r-

I can fix those things now, that's easy. Would feedable_checks be a better name?

@rust-bors rust-bors bot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 13, 2026
@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 13, 2026

This pull request was unapproved.

This PR was contained in a rollup (#153802), which was unapproved.

@nnethercote
Don't recompute key_hash in wait_for_query.
73e56e8 
We compute `key_hash` in `try_execute_query`, so we can just pass the
value in.
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 13, 2026

Oh, I didn't realize this was in a rollup...

@nnethercote
Inline and remove execute_job.
8b0c259 
`execute_job` has a single call site in `try_execute_query`. This commit
inlines and removes `execute_job`, but also puts the part that checks
feedable results in its own separate function, `check_feedable`, because
it's a nicely separate piece of logic.

The big win here is that all the code dealing with the `QueryState` is
now together in `try_execute_query`: get the lock, do the lookup, drop
the lock, create the job guard, and complete the job guard. Previously
these steps were split across two functions which I found hard to
follow.

This commit purely moves code around, there are no other changes.
@nnethercote
Some tiny improvements to try_execute_query.
f0c2760 
The previous commit inlined `execute_job` without changing any of its
code. This commit does a few tiny follow-up changes.
@nnethercote nnethercote force-pushed the try_execute_query-tweaks branch from f70fb2c to f0c2760 Compare March 13, 2026 03:04
@rustbot
Copy link
Collaborator

rustbot commented Mar 13, 2026

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.

@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 13, 2026

I addressed @Zalathar's comments.

@bors r=cjgillot

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 13, 2026

📌 Commit f0c2760 has been approved by cjgillot

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 13, 2026
@Zalathar Zalathar mentioned this pull request Mar 13, 2026
Merged
rust-bors bot pushed a commit that referenced this pull request Mar 13, 2026
@bors
Auto merge of #153805 - Zalathar:rollup-rfXICNX, r=Zalathar
7ad5c60 
Rollup of 5 pull requests

Successful merges:

 - #152258 (fixed VecDeque::splice() not filling the buffer correctly when resizing the buffer on start = end range)
 - #153691 (Miscellaneous tweaks)
 - #153766 (`try_execute_query` tweaks)
 - #153188 (implementation of `-Z min-recursion-limit`)
 - #153428 (Avoid duplicated query modifier comments.)
@rust-bors rust-bors bot merged commit a8290c6 into rust-lang:main Mar 13, 2026
11 checks passed
@rustbot rustbot added this to the 1.96.0 milestone Mar 13, 2026
@nnethercote nnethercote deleted the try_execute_query-tweaks branch March 13, 2026 08:20
@Zalathar Zalathar mentioned this pull request Mar 13, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@jyn514 jyn514 jyn514 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cjgillot cjgillot

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.96.0

Development

Successfully merging this pull request may close these issues.

6 participants

@nnethercote @rust-timer @cjgillot @Zalathar @rustbot @jyn514