Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove rt::init allocation for thread name #123433

Merged
merged 1 commit into from Apr 6, 2024

Conversation

GnomedDev
Copy link
Contributor

This removes one of the allocations in a fn main() {} program.

@rustbot
Copy link
Collaborator

rustbot commented Apr 3, 2024

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 3, 2024
@workingjubilee
Copy link
Contributor

cc @joboet who is also messing around with init startup code in

@jhpratt
Copy link
Member

jhpratt commented Apr 4, 2024

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

@workingjubilee
Copy link
Contributor

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

@Kobzol
Copy link
Contributor

Kobzol commented Apr 4, 2024

@bors try @rust-timer queue

We can find out :)

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2024
…=<try>

Remove rt::init allocation for thread name

This removes one of the allocations in a `fn main() {}` program.
@bors
Copy link
Contributor

bors commented Apr 4, 2024

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

@bors
Copy link
Contributor

bors commented Apr 4, 2024

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

@rust-timer

This comment has been minimized.

@GnomedDev
Copy link
Contributor Author

GnomedDev commented Apr 4, 2024

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.

@rust-timer
Copy link
Collaborator

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

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.9% [2.9%, 2.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.0% [-10.2%, -0.2%] 6
Improvements ✅
(secondary)
-1.6% [-1.6%, -1.6%] 1
All ❌✅ (primary) -2.2% [-10.2%, 2.9%] 7

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)
2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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.2%] 15
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-1.1%, -0.1%] 13
Improvements ✅
(secondary)
-1.0% [-1.3%, -0.1%] 38
All ❌✅ (primary) -0.2% [-1.1%, 0.2%] 28

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

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

Kobzol commented Apr 4, 2024

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

@GnomedDev
Copy link
Contributor Author

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

@klensy
Copy link
Contributor

klensy commented Apr 4, 2024

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

@GnomedDev
Copy link
Contributor Author

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

@klensy
Copy link
Contributor

klensy commented Apr 4, 2024

@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.

@GnomedDev
Copy link
Contributor Author

GnomedDev commented Apr 4, 2024

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.

@joboet
Copy link
Contributor

joboet commented Apr 4, 2024

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 joboet assigned joboet and unassigned jhpratt Apr 4, 2024
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 4, 2024
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 4, 2024
@Kobzol
Copy link
Contributor

Kobzol commented Apr 4, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 4, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 4, 2024
…=<try>

Remove rt::init allocation for thread name

This removes one of the allocations in a `fn main() {}` program.
@bors
Copy link
Contributor

bors commented Apr 4, 2024

⌛ Trying commit 0989416 with merge d9d26f0...

@bors
Copy link
Contributor

bors commented Apr 4, 2024

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

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

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

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)
-5.5% [-6.3%, -4.6%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -5.5% [-6.3%, -4.6%] 2

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

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.3% [0.0%, 0.4%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.5% [-1.1%, -0.0%] 12
Improvements ✅
(secondary)
-1.0% [-1.3%, -0.1%] 38
All ❌✅ (primary) -0.3% [-1.1%, 0.4%] 16

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

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 5, 2024
@workingjubilee
Copy link
Contributor

Much fewer tiny regressions in binary size. Interesting.

@joboet
Copy link
Contributor

joboet commented Apr 5, 2024

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Apr 5, 2024

📌 Commit 0989416 has been approved by joboet

It is now in the queue for this repository.

@bors 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 5, 2024
@bors
Copy link
Contributor

bors commented Apr 6, 2024

⌛ Testing commit 0989416 with merge 30840c5...

@bors
Copy link
Contributor

bors commented Apr 6, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 6, 2024
@bors bors merged commit 30840c5 into rust-lang:master Apr 6, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 6, 2024
@rust-timer
Copy link
Collaborator

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

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

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)
9.3% [9.3%, 9.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
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.3% [0.2%, 0.4%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.6% [-1.1%, -0.0%] 11
Improvements ✅
(secondary)
-0.3% [-1.1%, -0.1%] 38
All ❌✅ (primary) -0.3% [-1.1%, 0.4%] 15

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

@GnomedDev GnomedDev deleted the remove-threadname-alloc branch April 6, 2024 11:42
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 6, 2024
Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This currently does not have a regression test to prevent future changes from re-adding allocations pre-main as I'm [having trouble](GnomedDev@6f7be53) implementing it, but if wanted I can draft this PR until that test is ready.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 24, 2024
…trieb

Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 25, 2024
…trieb

Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 26, 2024
…trieb

Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 27, 2024
…trieb,saethlin

Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 29, 2024
…trieb,saethlin

Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 30, 2024
Remove last rt::init allocation for thread info

Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
- The thread name can just be a hard coded literal, as was done in rust-lang#123433.
- The ThreadId is always the `1` value, so `ThreadId::new` now starts at `2` and can fabricate the `1` value when needed.
- Storing Parker in a static that is initialized once at startup. This uses SyncUnsafeCell and MaybeUninit as this is quite performance critical and we don't need synchronization or to store a tag value and possibly leave in a panic.

This also adds a UI test to make sure that allocations do not occur before main ever again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants