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.

Sign up for GitHub

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
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cf720b
Make Option<ThreadId> no larger than ThreadId, with NonZeroU64
SimonSapin Mar 19, 2019
c1d9191
Add a test for size_of Option<ThreadId>
SimonSapin Mar 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/libstd/thread/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ use crate::ffi::{CStr, CString};
use crate::fmt;
use crate::io;
use crate::mem;
use crate::num::NonZeroU64;
use crate::panic;
use crate::panicking;
use crate::str;
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@sanmai-NL sanmai-NL Mar 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

static mut COUNTER: u64 = 0;
static mut COUNTER: u64 = 1;

unsafe {
let _guard = GUARD.lock();
Expand All @@ -1058,7 +1059,7 @@ impl ThreadId {
let id = COUNTER;
COUNTER += 1;

ThreadId(id)
ThreadId(NonZeroU64::new(id).unwrap())
}
}
}
Expand Down