Avoid unnecessary hashing #108794

nnethercote · 2023-03-06T00:48:33Z

I noticed some stable hashing being done in a non-incremental build. It turns out that some of this is necessary to compute the crate hash, but some of it is not. Removing the unnecessary hashing is a perf win.

r? @cjgillot

nnethercote · 2023-03-06T01:43:51Z

@bors try @rust-timer queue

bors · 2023-03-06T01:44:00Z

⌛ Trying commit b238d9c0b778a6034ad70473bda04ddd8b46760c with merge 8c17dc6956a284528df7e9916a2aa84ebfae0642...

bors · 2023-03-06T04:08:09Z

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

rust-timer · 2023-03-06T05:36:30Z

Finished benchmarking commit (8c17dc6956a284528df7e9916a2aa84ebfae0642): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.2%, 1.1%]	62
Regressions ❌ (secondary)	0.8%	[0.2%, 2.6%]	29
Improvements ✅ (primary)	-1.2%	[-3.0%, -0.3%]	37
Improvements ✅ (secondary)	-1.7%	[-6.9%, -0.2%]	33
All ❌✅ (primary)	-0.2%	[-3.0%, 1.1%]	99

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.2%	[3.2%, 3.2%]	1
Regressions ❌ (secondary)	1.7%	[1.5%, 1.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	3.2%	[3.2%, 3.2%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.5%	[1.1%, 10.2%]	16
Regressions ❌ (secondary)	1.1%	[1.1%, 1.2%]	2
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-3.4%	[-5.0%, -1.5%]	8
All ❌✅ (primary)	4.1%	[-2.1%, 10.2%]	17

nnethercote · 2023-03-06T06:56:59Z

I realized that the crate hash is being computed in cases where it's not necessary (i.e. non-incremental builds of leaf crates). I'm now working on more changes to handle those cases, which bring more performance benefits.

rustbot · 2023-03-06T08:00:36Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

nnethercote · 2023-03-06T08:00:47Z

@bors try @rust-timer queue

bors · 2023-03-06T08:00:56Z

⌛ Trying commit 8953bf35baee8ab06f8ca297621bef253923136d with merge 25e8184a66493827bfc5cbe8d6e5114603d4e4ae...

bors · 2023-03-06T09:57:46Z

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

rust-timer · 2023-03-06T11:22:09Z

Finished benchmarking commit (25e8184a66493827bfc5cbe8d6e5114603d4e4ae): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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 may lead 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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.2%, 0.5%]	21
Regressions ❌ (secondary)	0.5%	[0.4%, 0.7%]	7
Improvements ✅ (primary)	-1.1%	[-3.0%, -0.3%]	37
Improvements ✅ (secondary)	-2.3%	[-11.5%, -0.2%]	46
All ❌✅ (primary)	-0.6%	[-3.0%, 0.5%]	58

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.2%	[0.9%, 3.5%]	2
Regressions ❌ (secondary)	1.8%	[1.8%, 1.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-2.2%, -1.8%]	2
All ❌✅ (primary)	2.2%	[0.9%, 3.5%]	2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.2%	[1.3%, 3.1%]	2
Improvements ✅ (primary)	-1.3%	[-2.3%, -0.9%]	4
Improvements ✅ (secondary)	-5.0%	[-10.1%, -2.5%]	8
All ❌✅ (primary)	-1.3%	[-2.3%, -0.9%]	4

bugadani · 2023-03-06T11:28:28Z

compiler/rustc_session/src/session.rs

+    }
+
+    pub fn metadata_kind(&self) -> MetadataKind {
+        self.crate_types()


Although almost all current regressions are incremental so this isn't a factor, would it not be worth caching this?

It's not showing up at all in profiles, so it's not worth optimizing.

nnethercote · 2023-03-06T21:28:50Z

The CI x86_64-gnu-llvm-14 build is failing very early while building a stage 2 compiler. I can't reproduce this locally with commands like x build --stage=2. Weird.

nnethercote · 2023-03-06T22:02:44Z

The CI x86_64-gnu-llvm-14 build is failing very early while building a stage 2 compiler. I can't reproduce this locally with commands like x build --stage=2. Weird.

Aha, debug-assertions = true in the config.toml is the difference.

nnethercote · 2023-03-06T23:01:05Z

This is now ready for review. One thing about performance: there are lots of sub-1% icount regressions for unchanged/patched incremental runs on CI. I didn't see these in my local builds. I downloaded one of the CI builds and tried that for wg-grammar with IncrUnchanged, the difference seemed to be just random fluctuations in encoding and decoding functions, in a fashion not obviously related to this PR's changes.

--------------------------------------------------------------------------------
Ir
--------------------------------------------------------------------------------
4,689,979  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
10,551,511  ???:rustc_middle::ty::codec::encode_with_shorthand::<rustc_query_impl::on_disk_cache::CacheEncoder, rustc_middle::ty::Ty, <rustc_query_impl::on_disk_cache::CacheEncoder as rustc_type_ir::codec::TyEncoder>::type_shorthands>
-9,904,090  ???:<rustc_middle::ty::context::TyCtxt>::mk_substs_from_iter::<core::iter::adapters::map::Map<core::ops::range::Range<usize>, <&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode::{closure#0}>, rustc_middle::ty::subst::GenericArg>
-4,200,409  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, rustc_middle::ty::Ty, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
-4,120,689  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, &rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 3,947,326  ???:<rustc_middle::ty::subst::GenericArg as rustc_type_ir::CollectAndApply<rustc_middle::ty::subst::GenericArg, &rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>>>::collect_and_apply::<core::iter::adapters::map::Map<core::ops::range::Range<usize>, <&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode::{closure#0}>, <rustc_middle::ty::context::TyCtxt>::mk_substs_from_iter<core::iter::adapters::map::Map<core::ops::range::Range<usize>, <&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode::{closure#0}>, rustc_middle::ty::subst::GenericArg>::{closure#0}>
 3,621,904  ???:<&mut <&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode::{closure#0} as core::ops::function::FnOnce<(usize,)>>::call_once
 3,277,727  ???:<rustc_middle::ty::subst::GenericArg as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
-2,058,835  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, rustc_middle::infer::canonical::Canonical<rustc_middle::ty::typeck_results::UserType>, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 1,948,697  ???:<rustc_middle::infer::canonical::Canonical<rustc_middle::ty::typeck_results::UserType> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 1,848,926  ???:<&rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 1,676,905  ???:<rustc_hir::hir::QPath as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable
-1,612,138  ???:<rustc_middle::ty::Ty as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
-1,002,741  ???:<rustc_hir::hir::TyKind as rustc_data_structures::stable_hasher::HashStable<rustc_query_system::ich::hcx::StableHashingContext>>::hash_stable

I think the large improvements on the other scenarios justify these small (and possibly ephemeral) regressions.

r? @cjgillot

rustbot · 2023-03-06T23:01:07Z

Could not assign reviewer from: cjgillot.
User(s) cjgillot are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

cjgillot

Thanks. That's some great savings. r=me with a comments on a few places.

compiler/rustc_ast_lowering/src/lib.rs

compiler/rustc_middle/src/hir/map/mod.rs

compiler/rustc_ast_lowering/src/lib.rs

compiler/rustc_middle/src/hir/mod.rs

The crate hash is needed: - if debug assertions are enabled, or - if incr. comp. is enabled, or - if metadata is being generated, or - if `-C instrumentation-coverage` is enabled. This commit avoids computing the crate hash when these conditions are all false, such as when doing a release build of a binary crate. It uses `Option` to store the hashes when needed, rather than computing them on demand, because some of them are needed in multiple places and computing them on demand would make compilation slower. The commit also removes `Owner::hash_without_bodies`. There is no benefit to pre-computing that one, it can just be done in the normal fashion.

nnethercote · 2023-03-07T23:00:12Z

I have addressed the review comments.

@bors r=cjgillot

bors · 2023-03-07T23:00:14Z

📌 Commit 9570023 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-03-08T15:36:31Z

⌛ Testing commit 9570023 with merge 1a064760eee9b43c0645e23f647f87cd6ddee83d...

bors · 2023-03-08T19:51:31Z

💥 Test timed out

matthiaskrgr · 2023-03-08T20:30:57Z

@bors retry timeout

rust-log-analyzer · 2023-03-09T20:04:05Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-03-12T06:48:33Z

⌛ Testing commit 9570023 with merge 150cb38...

bors · 2023-03-12T09:44:10Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 150cb38 to master...

rust-timer · 2023-03-12T11:40:09Z

Finished benchmarking commit (150cb38): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.8%	[-2.9%, -0.2%]	68
Improvements ✅ (secondary)	-2.0%	[-11.3%, -0.2%]	53
All ❌✅ (primary)	-0.8%	[-2.9%, -0.2%]	68

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.4%	[-3.4%, -3.4%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.8%	[-2.0%, -1.7%]	2
Improvements ✅ (secondary)	-5.0%	[-11.1%, -1.6%]	10
All ❌✅ (primary)	-1.8%	[-2.0%, -1.7%]	2

rustbot assigned cjgillot Mar 6, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2023

nnethercote force-pushed the avoid-unnecessary-hashing branch from 97166f5 to b238d9c Compare March 6, 2023 01:43

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 6, 2023

nnethercote force-pushed the avoid-unnecessary-hashing branch from b238d9c to 8953bf3 Compare March 6, 2023 08:00

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023

bugadani reviewed Mar 6, 2023

View reviewed changes

nnethercote force-pushed the avoid-unnecessary-hashing branch from 8953bf3 to ddc32a6 Compare March 6, 2023 19:30

nnethercote force-pushed the avoid-unnecessary-hashing branch from ddc32a6 to 5a8afdd Compare March 6, 2023 22:52

cjgillot reviewed Mar 7, 2023

View reviewed changes

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

compiler/rustc_middle/src/hir/map/mod.rs Outdated Show resolved Hide resolved

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

compiler/rustc_middle/src/hir/mod.rs Outdated Show resolved Hide resolved

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2023

nnethercote force-pushed the avoid-unnecessary-hashing branch from 5a8afdd to 9570023 Compare March 7, 2023 22:59

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 8, 2023

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 Mar 8, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 12, 2023

bors merged commit 150cb38 into rust-lang:master Mar 12, 2023

rustbot added this to the 1.70.0 milestone Mar 12, 2023

rustbot removed the perf-regression Performance regression. label Mar 12, 2023

nnethercote deleted the avoid-unnecessary-hashing branch March 13, 2023 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary hashing #108794

Avoid unnecessary hashing #108794

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

nnethercote commented Mar 6, 2023

rustbot commented Mar 6, 2023

nnethercote commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

bugadani Mar 6, 2023

nnethercote Mar 6, 2023 •

edited

Loading

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

rustbot commented Mar 6, 2023

cjgillot left a comment

nnethercote commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

matthiaskrgr commented Mar 8, 2023

rust-log-analyzer commented Mar 9, 2023

bors commented Mar 12, 2023

bors commented Mar 12, 2023

rust-timer commented Mar 12, 2023

Avoid unnecessary hashing #108794

Avoid unnecessary hashing #108794

Conversation

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Mar 6, 2023

rustbot commented Mar 6, 2023

nnethercote commented Mar 6, 2023

This comment has been minimized.

bors commented Mar 6, 2023

bors commented Mar 6, 2023

This comment has been minimized.

rust-timer commented Mar 6, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

nnethercote Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

nnethercote commented Mar 6, 2023

rustbot commented Mar 6, 2023

cjgillot left a comment

Choose a reason for hiding this comment

nnethercote commented Mar 7, 2023

bors commented Mar 7, 2023

bors commented Mar 8, 2023

bors commented Mar 8, 2023

matthiaskrgr commented Mar 8, 2023

rust-log-analyzer commented Mar 9, 2023

bors commented Mar 12, 2023

bors commented Mar 12, 2023

rust-timer commented Mar 12, 2023

Overall result: ✅ improvements - no action needed

nnethercote Mar 6, 2023 •

edited

Loading