Store hir_id_to_def_id in OwnerInfo. #93301

spastorino · 2022-01-25T18:06:41Z

This is for perf test purposes only. Related to #89278

rust-highfive · 2022-01-25T18:06:45Z

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

spastorino · 2022-01-25T18:07:05Z

@bors try @rust-timer queue

rust-timer · 2022-01-25T18:07:06Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-25T18:10:22Z

⌛ Trying commit 80132c3 with merge 2717408ef4fd9ac31b62252f88820ae21a15c5a8...

bors · 2022-01-25T19:40:37Z

☀️ Try build successful - checks-actions
Build commit: 2717408ef4fd9ac31b62252f88820ae21a15c5a8 (2717408ef4fd9ac31b62252f88820ae21a15c5a8)

rust-timer · 2022-01-25T19:40:39Z

Queued 2717408ef4fd9ac31b62252f88820ae21a15c5a8 with parent 17dfae7, future comparison URL.

rust-timer · 2022-01-25T21:53:28Z

Finished benchmarking commit (2717408ef4fd9ac31b62252f88820ae21a15c5a8): comparison url.

Summary: This benchmark run shows 30 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.5%
Largest regression in instruction counts: 2.3% on incr-unchanged builds of match-stress-enum check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

compiler/rustc_ast_lowering/src/lib.rs

spastorino · 2022-01-26T12:56:44Z

@bors try @rust-timer queue

rust-timer · 2022-01-26T12:56:46Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-01-26T12:56:52Z

⌛ Trying commit a8ca8db17864e9a707c93103d3f9d5a13b44fa9f with merge b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90...

spastorino · 2022-01-26T12:57:47Z

@bors rollup=always

r? @oli-obk

bjorn3 · 2022-01-26T14:21:36Z

Shouldn't this be rollup=never given that it is expected to have a perf effect?

bors · 2022-01-26T14:23:28Z

☀️ Try build successful - checks-actions
Build commit: b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90 (b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90)

rust-timer · 2022-01-26T14:23:29Z

Queued b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90 with parent 788b1fe, future comparison URL.

spastorino · 2022-01-26T14:54:03Z

Shouldn't this be rollup=never given that it is expected to have a perf effect?

Agreed, I was doing that based on not seeing any perf change with this last commit added. This PR is not really about perf but the particular commit included was part of a larger change that was giving some perf issues, we wanted to be sure that this commit was not part of the regression.
Anyway, let's see what perf says, if there are regressions for sure we would need to remove rollup=never and justify if we want to land it, otherwise, if perf is correct I think this can be rollup=always.

spastorino · 2022-01-26T15:26:25Z

After perf run I've only added a commit that blesses some tests.

rust-timer · 2022-01-26T15:54:37Z

Finished benchmarking commit (b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90): comparison url.

Summary: This benchmark run shows 41 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.5%
Average relevant improvement: -0.5%
Largest regression in instruction counts: 2.3% on incr-unchanged builds of match-stress-enum check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

spastorino · 2022-01-26T16:05:46Z

@bors rollup=never

oli-obk · 2022-01-26T16:15:18Z

@rustbot label: +perf-regression-triaged

The regression is expected and unavoidable. We call a query in opt_local_def_id now, and even if it is always cached, calling a cached query is simply more expensive than just a hash table lookup.

--------------------------------------------------------------------------------
Ir         
--------------------------------------------------------------------------------
14,304,468  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
13,698,023  ???:<rustc_middle::hir::map::Map>::local_def_id
 2,216,018  ???:<rustc_resolve::Resolver as rustc_ast_lowering::ResolverAstLowering>::opt_local_def_id
 1,786,436  ???:<rustc_ast_lowering::LoweringContext>::make_owner_info
