Stabilize the size of incr comp object file names #123441

saethlin · 2024-04-04T01:12:59Z

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: #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:

rust/compiler/rustc_monomorphize/src/partitioning.rs

Lines 445 to 448 in ca7d34e

    
           // If we don't require CGU names to be human-readable, 
        
           // we use a fixed length hash of the composite CGU name 
        
           // instead. 
        
           let new_cgu_name = CodegenUnit::mangle_name(new_cgu_name);

rustbot · 2024-04-04T01:13:07Z

r? @Nadrieril

rustbot has assigned @Nadrieril.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

saethlin · 2024-04-04T03:01:57Z

Might as well
@bors try @rust-timer queue

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

bors · 2024-04-04T03:03:08Z

⌛ Trying commit a6397f0 with merge 0912e5f...

scottmcm · 2024-04-04T03:42:24Z

compiler/rustc_data_structures/src/base_n.rs

+}
+
+pub fn push_case_insensitive<N: Base36Encodable>(n: N, output: &mut String) {
+    // SAFETY: We will only append ASCII bytes.


This of course makes me wonder if there's a way it could be ascii::Char-based to avoid the unsafe, but there's no obvious way to &mut String -> ‽ Vec<ascii::Char.

Could encoded_len be a const instead of a fn? Then this could build into a stack buffer and then append that to output, which might allow removing all the unsafe. Come to think of it, I think that'd just be output.extend([ascii::Char::Null; N::ENCODED_LEN].as_str());...

(But this is very minor unsafe, so it doesn't bother me that much.)

I didn't do that because I don't know what to do with this diagnostic:

error: unconstrained generic constant --> compiler/rustc_data_structures/src/base_n.rs:76:25 | 76 | let mut buf = [0u8; N::LEN]; | ^^^^^^ | = help: try adding a `where` bound using this expression: `where [(); N::LEN]:`

Being able to use a stack buffer here would be very elegant because then I could justify to myself the design of writing this as a Display helper that does a single write_str call, and that would integrate very nicely with how this formatting is used.

Oh, bleh. I forgot that still doesn't work :(

I guess you could bypass that with

trait Base36Encodable { type StackBuffer: Copy + AsMut<[ascii::Char]>; }

or something, but I don't know if it'd be worth it.

Hm. I'll try that.

bors · 2024-04-04T04:34:25Z

☀️ Try build successful - checks-actions
Build commit: 0912e5f (0912e5ff9202f65c525935691abcd0e4896dbc04)

Nadrieril · 2024-04-04T06:57:55Z

r? compiler

rust-timer · 2024-04-04T06:58:16Z

Finished benchmarking commit (0912e5f): comparison URL.

Overall result: ❌ regressions - 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.3%	[0.3%, 0.3%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
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)	-	-	0
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)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	-	-	0

Binary size

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.1%	[0.0%, 0.5%]	58
Regressions ❌ (secondary)	0.2%	[0.0%, 0.6%]	25
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.5%]	58

Bootstrap: 670.495s -> 667.911s (-0.39%)
Artifact size: 317.99 MiB -> 318.00 MiB (0.00%)

compiler/rustc_data_structures/src/base_n.rs

compiler/rustc_incremental/src/persist/fs.rs

rustbot · 2024-04-05T01:16:58Z

Some changes occurred in compiler/rustc_symbol_mangling/src/typeid

cc @rust-lang/project-exploit-mitigations, @rcvalle

rustbot · 2024-04-05T03:36:46Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

bors · 2024-04-09T08:01:16Z

