Encode hashes as bytes, not varint #110083

saethlin · 2023-04-08T12:46:16Z

In a few places, we store hashes as u64 or u128 and then apply derive(Decodable, Encodable) to the enclosing struct/enum. It is more efficient to encode hashes directly than try to apply some varint encoding. This PR adds two new types Hash64 and Hash128 which are produced by StableHasher and replace every use of storing a u64 or u128 that represents a hash.

Distribution of the byte lengths of leb128 encodings, from x build --stage 2 with incremental = true

Before:

(  1) 373418203 (53.7%, 53.7%): 1
(  2) 196240113 (28.2%, 81.9%): 3
(  3) 108157958 (15.6%, 97.5%): 2
(  4)  17213120 ( 2.5%, 99.9%): 4
(  5)    223614 ( 0.0%,100.0%): 9
(  6)    216262 ( 0.0%,100.0%): 10
(  7)     15447 ( 0.0%,100.0%): 5
(  8)      3633 ( 0.0%,100.0%): 19
(  9)      3030 ( 0.0%,100.0%): 8
( 10)      1167 ( 0.0%,100.0%): 18
( 11)      1032 ( 0.0%,100.0%): 7
( 12)      1003 ( 0.0%,100.0%): 6
( 13)        10 ( 0.0%,100.0%): 16
( 14)        10 ( 0.0%,100.0%): 17
( 15)         5 ( 0.0%,100.0%): 12
( 16)         4 ( 0.0%,100.0%): 14

After:

(  1) 372939136 (53.7%, 53.7%): 1
(  2) 196240140 (28.3%, 82.0%): 3
(  3) 108014969 (15.6%, 97.5%): 2
(  4)  17192375 ( 2.5%,100.0%): 4
(  5)       435 ( 0.0%,100.0%): 5
(  6)        83 ( 0.0%,100.0%): 18
(  7)        79 ( 0.0%,100.0%): 10
(  8)        50 ( 0.0%,100.0%): 9
(  9)         6 ( 0.0%,100.0%): 19

The remaining 9 or 10 and 18 or 19 are u64 and u128 respectively that have the high bits set. As far as I can tell these are coming primarily from SwitchTargets.

saethlin · 2023-04-08T12:46:49Z

@bors try @rust-timer queue

bors · 2023-04-08T12:46:58Z

⌛ Trying commit 8627fe1a80c6f8cabc961d8ca7df3d05a61e956a with merge 6de33b85fc2e7dfa93da079f1e2e5b09b9fd618b...

bors · 2023-04-08T14:33:32Z

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

nbdd0121 · 2023-04-08T15:32:14Z

I don't think hash should be defined as bytes -- doing so reduces alignment and can be detrimental (especially on platforms that don't have efficient misaligned memory access). Also on 32-bit platforms, they now have to be passed via memory.

If this is only about encoding then a new type with custom Encodable and Decodable should be a better solution?

rust-timer · 2023-04-08T16:11:23Z

Finished benchmarking commit (6de33b85fc2e7dfa93da079f1e2e5b09b9fd618b): comparison URL.

Overall result: ✅ improvements - no 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.

@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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.5%	[-1.6%, -1.4%]	3
All ❌✅ (primary)	-	-	0

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)	-2.3%	[-2.3%, -2.3%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

saethlin · 2023-04-08T16:46:43Z

Cachegrind diff for the one benchmark with significant improvement says:

-54,785,914  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir           file:function
--------------------------------------------------------------------------------
-36,609,432  ???:<alloc::vec::Vec<rustc_middle::mir::syntax::Operand> as rustc_serialize::serialize::Decodable<rustc_query_impl::on_disk_cache::CacheDecoder>>::decode
-18,469,278  ???:<alloc::boxed::Box<rustc_middle::mir::Constant> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
    131,063  ???:<[rustc_middle::mir::LocalDecl] as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
    129,569  ???:<(rustc_middle::mir::syntax::Place, rustc_middle::mir::syntax::Rvalue) as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode

Which is not what I would have predicted. Interesting.

cjgillot

Which is not what I would have predicted. Interesting.

Maybe the hashes for type-ids? u*::MAX constants?

compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

compiler/rustc_span/src/def_id.rs

compiler/rustc_query_system/src/ich/impls_syntax.rs

saethlin · 2023-04-13T00:50:24Z

I've touched a lot more code now...
@bors try @rust-timer queue

bors · 2023-04-13T00:50:35Z

⌛ Trying commit 5118a860452ba5c8d48159ddaa18febcae879b37 with merge 4dc92c4e8ab0d1196d7c7b485a13ce4cbe848b4b...

saethlin · 2023-04-14T01:08:23Z

Once more, with feeling
@bors try @rust-timer queue

bors · 2023-04-14T01:08:31Z

⌛ Trying commit 16ada86621692739ee72861fb9c15043f1d27768 with merge e89b3eb207a1843d10eccc438536aeca22752ecf...

bors · 2023-04-14T02:52:16Z

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

saethlin · 2023-04-18T14:57:14Z

Rebased over #110343 to simplify the distributions in the PR description.

cjgillot

Would a few inlines be helpful?
r=me either way

compiler/rustc_span/src/def_id.rs

compiler/rustc_data_structures/src/fingerprint.rs

compiler/rustc_data_structures/src/hashes.rs

Co-authored-by: Camille Gillot <gillot.camille@gmail.com>

cjgillot · 2023-04-18T20:47:10Z

@bors r+

bors · 2023-04-18T20:47:12Z

📌 Commit 073d99b has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-04-18T22:27:18Z

⌛ Testing commit 073d99b with merge b3f1379...

bors · 2023-04-19T00:39:20Z

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

rust-timer · 2023-04-19T01:57:01Z

Finished benchmarking commit (b3f1379): comparison URL.

Overall result: ❌✅ regressions and 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.9%	[0.9%, 0.9%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.2%	[-1.3%, -1.1%]	3
All ❌✅ (primary)	0.9%	[0.9%, 0.9%]	1

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)	3.1%	[2.1%, 4.2%]	2
Improvements ✅ (primary)	-3.4%	[-3.4%, -3.4%]	1
Improvements ✅ (secondary)	-3.2%	[-3.2%, -3.2%]	1
All ❌✅ (primary)	-3.4%	[-3.4%, -3.4%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

…workingjubilee Replace some u64 hashes with Hash64 I introduced the Hash64 and Hash128 types in rust-lang#110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64. This reverts part of rust-lang#117603, which turned an encoded Hash64 into a u64. Based on rust-lang#110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.

Rollup merge of rust-lang#137095 - saethlin:use-hash64-for-hashes, r=workingjubilee Replace some u64 hashes with Hash64 I introduced the Hash64 and Hash128 types in rust-lang#110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64. This reverts part of rust-lang#117603, which turned an encoded Hash64 into a u64. Based on rust-lang#110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.

…bilee Replace some u64 hashes with Hash64 I introduced the Hash64 and Hash128 types in rust-lang/rust#110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64. This reverts part of rust-lang/rust#117603, which turned an encoded Hash64 into a u64. Based on rust-lang/rust#110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.