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

Make Option<ThreadId> no larger than ThreadId, with NonZeroU64 #59291

Merged
merged 2 commits into from Mar 23, 2019

Conversation

@SimonSapin
Copy link
Contributor

commented Mar 19, 2019

No description provided.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -1036,15 +1037,15 @@ pub fn park_timeout(dur: Duration) {
/// [`Thread`]: ../../std/thread/struct.Thread.html
#[stable(feature = "thread_id", since = "1.19.0")]
#[derive(Eq, PartialEq, Clone, Copy, Hash, Debug)]
pub struct ThreadId(u64);
pub struct ThreadId(NonZeroU64);

impl ThreadId {
// Generate a new unique thread ID.
fn new() -> ThreadId {
// We never call `GUARD.init()`, so it is UB to attempt to
// acquire this mutex reentrantly!
static GUARD: mutex::Mutex = mutex::Mutex::new();

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Mar 19, 2019

Member

Out of interest, is there any reason we don't use an atomicu64 here instead?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 19, 2019

Member

Using a mutex is slightly more portable than an AtomicU64, and this isn't really perf critical so we didn't need to reach for that.

This comment has been minimized.

Copy link
@sanmai-NL

sanmai-NL Mar 20, 2019

@alexcrichton: At @Structure-Systems we're logging thread IDs at a high frequency. How does this design decision impact performance?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 20, 2019

Member

@sanmai-NL have you profiled and seen this as a problem?

This comment has been minimized.

Copy link
@sfackler

sfackler Mar 20, 2019

Member

This mutex is only locked when a thread is created, not when its thread id is read, FYI.

@@ -1036,15 +1037,15 @@ pub fn park_timeout(dur: Duration) {
/// [`Thread`]: ../../std/thread/struct.Thread.html
#[stable(feature = "thread_id", since = "1.19.0")]
#[derive(Eq, PartialEq, Clone, Copy, Hash, Debug)]
pub struct ThreadId(u64);
pub struct ThreadId(NonZeroU64);

This comment has been minimized.

Copy link
@oli-obk

oli-obk Mar 19, 2019

Contributor

If you want to allow more aggressive niche optimizations you can also reserve a few (thousand?) values at the end of the value range via #[rustc_layout_scalar_valid_range_end(0xFFFF_FFFF_FFFF_0000)] (that will require unsafe code though)

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Mar 20, 2019

Author Contributor

I’m a bit wary of adding more use of rustc_layout_scalar_valid_range_end when there seems to be no plan to ever stabilize it. And I don’t know how common are cases when this would make a difference over non-zero.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Thanks @SimonSapin! r=me if you add a test as well asserting that sizeof(ThreadId) == sizeof(Option<ThreadId>)

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

📌 Commit c1d9191 has been approved by alexcrichton

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Mar 22, 2019

Rollup merge of rust-lang#59291 - SimonSapin:nonzero-thread-id, r=ale…
…xcrichton

Make Option<ThreadId> no larger than ThreadId, with NonZeroU64

Centril added a commit to Centril/rust that referenced this pull request Mar 22, 2019

Rollup merge of rust-lang#59291 - SimonSapin:nonzero-thread-id, r=ale…
…xcrichton

Make Option<ThreadId> no larger than ThreadId, with NonZeroU64

bors added a commit that referenced this pull request Mar 22, 2019

Auto merge of #59370 - Centril:rollup, r=Centril
Rollup of 18 pull requests

Successful merges:

 - #59106 (Add peer_addr function to UdpSocket)
 - #59170 (Add const generics to rustdoc)
 - #59172 (Update and clean up several parts of CONTRIBUTING.md)
 - #59190 (consistent naming for Rhs type parameter in libcore/ops)
 - #59236 (Rename miri component to miri-preview)
 - #59266 (Do not complain about non-existing fields after parse recovery)
 - #59273 (some small HIR doc improvements)
 - #59291 (Make Option<ThreadId> no larger than ThreadId, with NonZeroU64)
 - #59297 (convert field/method confusion help to suggestions)
 - #59304 (Move some bench tests back from libtest)
 - #59309 (Add messages for different verbosity levels. Output copy actions.)
 - #59321 (Unify E0109, E0110 and E0111)
 - #59322 (Tweak incorrect escaped char diagnostic)
 - #59323 (use suggestions for "enum instead of variant" error)
 - #59327 (Add NAN test to docs)
 - #59329 (cleanup: Remove compile-fail-fulldeps directory again)
 - #59347 (Move one test from run-make-fulldeps to ui)
 - #59360 (Add tracking issue number for `seek_convenience`)

Failed merges:

r? @ghost

@bors bors merged commit c1d9191 into rust-lang:master Mar 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.