Remove some suspicious cast truncations #110367

saethlin · 2023-04-15T17:54:44Z

These truncations were added a long time ago, and as best I can tell without a perf justification. And with #110410 it has become perf-neutral to not truncate anymore. We worked hard for all these bits, let's use them.

saethlin · 2023-04-15T17:59:22Z

@bors try @rust-timer queue

bors · 2023-04-15T17:59:30Z

⌛ Trying commit cbcf83ed485ee20d6e1ac94c696ed0a95fb59b5e with merge 20130e508d1a1e7a7110aec406d3b0499a14afd4...

bors · 2023-04-15T19:43:20Z

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

rust-timer · 2023-04-16T06:37:03Z

Finished benchmarking commit (20130e508d1a1e7a7110aec406d3b0499a14afd4): comparison URL.

Overall result: ❌ regressions - 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.6%]	46
Regressions ❌ (secondary)	0.4%	[0.2%, 0.9%]	17
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.2%, 0.6%]	46

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

Cycles

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

saethlin · 2023-04-16T13:24:59Z

@bors try @rust-timer queue

bors · 2023-04-16T13:25:09Z

⌛ Trying commit 2cdd20a94b80bc0046eefb45429d45405cc3a33e with merge 71b78b360fb374a9b98c3d8a54b40fc749f1f236...

bors · 2023-04-16T15:06:47Z

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

rust-timer · 2023-04-16T18:13:42Z

Finished benchmarking commit (71b78b360fb374a9b98c3d8a54b40fc749f1f236): 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.3%	[-0.3%, -0.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.3%, -0.3%]	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)	2.2%	[2.2%, 2.2%]	1
Regressions ❌ (secondary)	2.7%	[2.7%, 2.7%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.2%	[2.2%, 2.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)	1.5%	[1.5%, 1.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.6%	[-2.6%, -2.6%]	1
All ❌✅ (primary)	1.5%	[1.5%, 1.5%]	1

saethlin · 2023-04-16T20:06:20Z

Bootstrap timings skew into the red but everything else looks insignificant. Looks to me like we can probably use the full hashes in these cases.

saethlin · 2023-04-17T04:03:35Z

That's... Lucky?

oli-obk · 2023-04-17T08:16:50Z

compiler/rustc_data_structures/src/svh.rs

-        H: Hasher,
-    {
-        self.hash.to_le().hash(state);
+        format!("{:016x}", self.hash.to_smaller_hash())


We should probably look into changing this to print the full fingerprint.

oli-obk · 2023-04-17T08:17:50Z

@bors r+

bors · 2023-04-17T08:17:52Z

📌 Commit 84facac has been approved by oli-obk

It is now in the queue for this repository.

bors · 2023-04-17T09:38:34Z

⌛ Testing commit 84facac with merge e49122f...

bors · 2023-04-17T12:00:31Z

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

rust-timer · 2023-04-17T13:18:40Z

Finished benchmarking commit (e49122f): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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
cc @rust-lang/wg-compiler-performance

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%]	52
Regressions ❌ (secondary)	0.5%	[0.3%, 0.8%]	13
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-1.5%, -0.2%]	6
All ❌✅ (primary)	0.4%	[0.2%, 1.1%]	52

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.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.3%	[-3.3%, -3.3%]	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)	-	-	0
Improvements ✅ (secondary)	-3.8%	[-4.2%, -3.5%]	3
All ❌✅ (primary)	-	-	0

oli-obk · 2023-04-17T13:27:04Z

compiler/rustc_span/src/lib.rs

-        // We truncate the stable ID hash and line and column numbers. The chances
-        // of causing a collision this way should be minimal.
-        Hash::hash(&(file.name_hash as u64), hasher);
+        Hash::hash(&file.name_hash, hasher);


It seems like this actually has a perf effect after all. The benchmarked version was merging the two u64s of the stable hash and hashing the resulting u64.

This landed out of order. The PR that makes this not a regression: #110410 is still in queue

Implement StableHasher::write_u128 via write_u64 In rust-lang#110367 (comment) the cachegrind diffs indicate that nearly all the regression is from this: ``` 22,892,558 ???:<rustc_data_structures::sip128::SipHasher128>::slice_write_process_buffer -9,502,262 ???:<rustc_data_structures::sip128::SipHasher128>::short_write_process_buffer::<8> ``` Which happens because the diff for that perf run swaps a `Hash::hash` of a `u64` to a `u128`. But `slice_write_process_buffer` is a `#[cold]` function, and is for handling hashes of arbitrary-length byte arrays. Using the much more optimizer-friendly `u64` path twice to hash a `u128` provides a nice perf boost in some benchmarks.

