Skip to content

Remove impl QueryVTable#153323

Open
nnethercote wants to merge 2 commits intorust-lang:mainfrom
nnethercote:rm-impl-QueryVTable
Open

Remove impl QueryVTable#153323
nnethercote wants to merge 2 commits intorust-lang:mainfrom
nnethercote:rm-impl-QueryVTable

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Mar 3, 2026 •
edited by rustbot
Loading

Some simplifications to QueryVTable, partly extracted from #153065.

r? @Zalathar

@nnethercote
Streamline cache-related query functions.
4acadef 
There are three query vtable functions related to the `cache_on_disk_if`
modifier:
- `will_cache_on_disk_for_key_fn`
- `try_load_from_disk_fn`
- `is_loadable_from_disk_fn`

These are all function ptrs within an `Option`. They each have a wrapper
that returns `false`/`None` if the function ptr is missing.

This commit removes the `Option` wrappers. In the `None` case we now set
the function ptr to a trivial closure that returns `false`/`None`. The
commit also removes some typedefs that each have a single use. All this
is a bit simpler, with less indirection.

Note that `QueryVTable::hash_value_fn` is still an `Option`. Unlike the
above three functions, which have trivial behaviour in the `None` case,
`hash_value_fn` is used in ways where the `Some`/`None` distinction is
more meaningful.
@nnethercote
Remove QueryVTable::construct_dep_node.
a8af604 
It's just a wrapper for `DepNode::construct` and it only has three uses.
Better to just have one way of doing things.
@rustbot rustbot added A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 3, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2026

Zalathar is not on the review rotation at the moment.
They may take a while to respond.

@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 3, 2026

@Zalathar: as mentioned elsewhere, this partly undoes #151736. Not sure if you'll be accept it, but at least I won't die wondering.

@Zalathar
Copy link
Member

Zalathar commented Mar 3, 2026

I'm personally a little sad to see the options removed, but it's true that we currently aren't making any particular use of them (unlike hash_value_fn), so if perf is fine then it's hard for me to argue for keeping them as things currently stand.

I guess if I ever find a specific use case for wanting to know whether a query vtable ever caches to disk, I can always reintroduce a way to check that.

@Zalathar
Copy link
Member

Zalathar commented Mar 3, 2026

I think we already found no perf effects in #153065 (comment), but it's easy enough to double-check.

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
rust-bors bot pushed a commit that referenced this pull request Mar 3, 2026
@bors
Auto merge of #153323 - nnethercote:rm-impl-QueryVTable, r=<try>
0528e08 
Remove `impl QueryVTable`
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2026
@rust-bors

This comment has been minimized.

Sign in to view
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 3, 2026

BTW this will conflict with #153326 but I'm happy for that PR to land first.

@rust-bors
Copy link
Contributor

rust-bors bot commented Mar 3, 2026

☀️ Try build successful (CI)
Build commit: 0528e08 (0528e08a4da0a20fed6155ee91715eed54bee2fd, parent: ec818fda361ca216eb186f5cf45131bd9c776bb4)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

rust-timer commented Mar 3, 2026

Finished benchmarking commit (0528e08): 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.3% [-0.3%, -0.3%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (secondary -2.8%)

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)		 -2.8% [-2.8%, -2.8%] 1
All ❌✅ (primary) - - 0

Cycles

Results (secondary 3.1%)

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)		 3.1% [3.1%, 3.1%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 480.002s -> 481.596s (0.33%)
Artifact size: 394.97 MiB -> 394.98 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 3, 2026
@nnethercote
Copy link
Contributor Author

nnethercote commented Mar 3, 2026

@bors rollup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Zalathar Zalathar

Labels

A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nnethercote @rustbot @Zalathar @rust-timer