Merge several HIR-level queries into one#155678
Conversation
This comment has been minimized.
This comment has been minimized.
7b9e758 to
67a6a79
Compare
|
@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.
experiment: single owner query
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (a7e17f9): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @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.5%, secondary -0.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 40.1%, secondary 5.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 500.795s -> 487.693s (-2.62%) |
69beb66 to
464a7a0
Compare
|
@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.
experiment: single owner query
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (7769c1a): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing 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 -0.0%, secondary 2.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.9%, secondary -0.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary -0.0%, secondary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 493.065s -> 491.562s (-0.30%) |
This comment has been minimized.
This comment has been minimized.
ba05724 to
5bff8fa
Compare
|
Finished benchmarking commit (fd2440f): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing 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 1.4%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.6%, secondary 1.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (secondary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 510.527s -> 514.955s (0.87%) |
556806c to
1ebbd9b
Compare
|
Requested reviewer is already assigned to this pull request. Please choose another assignee. |
1ebbd9b to
5f6b026
Compare
| } | ||
| } | ||
|
|
||
| /// This function is used only inside eval-always query analysis |
There was a problem hiding this comment.
Assert (or debug assert that it's invoked from an eval-always query
| } | ||
|
|
||
| query delayed_owner(def_id: LocalDefId) -> hir::MaybeOwner<'tcx> { | ||
| query owner(def_id: LocalDefId) -> &'tcx rustc_middle::hir::ProjectedMaybeOwner<'tcx> { |
There was a problem hiding this comment.
Is arena-caching this useful considering all fields are references, too?
|
Reminder, once the PR becomes ready for a review, use |
| } | ||
|
|
||
| impl<'tcx> ProjectedMaybeOwner<'tcx> { | ||
| pub fn create_from(value: &'tcx MaybeOwner<'tcx>, def_id: LocalDefId) -> Self { |
There was a problem hiding this comment.
| pub fn create_from(value: &'tcx MaybeOwner<'tcx>, def_id: LocalDefId) -> Self { | |
| pub fn new(value: &'tcx MaybeOwner<'tcx>, def_id: LocalDefId) -> Self { |
|
|
||
| #[inline] | ||
| pub fn opt_hir_owner_nodes(self, def_id: LocalDefId) -> Option<&'tcx OwnerNodes<'tcx>> { | ||
| self.owner(def_id).as_owner().map(|i| i.nodes) |
There was a problem hiding this comment.
Style nit: 3 owner maps and 3 different names for the same thing (o, i, owner_info) 😄
|
@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.
Merge several HIR-level queries into one
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (8859956): comparison URL. Overall result: ✅ improvements - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing 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 -0.8%, secondary 1.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 510.626s -> 510.588s (-0.01%) |
|
|
||
| /// Attributes owned by a HIR owner. | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone)] |
There was a problem hiding this comment.
Where do we clone this ?
| } | ||
|
|
||
| query delayed_owner(def_id: LocalDefId) -> hir::MaybeOwner<'tcx> { | ||
| query owner(def_id: LocalDefId) -> &'tcx rustc_middle::hir::ProjectedMaybeOwner<'tcx> { |
There was a problem hiding this comment.
Could you keep a name starting with hir_ ?
| } | ||
|
|
||
| query delayed_owner(def_id: LocalDefId) -> hir::MaybeOwner<'tcx> { | ||
| query owner(def_id: LocalDefId) -> &'tcx rustc_middle::hir::ProjectedMaybeOwner<'tcx> { |
There was a problem hiding this comment.
Why can't we just return a MaybeOwner ?
| // This query has to be `no_hash` and `eval_always`, | ||
| // because it accesses `delayed_lints` which is not hashed as part of the HIR | ||
| no_hash | ||
| eval_always |
There was a problem hiding this comment.
How do we ensure that we are not leaking untracked state ?
View all comments
Now four queries (
local_def_id_to_hir_id,opt_hir_owner_nodes,opt_ast_lowering_delayed_lints,in_scope_traits_map) were replaced with regular methods which acts like getters.An
ownerquery was added that returns aProjectedMaybeOwnerthat contains all those fields.hir_attr_mapremains a separate query as adding attributes toProjectedMaybeOwnerled to perf regressions.There is a similar issue with
in_scopes_trait_map, but according to the comments from rust-lang/rustc-perf#2436 it is a rare case.Most of the changes in incremental tests are renames from
opt_hir_owner_nodes->owner, but there are few cases when new dirty queries were added.r? @petrochenkov
r? @oli-obk