rylev · 2023-04-19T08:19:24Z

Looks like this was fixed in #110410 so marking as triaged

@rustbot label +perf-regression-triaged

Implement StableHasher::write_u128 via write_u64 In rust-lang/rust#110367 (comment) the cachegrind diffs indicate that nearly all the regression is from this: ``` 22,892,558 ???:<rustc_data_structures::sip128::SipHasher128>::slice_write_process_buffer -9,502,262 ???:<rustc_data_structures::sip128::SipHasher128>::short_write_process_buffer::<8> ``` Which happens because the diff for that perf run swaps a `Hash::hash` of a `u64` to a `u128`. But `slice_write_process_buffer` is a `#[cold]` function, and is for handling hashes of arbitrary-length byte arrays. Using the much more optimizer-friendly `u64` path twice to hash a `u128` provides a nice perf boost in some benchmarks.

…-obk Use the full Fingerprint when stringifying Svh Finally circling back, per rust-lang#110367 (comment) r? `@oli-obk`

Stabilize the size of incr comp object file names The current implementation does not produce stable-length paths, and we create the paths in a way that makes our allocation behavior is nondeterministic. I think `@eddyb` fixed a number of other cases like this in the past, and this PR fixes another one. Whether that actually matters I have no idea, but we still have bimodal behavior in rustc-perf and the non-uniformity in `find` and `ls` was bothering me. I've also removed the truncation of the mangled CGU names. Before this PR incr comp paths look like this: ``` target/debug/incremental/scratch-38izrrq90cex7/s-gux6gz0ow8-1ph76gg-ewe1xj434l26w9up5bedsojpd/261xgo1oqnd90ry5.o ``` And after, they look like this: ``` target/debug/incremental/scratch-035omutqbfkbw/s-gux6borni0-16r3v1j-6n64tmwqzchtgqzwwim5amuga/55v2re42sztc8je9bva6g8ft3.o ``` On the one hand, I'm sure this will break some people's builds because they're on Windows and only a few bytes from the path length limit. But if we're that seriously worried about the length of our file names, I have some other ideas on how to make them smaller. And last time I deleted some hash truncations from the compiler, there was a huge drop in the number if incremental compilation ICEs that were reported: rust-lang#110367

…-obk Stabilize the size of incr comp object file names The current implementation does not produce stable-length paths, and we create the paths in a way that makes our allocation behavior is nondeterministic. I think `@eddyb` fixed a number of other cases like this in the past, and this PR fixes another one. Whether that actually matters I have no idea, but we still have bimodal behavior in rustc-perf and the non-uniformity in `find` and `ls` was bothering me. I've also removed the truncation of the mangled CGU names. Before this PR incr comp paths look like this: ``` target/debug/incremental/scratch-38izrrq90cex7/s-gux6gz0ow8-1ph76gg-ewe1xj434l26w9up5bedsojpd/261xgo1oqnd90ry5.o ``` And after, they look like this: ``` target/debug/incremental/scratch-035omutqbfkbw/s-gux6borni0-16r3v1j-6n64tmwqzchtgqzwwim5amuga/55v2re42sztc8je9bva6g8ft3.o ``` On the one hand, I'm sure this will break some people's builds because they're on Windows and only a few bytes from the path length limit. But if we're that seriously worried about the length of our file names, I have some other ideas on how to make them smaller. And last time I deleted some hash truncations from the compiler, there was a huge drop in the number if incremental compilation ICEs that were reported: rust-lang#110367 --- Upon further reading, this PR actually fixes a bug. This comment says the CGU names are supposed to be a fixed-length hash, and before this PR they aren't: https://github.com/rust-lang/rust/blob/ca7d34efa94afe271accf2bd3d44152a5bd6fff1/compiler/rustc_monomorphize/src/partitioning.rs#L445-L448