-1,406,859  ???:<core::iter::adapters::map::Map<core::iter::adapters::enumerate::Enumerate<core::slice::iter::Iter<core::option::Option<rustc_hir::hir_id::HirId>>>, <rustc_index::vec::IndexVec<rustc_span::def_id::LocalDefId, core::option::Option<rustc_hir::hir_id::HirId>>>::iter_enumerated::{closure
-1,299,881  ???:<hashbrown::raw::RawTable<(rustc_hir::hir_id::HirId, rustc_span::def_id::LocalDefId)>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_hir::hir_id::HirId, rustc_hir::hir_id::HirId, rustc_span::def_id::LocalDefId, core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
 1,245,119  ???:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure
  -950,272  ???:<rustc_privacy::EmbargoVisitor as rustc_hir::intravisit::Visitor>::visit_item
  -491,462  ???:<rustc_query_system::dep_graph::serialized::SerializedDepGraph<rustc_middle::dep_graph::dep_node::DepKind> as rustc_serialize::serialize::Decodable<rustc_serialize::opaque::Decoder>>::decode
  -343,403  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
  -303,104  ???:<rustc_passes::stability::Annotator as rustc_hir::intravisit::Visitor>::visit_variant
   245,809  ???:<rustc_middle::ty::sty::TyKind as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable
  -132,146  ???:<core::panic::unwind_safe::AssertUnwindSafe<rustc_interface::passes::analysis::{closure
   131,853  ???:<rustc_session::session::Session>::time::<(), rustc_interface::passes::analysis::{closure
   131,316  ???:core::slice::sort::recurse::<(rustc_hir::hir_id::ItemLocalId, rustc_span::def_id::LocalDefId), <[(rustc_hir::hir_id::ItemLocalId, rustc_span::def_id::LocalDefId)]>::sort_unstable_by<<rustc_data_structures::sorted_map::SortedMap<rustc_hir::hir_id::ItemLocalId, rustc_span::def_id::LocalDefId> as core::iter::traits::collect::FromIterator<(rustc_hir::hir_id::ItemLocalId, rustc_span::def_id::LocalDefId)>>::from_iter<core::iter::adapters::filter_map::FilterMap<core::slice::iter::Iter<rustc_ast::node_id::NodeId>, <rustc_ast_lowering::LoweringContext>::make_owner_info::{closure
  -129,050  ???:<rustc_span::span_encoding::Span as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable
    73,719  ???:rustc_middle::ty::context::tls::TLV::__getit
   -67,486  ???:<rustc_resolve::Resolver>::resolve_path_with_ribs
   -63,603  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S:__memset_avx2_erms
   -36,645  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_sse2_unaligned_erms
    24,576  ???:<rustc_resolve::access_levels::AccessLevelsVisitor as rustc_ast::visit::Visitor>::visit_item
    16,335  /build/glibc-eX1tMB/glibc-2.31/string/../sysdeps/x86_64/strcmp.S:strcmp

oli-obk · 2022-01-26T16:15:24Z

@bors r+

bors · 2022-01-26T16:15:25Z

📌 Commit 384189c has been approved by oli-obk

cjgillot · 2022-01-26T16:53:41Z

compiler/rustc_hir/src/stable_hash_impls.rs

+            hash_without_bodies: _,
+            nodes: _,
+            bodies: _,
+            local_id_to_def_id: _,


Good idea! I think that skipping the hash for local_id_to_def_id is safe, but the rationale definitely deserves a comment: all the information used to create a DefPathHash exist within the HIR: the parent as the top node's LocalDefId, it's DefPathData as the node's ident and kind, and the disambiguator from the order in nodes.

😆 this is your code, we just accidentally took it over and I didn't realize at all what was going on.

Since we require

x != y implies hash_stable(x) != hash_stable(y)

for stable hashing, but any info in local_id_to_def_id is dependent on the body, then just hashing the body satisfies this condition.

I did not intend to congratulate myself. Sorry for the impoliteness.
I just thought it was not obvious that any info in local_id_to_def_id is dependent on the body, because the LocalDefIds do not appear inside HIR. I'll probably end up adding the comment in another PR, no need to block this one.

Oops I thought we added the comment. Yea, let's do it in a follow up

cc @spastorino

cjgillot · 2022-01-26T19:01:42Z

What's the difference with #89278? How did you manage to tame the regression?

bors · 2022-01-26T19:45:17Z

⌛ Testing commit 384189c with merge 6abb638...

bors · 2022-01-26T22:54:20Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6abb638 to master...

rust-timer · 2022-01-27T00:25:32Z

Finished benchmarking commit (6abb638): comparison url.

Summary: This benchmark run shows 6 relevant improvements 🎉 but 25 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.6%
Average relevant improvement: -0.5%
Largest improvement in instruction counts: -0.6% on full builds of unused-warnings check
Largest regression in instruction counts: 2.2% on incr-unchanged builds of match-stress-enum check

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

oli-obk · 2022-01-27T10:31:08Z

@rustbot label: +perf-regression-triaged

See #93301 (comment)

oli-obk · 2022-01-27T10:32:39Z

What's the difference with #89278? How did you manage to tame the regression?

This PR is just the first commit, we'll analyze the perf on the second big commit in isolation. Divide and conquer ^^

Store hir_id_to_def_id in OwnerInfo.

80132c3

rust-highfive assigned estebank Jan 25, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 25, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 25, 2022

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jan 25, 2022

oli-obk reviewed Jan 26, 2022

View reviewed changes

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2022

spastorino changed the title ~~[DO NOT MERGE] Store hir_id_to_def_id in OwnerInfo.~~ Store hir_id_to_def_id in OwnerInfo. Jan 26, 2022

rust-highfive assigned oli-obk and unassigned estebank Jan 26, 2022

Bless incremental tests.

14a64e0

Filter out local_id == 0, those are already considered on the call site

384189c

spastorino force-pushed the perf-test-1 branch from a8ca8db to 384189c Compare January 26, 2022 15:25

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 26, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jan 26, 2022

bors 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 Jan 26, 2022

cjgillot reviewed Jan 26, 2022

View reviewed changes

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 26, 2022

bors merged commit 6abb638 into rust-lang:master Jan 26, 2022

rustbot added this to the 1.60.0 milestone Jan 26, 2022

This was referenced Jan 26, 2022

Encode spans relative to the enclosing item -- enable on nightly #84762

Merged

Store hir_id_to_def_id in OwnerNodes. #91112

Closed

cjgillot mentioned this pull request Feb 1, 2022

Add queries for LocalDefId <-> HirId conversion (take 2) #75641

Closed

rylev mentioned this pull request Feb 2, 2022

Store def_id_to_hir_id as variant in hir_owner. #93373

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store hir_id_to_def_id in OwnerInfo. #93301

Store hir_id_to_def_id in OwnerInfo. #93301

spastorino commented Jan 25, 2022 •

edited

Loading

rust-highfive commented Jan 25, 2022

spastorino commented Jan 25, 2022

rust-timer commented Jan 25, 2022

bors commented Jan 25, 2022

bors commented Jan 25, 2022

rust-timer commented Jan 25, 2022

rust-timer commented Jan 25, 2022

spastorino commented Jan 26, 2022

rust-timer commented Jan 26, 2022

bors commented Jan 26, 2022

spastorino commented Jan 26, 2022

bjorn3 commented Jan 26, 2022

bors commented Jan 26, 2022

rust-timer commented Jan 26, 2022

spastorino commented Jan 26, 2022

spastorino commented Jan 26, 2022

rust-timer commented Jan 26, 2022

spastorino commented Jan 26, 2022

oli-obk commented Jan 26, 2022

oli-obk commented Jan 26, 2022

bors commented Jan 26, 2022

cjgillot Jan 26, 2022

oli-obk Jan 26, 2022

cjgillot Jan 26, 2022

oli-obk Jan 27, 2022

cjgillot commented Jan 26, 2022

bors commented Jan 26, 2022

bors commented Jan 26, 2022

rust-timer commented Jan 27, 2022

oli-obk commented Jan 27, 2022

oli-obk commented Jan 27, 2022

Store hir_id_to_def_id in OwnerInfo. #93301

Store hir_id_to_def_id in OwnerInfo. #93301

Conversation

spastorino commented Jan 25, 2022 • edited Loading

rust-highfive commented Jan 25, 2022

spastorino commented Jan 25, 2022

rust-timer commented Jan 25, 2022

bors commented Jan 25, 2022

bors commented Jan 25, 2022

rust-timer commented Jan 25, 2022

rust-timer commented Jan 25, 2022

spastorino commented Jan 26, 2022

rust-timer commented Jan 26, 2022

bors commented Jan 26, 2022

spastorino commented Jan 26, 2022

bjorn3 commented Jan 26, 2022

bors commented Jan 26, 2022

rust-timer commented Jan 26, 2022

spastorino commented Jan 26, 2022

spastorino commented Jan 26, 2022

rust-timer commented Jan 26, 2022

spastorino commented Jan 26, 2022

oli-obk commented Jan 26, 2022

oli-obk commented Jan 26, 2022

bors commented Jan 26, 2022

cjgillot Jan 26, 2022

Choose a reason for hiding this comment

oli-obk Jan 26, 2022

Choose a reason for hiding this comment

cjgillot Jan 26, 2022

Choose a reason for hiding this comment

oli-obk Jan 27, 2022

Choose a reason for hiding this comment

cjgillot commented Jan 26, 2022

bors commented Jan 26, 2022

bors commented Jan 26, 2022

rust-timer commented Jan 27, 2022

oli-obk commented Jan 27, 2022

oli-obk commented Jan 27, 2022

spastorino commented Jan 25, 2022 •

edited

Loading