perf: Don't hash the full (byte) string for Symbol interner if it's long

[panstromek]

When investigating include-blob bimodailty, I noticed that we hash the whole included blob (31MB) and every other string that we intern no matter the length. This intuitively seems a bit wasteful - simply speaking, I can't imagine that after 15MB of hashing, we get a meaningful hash quality improvement with 15MB more. This is in the spirit of the comment in rustc-hash readme:

Spending more CPU cycles on a higher quality hash does not reduce hash collisions enough to make the compiler faster on real-world benchmarks.

This PR changes the code to only hash the first N bytes (more on N later). The assumption (and intuition) behind this is:

• we assume most interned strings are short (idents, literals), so most strings are not affected
• we assume that we get enough entropy in the first N bytes (+length)
  • In other words, most interned strings have distinct len and distinct first N bytes
• If we get a collision, the algorithm needs to compare the full length string anyway
  • If the strings have distinct len, the eq check is very cheap, so it doesn't make that much sense to guard against that collision by computing very expensive hash of the whole string.
  • Even if the len is the same, we can probably assume that the unequal byte occurs pretty early anyway, so similar argument applies

About the specific N, I tried to measure a few options in this PR, mostly picked by intuition

• 32 bytes - perf: Don't hash the full (byte) string for Symbol interner if it's long #162529 (comment)
  • include-blob improvements, some regressions, not sure if relevant
• 16 bytes -perf: Don't hash the full (byte) string for Symbol interner if it's long #162529 (comment)
  • there's a distinct path for <=16 bytes in hash_bytes in FxHasher, so I wanted to try whether it makes a difference if we always use only that code path (and LLVM could optimize the other branch out)
  • This looks worse, 16 bytes is a pretty low threshold, this will hit a lot of strings
• 64 bytes - perf: Don't hash the full (byte) string for Symbol interner if it's long #162529 (comment)
  • This is a cache line size, so a natural number to try. Looks best so far
  • Non-relevant results are tilted slightly red, but that might as well be just noise

I'm still not 100% confident about picking the number, though, because I'm not sure we have crates in the benchmark suite that would stress the Symbol interner a lot, so I see a few paths to go from here:

• just merge with 64 limit and hope for the best 🤞

• pick a fairly high "safe choice" (e.g. 1kb) for now

  • this will still improve include-blob, but it should rarely affect anything else

• find some crates that stress this code a lot and measure on them

  • probably crates that are big or contain a lot of somewhat longer symbols (long identifiers, literals, etc.)
  • any ideas welcome :)

r? nnethercote

I want to know what you think, because you worked on this a lot, or if you have recommendations for crates to test this on. I'll squash and clean it up based on what we decide to do.

View all comments

[panstromek]

@bors try @rust-timer queue

[panstromek]

@bors try cancel

This will not work, I didn't realize end index has to be inbounds

[rust-bors]

Try build cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/34349214909

Hint: if you want to run another try build, you do not need to manually cancel the previous one. Just run @bors try and bors will cancel the previous build automatically.

[panstromek]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 2ed7127 (2ed7127393437b9bd7c067adfd70f7b716e875b1)
Base parent: eca445e (eca445e5ae4a6679cc27d3a09106ce245e13a5a6)

[rust-timer]

Finished benchmarking commit (2ed7127): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.3%	[0.2%, 0.3%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-8.8%, -0.8%]	14
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary 1.3%, secondary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.3%	[0.5%, 2.7%]	3
Regressions ❌ (secondary)	2.5%	[2.1%, 2.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.3%	[0.5%, 2.7%]	3

Cycles

Results (primary -0.4%, secondary -1.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.5%, 0.8%]	4
Regressions ❌ (secondary)	4.3%	[4.1%, 4.4%]	2
Improvements ✅ (primary)	-0.7%	[-2.5%, -0.4%]	11
Improvements ✅ (secondary)	-3.7%	[-5.2%, -2.2%]	7
All ❌✅ (primary)	-0.4%	[-2.5%, 0.8%]	15

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.1%]	40
Regressions ❌ (secondary)	0.1%	[0.0%, 0.1%]	7
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	4
Improvements ✅ (secondary)	-0.0%	[-0.0%, -0.0%]	1
All ❌✅ (primary)	0.0%	[-0.0%, 0.1%]	44

Bootstrap: 481.923s -> 485.241s (0.69%)
Artifact size: 403.28 MiB -> 403.35 MiB (0.02%)

[panstromek]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: c0c7b04 (c0c7b0466beb1d9185dd31666d5a776651d61e53)
Base parent: 88acefd (88acefdf6c6b4ddaab64e27299c3b191b4ffdc32)

[rust-timer]

