Store hir_id_to_def_id in OwnerInfo.

[spastorino]

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

[rust-highfive]

r? @estebank

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

[spastorino]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

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

[bors]

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

[rust-timer]

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

[rust-timer]

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

[spastorino]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit a8ca8db17864e9a707c93103d3f9d5a13b44fa9f with merge b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90...

[spastorino]

@bors rollup=always

r? @oli-obk

[bjorn3]

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

[bors]

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

[rust-timer]

Queued b9dd0d7ae1f6f4c3c1a8764996ea9c608d131b90 with parent 788b1fe, future comparison URL.

[spastorino]

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]

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

[rust-timer]

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]

@bors rollup=never

[oli-obk]

@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]

@bors r+

[bors]

📌 Commit 384189c has been approved by oli-obk

[cjgillot]

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.

[oli-obk]

😆 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.

[cjgillot]

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.

[oli-obk]

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

cc @spastorino

[cjgillot]

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

[bors]

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

[bors]

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

[rust-timer]

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]

@rustbot label: +perf-regression-triaged

See #93301 (comment)

[oli-obk]

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 ^^