Streamline QueryVTableUnerased into GetQueryVTable#152841
Streamline QueryVTableUnerased into GetQueryVTable#152841Zalathar wants to merge 1 commit intorust-lang:mainfrom
QueryVTableUnerased into GetQueryVTable#152841Conversation
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Streamline `QueryVTableUnerased` into `QueryVTableGetter`
|
| /// Marker type that implements [`QueryVTableGetter`] for this query. | ||
| pub(crate) enum VTableGetter {} | ||
|
|
||
| impl<'tcx> QueryVTableGetter<'tcx, FLAGS> for VTableGetter { |
There was a problem hiding this comment.
This is a trait with a single implementer (per query).
Would it be hard to refactor it into a free (per query) function somehow?
(Similarly to how restore_val was removed from the trait's interface.)
There was a problem hiding this comment.
Using a free function seems tricky, because the generics needed per callee would get more complicated, and that's something I'm trying to cut down on.
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
|
Looks like I need to try some permutations to track down what’s triggering the perf changes. |
QueryVTableUnerased into QueryVTableGetterQueryVTableUnerased into GetQueryVTable
|
The current iteration only changes the trait itself and |
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Streamline `QueryVTableUnerased` into `GetQueryVTable`
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f16cfc0): comparison URL. Overall result: ❌✅ regressions and 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 (primary 3.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 496.246s -> 482.423s (-2.79%) |
This comment has been minimized.
This comment has been minimized.
|
Let's see what perf looks like after removing @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Streamline `QueryVTableUnerased` into `GetQueryVTable`
|
r? nnethercote (or compiler) |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f8e0d28): 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 (primary 1.1%, secondary 4.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -3.0%)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: 479.41s -> 479.13s (-0.06%) |
|
@bors r+ |
|
These changes should have no perf effects, and the most recent perf run only shows noise, so this PR seems safe for rollup. @bors rollup=maybe |
Streamline `QueryVTableUnerased` into `GetQueryVTable` *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152841)* `QueryDispatcherUnerased` is an awkward name for an awkward trait. We can make it a bit more straightforward by removing its responsibility for erasing query values, and by observing that its only real responsibility beyond that is to know how to obtain the vtable for a particlar query from `tcx`.
Streamline `QueryVTableUnerased` into `GetQueryVTable` *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152841)* `QueryDispatcherUnerased` is an awkward name for an awkward trait. We can make it a bit more straightforward by removing its responsibility for erasing query values, and by observing that its only real responsibility beyond that is to know how to obtain the vtable for a particlar query from `tcx`.
Streamline `QueryVTableUnerased` into `GetQueryVTable` *[View all comments](https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/152841)* `QueryDispatcherUnerased` is an awkward name for an awkward trait. We can make it a bit more straightforward by removing its responsibility for erasing query values, and by observing that its only real responsibility beyond that is to know how to obtain the vtable for a particlar query from `tcx`.
View all comments
QueryDispatcherUnerasedis an awkward name for an awkward trait.We can make it a bit more straightforward by removing its responsibility for erasing query values, and by observing that its only real responsibility beyond that is to know how to obtain the vtable for a particlar query from
tcx.