Finished benchmarking commit (c0c7b04): 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	1.5%	[0.3%, 3.0%]	17
Regressions ❌ (secondary)	1.1%	[0.1%, 4.3%]	61
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-8.8%, -0.8%]	14
All ❌✅ (primary)	1.5%	[0.3%, 3.0%]	17

Max RSS (memory usage)

Results (primary -0.4%, secondary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.5%	[0.5%, 0.5%]	1
Regressions ❌ (secondary)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-0.5%	[-0.8%, -0.4%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.8%, 0.5%]	6

Cycles

Results (primary -0.3%, secondary -0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.0%	[0.4%, 2.1%]	3
Regressions ❌ (secondary)	3.2%	[2.2%, 4.1%]	8
Improvements ✅ (primary)	-0.8%	[-2.3%, -0.4%]	8
Improvements ✅ (secondary)	-3.7%	[-5.3%, -2.0%]	7
All ❌✅ (primary)	-0.3%	[-2.3%, 2.1%]	11

Binary size

Results (primary 0.0%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

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

Bootstrap: 480.225s -> 481.543s (0.27%)
Artifact size: 403.27 MiB -> 403.37 MiB (0.02%)

[nnethercote]

Heh, I tried the same idea in Firefox during the Obama administration. I was looking at the first 1,000 bytes (16-64 bytes feels way too small) and in the end I decided against it because it felt not quite right and pathological inputs could lead to performance disaster. In the rustc context it also feels like it's specializing too much for a single unusual benchmark.

[panstromek]

during the Obama administration

😄 interesting choice of timestamp format

I'll add for context that one motivation at the moment is that this also drastically reduces include-blob noise, because that comes from the hashing function optimization flipping between two states. We should probably find a way to address that one "properly" but that might take some time.

[panstromek]

Let's try 128. I want to have another CI build for stress testing, and 128 is another intersting boundary because it should be enough to contain most doc comments (doc comments go over 64 bytes fairly often).

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 9eafee5 (9eafee5e7352597d6b8db4526fcc202f3217701d)
Base parent: 74b3f08 (74b3f086e27d20f04611a52addc3781576f3c749)

[rust-timer]

Finished benchmarking commit (9eafee5): 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 @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-8.8%, -0.8%]	14
All ❌✅ (primary)	0.2%	[0.2%, 0.2%]	1

Max RSS (memory usage)

Results (primary -1.1%, secondary 0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[3.3%, 4.9%]	3
Improvements ✅ (primary)	-1.1%	[-2.1%, -0.5%]	6
Improvements ✅ (secondary)	-2.5%	[-3.2%, -2.0%]	4
All ❌✅ (primary)	-1.1%	[-2.1%, -0.5%]	6

Cycles

Results (primary 0.5%, secondary -4.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.4%, 1.7%]	14
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-1.3%, -0.4%]	2
Improvements ✅ (secondary)	-4.0%	[-5.5%, -2.2%]	6
All ❌✅ (primary)	0.5%	[-1.3%, 1.7%]	16

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 493.042s -> 493.985s (0.19%)
Artifact size: 406.92 MiB -> 406.94 MiB (0.01%)

[panstromek]

I'm a bit suspicious of the regressions in the run above. They look related to symbol interning, but something feels off to me.

Anyway, let's try the (assumed) safe limit of 1kb.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d045a7d (d045a7d68b92d85610841f64f6a4037fc13e1eab)
Base parent: adcc5f3 (adcc5f3bdc1a3c205141996ba01404b6e4b27310)

[rust-timer]

Finished benchmarking commit (d045a7d): comparison URL.

Overall result: ✅ improvements - no action needed

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.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.3%	[-8.8%, -0.7%]	14
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-0.6%, -0.4%]	2

Cycles

Results (primary -0.2%, secondary -3.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.5%, 0.9%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.9%	[-2.1%, -0.4%]	7
Improvements ✅ (secondary)	-3.6%	[-5.6%, -2.4%]	8
All ❌✅ (primary)	-0.2%	[-2.1%, 0.9%]	12

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 493.58s -> 494.498s (0.19%)
Artifact size: 406.93 MiB -> 407.61 MiB (0.17%)

[nnethercote]

Can you try generating a pathological case and see how badly it performs?

Also, I wonder if hashing the first 1k and last 1k is a good idea. Seems like if there are lots of long almost equal strings that the distinguishing characters might be at the end.

[panstromek]

Can you try generating a pathological case and see how badly it performs?

Yes, it's on my todo list. I want to get an idea how it behaves and I haven't found a notable change in crates I've tried so far. It looks like crates that have a lot of symbols also have a ton of other stuff going on, so any change in Symbol interning is barely visible.