☔ The latest upstream changes (presumably #123663) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-04-15T19:56:42Z

☔ The latest upstream changes (presumably #123968) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2024-04-22T06:47:53Z

☔ The latest upstream changes (presumably #124112) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot · 2024-04-22T14:52:32Z

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

oli-obk · 2024-05-02T21:42:26Z

Sorry for losing track of this. Looks correct to me

@bors r+

bors · 2024-05-02T21:42:28Z

📌 Commit 6ee3713 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-05-02T23:18:30Z

⌛ Testing commit 6ee3713 with merge 293868a...

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

rust-log-analyzer · 2024-05-03T00:02:12Z

The job aarch64-gnu failed! Check out the build log: (web) (plain)

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

[RUSTC-TIMING] serde test:false 7.680
[RUSTC-TIMING] bincode test:false 1.205
[RUSTC-TIMING] serde_json test:false 3.171
[RUSTC-TIMING] rustdoc_json_types test:false 4.306
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.

Session terminated, killing shell...::group::Clock drift check
  network time:  ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

bors · 2024-05-03T00:02:50Z

💔 Test failed - checks-actions

saethlin · 2024-05-03T00:16:35Z

@bors retry

bors · 2024-05-03T17:41:52Z

⌛ Testing commit 6ee3713 with merge 0d7b2fb...

bors · 2024-05-03T19:44:56Z

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

rust-timer · 2024-05-03T21:00:31Z

Finished benchmarking commit (0d7b2fb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Cycles

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

Binary size

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.1%	[0.0%, 0.6%]	48
Regressions ❌ (secondary)	0.1%	[0.0%, 0.6%]	29
Improvements ✅ (primary)	-0.1%	[-0.4%, -0.0%]	13
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.4%, 0.6%]	61

Bootstrap: 674.79s -> 675.47s (0.10%)
Artifact size: 315.91 MiB -> 315.93 MiB (0.01%)

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

rustbot assigned Nadrieril Apr 4, 2024

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 Apr 4, 2024

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 4, 2024

scottmcm reviewed Apr 4, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot assigned oli-obk and unassigned Nadrieril Apr 4, 2024

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 4, 2024

klensy reviewed Apr 4, 2024

View reviewed changes

compiler/rustc_data_structures/src/base_n.rs Outdated Show resolved Hide resolved

klensy reviewed Apr 4, 2024

View reviewed changes

compiler/rustc_incremental/src/persist/fs.rs Outdated Show resolved Hide resolved

saethlin force-pushed the fixed-len-file-names branch from a6397f0 to 2de1832 Compare April 5, 2024 01:16

rustbot added the PG-exploit-mitigations Project group: Exploit mitigations label Apr 5, 2024

This comment has been minimized.

Sign in to view

saethlin force-pushed the fixed-len-file-names branch from 2de1832 to 91b20bc Compare April 5, 2024 03:36

saethlin force-pushed the fixed-len-file-names branch from 91b20bc to 6f4a901 Compare April 9, 2024 15:06

saethlin mentioned this pull request Apr 15, 2024

Remove magic constants when using base_n. #123922

Merged

saethlin force-pushed the fixed-len-file-names branch from 6f4a901 to 8d28a57 Compare April 15, 2024 20:33

Stabilize the size of incr comp object file names

6ee3713

saethlin force-pushed the fixed-len-file-names branch from 8d28a57 to 6ee3713 Compare April 22, 2024 14:52

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 May 2, 2024

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 May 3, 2024

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 May 3, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 3, 2024

bors merged commit 0d7b2fb into rust-lang:master May 3, 2024
13 checks passed

rustbot added this to the 1.80.0 milestone May 3, 2024

saethlin deleted the fixed-len-file-names branch May 3, 2024 22:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stabilize the size of incr comp object file names #123441

Stabilize the size of incr comp object file names #123441

saethlin commented Apr 4, 2024 •

edited

Loading

rustbot commented Apr 4, 2024

saethlin commented Apr 4, 2024

This comment has been minimized.

bors commented Apr 4, 2024

scottmcm Apr 4, 2024

saethlin Apr 4, 2024

saethlin Apr 4, 2024

scottmcm Apr 4, 2024

scottmcm Apr 4, 2024

saethlin Apr 4, 2024

bors commented Apr 4, 2024

This comment has been minimized.

Nadrieril commented Apr 4, 2024

rust-timer commented Apr 4, 2024

rustbot commented Apr 5, 2024

This comment has been minimized.

rustbot commented Apr 5, 2024

bors commented Apr 9, 2024

bors commented Apr 15, 2024

bors commented Apr 22, 2024

rustbot commented Apr 22, 2024

oli-obk commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

rust-log-analyzer commented May 3, 2024

bors commented May 3, 2024

saethlin commented May 3, 2024

bors commented May 3, 2024

bors commented May 3, 2024

rust-timer commented May 3, 2024

	// If we don't require CGU names to be human-readable,
	// we use a fixed length hash of the composite CGU name
	// instead.
	let new_cgu_name = CodegenUnit::mangle_name(new_cgu_name);

Stabilize the size of incr comp object file names #123441

Stabilize the size of incr comp object file names #123441

Conversation

saethlin commented Apr 4, 2024 • edited Loading

rustbot commented Apr 4, 2024

saethlin commented Apr 4, 2024

This comment has been minimized.

bors commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Apr 4, 2024

This comment has been minimized.

Nadrieril commented Apr 4, 2024

rust-timer commented Apr 4, 2024

Overall result: ❌ regressions - no action needed

rustbot commented Apr 5, 2024

This comment has been minimized.

rustbot commented Apr 5, 2024

bors commented Apr 9, 2024

bors commented Apr 15, 2024

bors commented Apr 22, 2024

rustbot commented Apr 22, 2024

oli-obk commented May 2, 2024

bors commented May 2, 2024

bors commented May 2, 2024

rust-log-analyzer commented May 3, 2024

bors commented May 3, 2024

saethlin commented May 3, 2024

bors commented May 3, 2024

bors commented May 3, 2024

rust-timer commented May 3, 2024

Overall result: no relevant changes - no action needed

saethlin commented Apr 4, 2024 •

edited

Loading