Conversation
rustbot
added
A-query-system
|
|
|
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 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. |
|
I think we already found no perf effects in #153065 (comment), but it's easy enough to double-check. @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Remove `impl QueryVTable`
rustbot
added
the
S-waiting-on-perf
This comment has been minimized.
This comment has been minimized.
|
BTW this will conflict with #153326 but I'm happy for that PR to land first. |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (0528e08): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking 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 Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
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.
CyclesResults (secondary 3.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.002s -> 481.596s (0.33%) |
rustbot
removed
the
S-waiting-on-perf
|
@bors rollup |
This comment has been minimized.
This comment has been minimized.
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.
It's just a wrapper for `DepNode::construct` and it only has three uses. Better to just have one way of doing things.
nnethercote
force-pushed
the
rm-impl-QueryVTable
branch
from
a8af604 to
72c092c
Compare
That PR has landed and I have rebased. Ready for review. |
|
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. |
| pub state: QueryState<'tcx, C::Key>, | ||
| pub cache: C, | ||
| pub will_cache_on_disk_for_key_fn: Option<fn(tcx: TyCtxt<'tcx>, key: &C::Key) -> bool>, | ||
| pub will_cache_on_disk_for_key_fn: fn(tcx: TyCtxt<'tcx>, key: &C::Key) -> bool, |
There was a problem hiding this comment.
Pre-existing nit: Since we're touching this field anyway, could you move it down to just above try_load_from_disk_fn, so that the three disk-cache functions are grouped together?
|
r=me after considering the nit. |
4 participants
Some simplifications to
QueryVTable, partly extracted from #153065.r? @Zalathar