Remove rt::init allocation for thread name by GnomedDev · Pull Request #123433 ·...

r? @jhpratt

rustbot has assigned @jhpratt.
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

I'll await input from @joboet given the preceding comment, but r=me otherwise.

@GnomedDev Out of curiosity, what is the size of a helloworld binary after this change?

@bors try @rust-timer queue

We can find out :)

⌛ Trying commit 900634e with merge 866281d...

☀️ Try build successful - checks-actions
Build commit: 866281d (866281ddb889918640c4e64d01fab3902d6fd245)

For an ./x build library --stage 1, this PR seems to make a hello world in release go from 570448 bytes to 570904 bytes and in debug it goes from 579856 to 580320. This is just from a default cargo new --bin project, not using any extra profile flags.

Finished benchmarking commit (866281d): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

Results

Binary size

Results

Bootstrap: 667.94s -> 668.279s (0.05%)
Artifact size: 318.08 MiB -> 318.08 MiB (0.00%)

Looks like binary size for a (size-)optimized helloworld was reduced by ~1%.

I could change this PR to instead store an enum, do you want me to try this here or just merge this in and try as a follow-up?

enum ThreadName {
		Main,
		Other(CString)
}

impl ThreadName {
		fn as_cstr(&self) -> &CStr {
				match self {
						Self::Main => c"main",
						Self::Other(other) => &*other
				}
		}
}

I'd say this more like change in std::thread::Thread implementation, which allows to remove unwrap.

@klensy I'm sorry, I can't understand that? Can you reword it?

@klensy I'm sorry, I can't understand that? Can you reword it?

You're changed implementation of stdlib's Thread to support that one specific use case, but degrading all others (bigger Inner size?). Probably libs members should review this.

Inner is bigger by 8 bytes as it seems like Cow<CStr> does not niche like Cow<str>. I can get it back down via the enum approach mentioned above.

Other than that, it would cost one extra branch during code that is accessing the thread name, which is documented to mostly be for panic handling and other diagnostics... not hot code like startup that every program has to pay for.

Getting the name of a thread is really not on the hot path, and those 8 bytes are totally insignificant given the size of a stack.

The question here is whether this added complexity is worth it. The binary size improvements seem to suggest as much, and I feel like this could improve some other things too, so I'm going to accept this.

Please do use the enum approach though and add another case: Unnamed. Then we can get rid of the Option around the name, which should make the code a bit cleaner.

@rustbot author

@joboet Thanks for the review! I've just pushed the enum approach so we should probably run another timer if the queue isn't overflowing.

@rustbot review

⌛ Trying commit 0989416 with merge d9d26f0...

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

Finished benchmarking commit (d9d26f0): comparison URL.

Overall result: no relevant changes - 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 benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

Cycles

Results

Binary size

Results

Bootstrap: 670.029s -> 668.109s (-0.29%)
Artifact size: 318.08 MiB -> 318.04 MiB (-0.01%)

Much fewer tiny regressions in binary size. Interesting.

Thanks!

@bors r+

📌 Commit 0989416 has been approved by joboet

It is now in the queue for this repository.

⌛ Testing commit 0989416 with merge 30840c5...

☀️ Test successful - checks-actions
Approved by: joboet
Pushing 30840c5 to master...

Finished benchmarking commit (30840c5): 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

Cycles

Results

Binary size

Results

Bootstrap: 668.929s -> 669.502s (0.09%)
Artifact size: 318.19 MiB -> 318.19 MiB (-0.00%)