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

std::thread::Thread grew from one to two pointer sizes #132619

Closed
ginnyTheCat opened this issue Nov 4, 2024 · 3 comments · Fixed by #132654
Closed

std::thread::Thread grew from one to two pointer sizes #132619

ginnyTheCat opened this issue Nov 4, 2024 · 3 comments · Fixed by #132654
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@ginnyTheCat
Copy link

Caused by the changes in #123550 the size of std::thread::Thread grew from one to two pointer sizes.

The docs don't promise Thread to have a certain size so I'm not quite sure whether this would be considered a breaking change. It doesn't seem however like this was noticed/acknowledged in the PR, as having a larger thread handle might negate the benefits that PR brought with it.

Code

I tried this code:

let pointer_size = size_of::<*const ()>();
let thread_size = size_of::<std::thread::Thread>();
assert_eq!(pointer_size, thread_size);

I expected to see this happen: it to run successfully

Instead, this happened: the assert is unfulfilled and it panics

Version it worked on

It most recently worked on: Rust 1.82

Version with regression

rustc --version --verbose:

rustc 1.84.0-nightly (b8c8287a2 2024-11-03)
binary: rustc
commit-hash: b8c8287a229cd79604aa84c25e1235fc78cd5f2e
commit-date: 2024-11-03
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.3
@ginnyTheCat ginnyTheCat added C-bug Category: This is a bug. regression-untriaged Untriaged performance or correctness regression. labels Nov 4, 2024
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-untriaged Untriaged performance or correctness regression. labels Nov 4, 2024
@ginnyTheCat ginnyTheCat changed the title std::thread::Thread growing in size std::thread::Thread grew from one to two pointer sizes Nov 4, 2024
@GnomedDev
Copy link
Contributor

GnomedDev commented Nov 5, 2024

This performance regression (Thread inline size increasing) only affects code which handles std::thread::Thread, which is going to be much less than programs that hit the rt startup code which previously allocated for this, however this regression isn't impossible to claw back.

Alignment tagging could possibly allow fitting the two pointers into a single one, via even more unsafe code, although that's more unsafe as a maintenance burden.

@saethlin saethlin added T-libs Relevant to the library team, which will review and decide on the PR/issue. C-discussion Category: Discussion or questions that doesn't represent real issues. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. C-bug Category: This is a bug. I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Nov 5, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Nov 5, 2024

Yes, unspecified layouts are unspecified and std::thread::Thread can be 1 pointer or 1 kilobyte.

A benchmark that reflects an excerpt from an even vaguely practical program would be useful information, here, to measure any supposed regressions against. Or improvements, for that matter. When you start packing more data into the same eight bytes, it is very easy for packing more data in a smaller space to cost more instructions to actually act on that data.

@workingjubilee
Copy link
Member

well, unstable implementation details may change at any time.

@workingjubilee workingjubilee closed this as not planned Won't fix, can't repro, duplicate, stale Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants