-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Add ThreadId for comparing threads #36341
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
Merged
Merged
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
5bd834b
Add ThreadId for comparing threads
sagebind 6e10e29
Fix tests
sagebind 894ef96
Generate ID using u64 + atomic spinlock
sagebind e80fd25
Use mutex to guard thread ID counter
sagebind 032bffa
Unlock guard before overflow panic
sagebind File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,6 +165,7 @@ use panic; | |
use panicking; | ||
use str; | ||
use sync::{Mutex, Condvar, Arc}; | ||
use sync::atomic::{AtomicBool, Ordering}; | ||
use sys::thread as imp; | ||
use sys_common::thread_info; | ||
use sys_common::util; | ||
|
@@ -524,13 +525,74 @@ pub fn park_timeout(dur: Duration) { | |
*guard = false; | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
// ThreadId | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
/// A unique identifier for a running thread. | ||
/// | ||
/// A `ThreadId` is an opaque object that has a unique value for each thread | ||
/// that creates one. `ThreadId`s do not correspond to a thread's system- | ||
/// designated identifier. | ||
#[unstable(feature = "thread_id", issue = "21507")] | ||
#[derive(Eq, PartialEq, Copy, Clone)] | ||
pub struct ThreadId(u64); | ||
|
||
impl ThreadId { | ||
// Generate a new unique thread ID. Since this function is called every | ||
// time a thread is created, this is optimized to generate unique values | ||
// as quickly as possible. | ||
fn new() -> ThreadId { | ||
// 64-bit operations are not atomic on all systems, so use an atomic | ||
// flag as a guard around a 64-bit global counter. The window for | ||
// contention on the counter is rather narrow since the general case | ||
// should be compiled down to three instructions between locking and | ||
// unlocking the guard. Since contention on the guard is low, use a | ||
// spinlock that optimizes for the fast path of the guard being | ||
// unlocked. | ||
static GUARD: AtomicBool = AtomicBool::new(false); | ||
static mut COUNTER: u64 = 0; | ||
|
||
// Get exclusive access to the counter. | ||
while GUARD.compare_exchange_weak( | ||
false, | ||
true, | ||
Ordering::Acquire, | ||
Ordering::Relaxed | ||
).is_err() { | ||
// Give up the rest of our thread quantum if another thread is | ||
// using the counter. This is the slow_er_ path. | ||
yield_now(); | ||
} | ||
|
||
// We have exclusive access to the counter, so use it fast and get out. | ||
let id = unsafe { | ||
// If we somehow use up all our bits, panic so that we're not | ||
// covering up subtle bugs of IDs being reused. | ||
if COUNTER == ::u64::MAX { | ||
panic!("failed to generate unique thread ID: bitspace exhausted"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This'll want to unlock the lock before the thread panics There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I'll fix that in my next commit. |
||
} | ||
|
||
let id = COUNTER; | ||
COUNTER += 1; | ||
id | ||
}; | ||
|
||
// Unlock the guard. | ||
GUARD.store(false, Ordering::Release); | ||
|
||
ThreadId(id) | ||
} | ||
} | ||
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
// Thread | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
/// The internal representation of a `Thread` handle | ||
struct Inner { | ||
name: Option<CString>, // Guaranteed to be UTF-8 | ||
id: ThreadId, | ||
lock: Mutex<bool>, // true when there is a buffered unpark | ||
cvar: Condvar, | ||
} | ||
|
@@ -551,6 +613,7 @@ impl Thread { | |
Thread { | ||
inner: Arc::new(Inner { | ||
name: cname, | ||
id: ThreadId::new(), | ||
lock: Mutex::new(false), | ||
cvar: Condvar::new(), | ||
}) | ||
|
@@ -569,6 +632,12 @@ impl Thread { | |
} | ||
} | ||
|
||
/// Gets the thread's unique identifier. | ||
#[unstable(feature = "thread_id", issue = "21507")] | ||
pub fn id(&self) -> ThreadId { | ||
self.inner.id | ||
} | ||
|
||
/// Gets the thread's name. | ||
/// | ||
/// # Examples | ||
|
@@ -977,6 +1046,17 @@ mod tests { | |
thread::sleep(Duration::from_millis(2)); | ||
} | ||
|
||
#[test] | ||
fn test_thread_id_equal() { | ||
assert!(thread::current().id() == thread::current().id()); | ||
} | ||
|
||
#[test] | ||
fn test_thread_id_not_equal() { | ||
let spawned_id = thread::spawn(|| thread::current().id()).join().unwrap(); | ||
assert!(thread::current().id() != spawned_id); | ||
} | ||
|
||
// NOTE: the corresponding test for stderr is in run-pass/thread-stderr, due | ||
// to the test harness apparently interfering with stderr configuration. | ||
} |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a busy loop, could this just use a mutex for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the best way would be to set up a mutex statically. Isn't
StaticMutex
gone now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of libstd you can reach into
sys_common::mutex::Mutex
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like more time would be spent in the mutex syscalls than actually using the counter -- doesn't seem worth it. Using a spinlock, at least the normal case of no contention uses no syscalls.
There's no defined time where we could call
Mutex::destroy()
, since a thread could be created at any time.ONCE
uses a similar atomics approach as this if I recall for the same reasons.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's ok to in general not call
Mutex::destroy
, and otherwise I don't think there's going to be any real impact here on a mutex vs a spinlock. WhileOnce
uses atomics, it doesn't use spinlocks internally.In general we try to avoid spinlocks wherever possible as there are generally more suitable synchronization primitives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the relevant systems have fast userspace implementations which get used in low-contention conditions. Please use
Mutex
or whatever based onMutex
in order to avoid rolling out your own.