Remove last rt::init allocation for thread info #123550
Open
+151
−42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
r? @Nilstrieb rustbot has assigned @Nilstrieb. Use |
rustbot
added
S-waiting-on-review
|
I just checked and |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
GnomedDev
force-pushed
the
remove-initial-arc
branch
from
d5a081b
to
d5b8b00
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
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.
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (666bbff): comparison URL. Overall result: ❌ regressions - no action neededBenchmarking 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 Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis 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.
CyclesResultsThis 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.
Binary sizeResultsThis 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.
Bootstrap: 666.761s -> 666.789s (0.00%) |
rustbot
removed
the
S-waiting-on-perf
Nilstrieb
reviewed
Apr 6, 2024
|
The increased complexity is a bit sad, but this is already a complex and unsafe process to initialise the basics for the runtime, so I felt that the increased performance and decreased compile time was worth a small amount of well documented unsafety. |
|
Hmm, looking at the actual perf run, it seems quite negative which is certainly unexpected. How is this commonly debugged, as I don't want to go off vibes? |
|
Run the cachegrind command to see where in the compiler the diff occurs. Though FWIW, I would expect these results to be noise and wouldn't chase them further myself - I'd just treat it as "makes no difference". |
so yeah, no real decreased compile time. as for increased performance, I doubt that this will be measurable, maybe |
|
Okay, I don't have a benchmark (I never have a benchmark). Would you like me to rewrite this using OnceLock, just to see if that perf run is also neutral? |
GnomedDev
force-pushed
the
remove-initial-arc
branch
2 times, most recently
from
778330b
to
ab8eba1
Compare
|
Sorted the existing review comments, just waiting on a reply to my last comment. |
|
☔ The latest upstream changes (presumably #123913) made this pull request unmergeable. Please resolve the merge conflicts. |
GnomedDev
force-pushed
the
remove-initial-arc
branch
from
ab8eba1
to
2c45b39
Compare
|
Okay, @Nilstrieb I've been trying for the last week different ways to make this less unsafe and complex but it doesn't seem possible with the "Parker must be initialized in place" requirement. I cannot initialize a OnceLock or an Option in-place without increasing complexity significantly, so this seems like the least complex (and most performant) way to do this. |
Nilstrieb
reviewed
Apr 23, 2024
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.
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors try |
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.
|
☀️ Try build successful - checks-actions |
|
hm. let's try again. |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Apr 30, 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.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
I'm so sorry? |
|
good luck @GnomedDev , looks like the debuginfo does not like you today, probably overriden by compiletest despite the flag? |
|
Should I just kill the test for now and try getting it merged separately? |
somewhat baffled here... |
|
Isn't that the error from the test runner, not the actual test? |
I am not comfortable merging this with a failing test like that. I'd recommend running the docker image yourself locally and adding interactive prompts with bash into it to try to debug what's going on. |
|
(sync) |
bors
added
S-waiting-on-author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the last allocation pre-main by just not storing anything in std::thread::Thread for the main thread.
1value, soThreadId::newnow starts at2and can fabricate the1value when needed.This also adds a UI test to make sure that allocations do not occur before main ever again.