Stabilize the size of incr comp object file names The current implementation does not produce stable-length paths, and we create the paths in a way that makes our allocation behavior is nondeterministic. I think `@eddyb` fixed a number of other cases like this in the past, and this PR fixes another one. Whether that actually matters I have no idea, but we still have bimodal behavior in rustc-perf and the non-uniformity in `find` and `ls` was bothering me. I've also removed the truncation of the mangled CGU names. Before this PR incr comp paths look like this: ``` target/debug/incremental/scratch-38izrrq90cex7/s-gux6gz0ow8-1ph76gg-ewe1xj434l26w9up5bedsojpd/261xgo1oqnd90ry5.o ``` And after, they look like this: ``` target/debug/incremental/scratch-035omutqbfkbw/s-gux6borni0-16r3v1j-6n64tmwqzchtgqzwwim5amuga/55v2re42sztc8je9bva6g8ft3.o ``` On the one hand, I'm sure this will break some people's builds because they're on Windows and only a few bytes from the path length limit. But if we're that seriously worried about the length of our file names, I have some other ideas on how to make them smaller. And last time I deleted some hash truncations from the compiler, there was a huge drop in the number if incremental compilation ICEs that were reported: rust-lang/rust#110367 --- Upon further reading, this PR actually fixes a bug. This comment says the CGU names are supposed to be a fixed-length hash, and before this PR they aren't: https://github.com/rust-lang/rust/blob/ca7d34efa94afe271accf2bd3d44152a5bd6fff1/compiler/rustc_monomorphize/src/partitioning.rs#L445-L448

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 Apr 15, 2023

saethlin changed the title ~~Remove some suspicious has truncations~~ Remove some suspicious cast truncations Apr 15, 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 Apr 16, 2023

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 Apr 16, 2023

saethlin mentioned this pull request Apr 16, 2023

Implement StableHasher::write_u128 via write_u64 #110410

Merged

saethlin mentioned this pull request Apr 16, 2023

Encode hashes as bytes, not varint #110083

Merged

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Apr 16, 2023

Remove some unnecessary hash truncations

84facac

saethlin force-pushed the no-truncations branch from 2cdd20a to 84facac Compare April 17, 2023 00:05

oli-obk approved these changes Apr 17, 2023

View reviewed changes

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 Apr 17, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 17, 2023

bors merged commit e49122f into rust-lang:master Apr 17, 2023

rustbot added this to the 1.71.0 milestone Apr 17, 2023

rustbot added the perf-regression Performance regression. label Apr 17, 2023

oli-obk reviewed Apr 17, 2023

View reviewed changes

saethlin deleted the no-truncations branch April 17, 2023 14:20

rustbot added the perf-regression-triaged The performance regression has been triaged. label Apr 19, 2023

saethlin mentioned this pull request Apr 30, 2023

Use the full Fingerprint when stringifying Svh #111024

Merged

saethlin mentioned this pull request Jul 24, 2023

Stable crate hash depends on host target #113990

Open

saethlin mentioned this pull request Apr 4, 2024

Stabilize the size of incr comp object file names #123441

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some suspicious cast truncations #110367

Remove some suspicious cast truncations #110367

saethlin commented Apr 15, 2023 •

edited

Loading

saethlin commented Apr 15, 2023

This comment has been minimized.

bors commented Apr 15, 2023

bors commented Apr 15, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

saethlin commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

bors commented Apr 16, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

saethlin commented Apr 16, 2023

saethlin commented Apr 17, 2023

oli-obk Apr 17, 2023

oli-obk commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 17, 2023

oli-obk Apr 17, 2023

saethlin Apr 17, 2023

rylev commented Apr 19, 2023

Remove some suspicious cast truncations #110367

Remove some suspicious cast truncations #110367

Conversation

saethlin commented Apr 15, 2023 • edited Loading

saethlin commented Apr 15, 2023

This comment has been minimized.

bors commented Apr 15, 2023

bors commented Apr 15, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

Overall result: ❌ regressions - ACTION NEEDED

saethlin commented Apr 16, 2023

This comment has been minimized.

bors commented Apr 16, 2023

bors commented Apr 16, 2023

This comment has been minimized.

rust-timer commented Apr 16, 2023

Overall result: ✅ improvements - no action needed

saethlin commented Apr 16, 2023

saethlin commented Apr 17, 2023

Choose a reason for hiding this comment

oli-obk commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

bors commented Apr 17, 2023

rust-timer commented Apr 17, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rylev commented Apr 19, 2023

saethlin commented Apr 15, 2023 •

edited

Loading