std: directly use pthread in UNIX parker implementation

[joboet]

Mutex and Condvar are being replaced by more efficient implementations, which need thread parking themselves (see #93740). Therefore we should use the pthread synchronization primitives directly. Also, we can avoid allocating the mutex and condition variable because the Parker struct is being placed in an Arc anyways.

This basically is just a copy of the current Mutex and Condvar code, which will however be removed (again, see #93740). An alternative implementation could be to use dedicated private OsMutex and OsCondvar types, but all the other platforms supported by std actually have their own thread parking primitives.

I used Pin to guarantee a stable address for the Parker struct, while the current implementation does not, rather using extra unsafe declaration. Since the thread struct is shared anyways, I assumed this would not add too much clutter while being clearer.

[rust-highfive]

r? @thomcc

(rust-highfive has picked a reviewer for you, use r? to override)

[thomcc]

An alternative implementation could be to use a dedicated private OsMutex and OsCondvar type...

Hm, I think I'd prefer this, but I won't block it on it. I'll get to the review later today.

[thomcc]

Also CC @m-ou-se just to make sure she's aware of this change.

(Also if you'd rather review it, feel free to take. Although I do feel comfortable doing this review, I suspect you've been thinking much more about this code recently)

[m-ou-se]

Oh, thanks for working on this! I'll take a look tomorrow. :)

[thomcc]

This looks fine to me. I think it's a little unfortunate that the "generic" parker is now unix-specific, and wonder if it should be renamed to the pthreads parker or something. But I don't feel that strongly about it.

I also am surprised you don't need to do the condattr clock dance on all targets, I had thought that at least on Darwin that it was needed (but I'll trust if you've done investigation and it is not).

r=me unless @m-ou-se wants to review it too.

[m-ou-se]

If this took a &mut MaybeUninit<Parker> instead of a *mut Parker, then this function could be safe.

Without MaybeUninit 'projections' though, that makes using this function tricky when the parker is part of a bigger MaybeUninit. :(

[joboet]

That, and the lack of a dependency on pin_project in std, led me to use a pointer here, as we would require unsafe in Thread::new anyways. I could change this however.

[m-ou-se]

Looks good. Thanks for working on this! :)

[thomcc]

Oh, this is waiting on me.

@bors r+

[bors]

📌 Commit 2b71201 has been approved by thomcc

[m-ou-se]

@bors r=thomcc rollup=iffy

[bors]

📌 Commit 5502733 has been approved by thomcc

[bors]

⌛ Testing commit 5502733 with merge 7cd4e251f03bd860be1f0f5d81fe3a41e01559b9...

[bors]

💔 Test failed - checks-actions

[joboet]

Like so?

// cdb-check:    [...] inner            [...] [Type: core::pin::Pin<alloc::sync::Arc<std::thread::Inner> >]

[m-ou-se]

I suppose you can leave out the extra whitespace before [...] and just leave a single space. Then it still works if there's less whitespace in cdb's output.

[m-ou-se]

([...] just matches anything, see

rust/src/tools/compiletest/src/runtest/debugger.rs

Lines 82 to 84 in 3bfeffd

	fn check_single_line(line: &str, check_line: &str) -> bool {
	// Allow check lines to leave parts unspecified (e.g., uninitialized
	// bits in the wrong case of an enum) with the notation "[...]".

)

[m-ou-se]

Let's try again :)

@bors r=thomcc

[bors]

📌 Commit 1285fb7 has been approved by thomcc

[bors]

⌛ Testing commit 1285fb7 with merge baaa3b6...

[bors]

☀️ Test successful - checks-actions
Approved by: thomcc
Pushing baaa3b6 to master...

[rust-timer]

Finished benchmarking commit (baaa3b6): comparison url.

Summary:

• Primary benchmarks: 🎉 relevant improvement found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	0	1	0	1
mean2	N/A	N/A	-0.4%	N/A	-0.4%
max	N/A	N/A	-0.4%	N/A	-0.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩

2. the arithmetic mean of the percent change ↩

[nbdd0121]

An alternative implementation could be to use dedicated private OsMutex and OsCondvar types, but all the other platforms supported by std actually have their own thread parking primitives.

We still have hermit, solid and sgx which doesn't have their own thread parking primitives.

[joboet]

Well, they are not currently implemented:

• Hermit has block_current_task and wakeup_task (used in the current mutex):

rust/library/std/src/sys/hermit/mutex.rs

Lines 182 to 208 in 87937d3

	#[inline]
	pub unsafe fn lock(&self) {
	loop {
	let mut guard = self.inner.lock();
	if guard.locked == false {
	guard.locked = true;
	return;
	} else {
	let prio = abi::get_priority();
	let id = abi::getpid();
	guard.blocked_task.push(prio, id);
	abi::block_current_task();
	drop(guard);
	abi::yield_now();
	}
	}
	}
	#[inline]
	pub unsafe fn unlock(&self) {
	let mut guard = self.inner.lock();
	guard.locked = false;
	if let Some(tid) = guard.blocked_task.pop() {
	abi::wakeup_task(tid);
	}
	}

• Solid has event flags via µITRON (see http://www.ertl.jp/ITRON/SPEC/FILE/mitron-400e.pdf)
• SGX has EV_UNPARK (used in the current mutex)

rust/library/std/src/sys/sgx/waitqueue/mod.rs

Lines 147 to 216 in 87937d3

	/// Adds the calling thread to the `WaitVariable`'s wait queue, then wait
	/// until a wakeup event.
	///
	/// This function does not return until this thread has been awoken.
	pub fn wait<T, F: FnOnce()>(mut guard: SpinMutexGuard<'_, WaitVariable<T>>, before_wait: F) {
	// very unsafe: check requirements of UnsafeList::push
	unsafe {
	let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry {
	tcs: thread::current(),
	wake: false,
	}));
	let entry = guard.queue.inner.push(&mut entry);
	drop(guard);
	before_wait();
	while !entry.lock().wake {
	// don't panic, this would invalidate `entry` during unwinding
	let eventset = rtunwrap!(Ok, usercalls::wait(EV_UNPARK, WAIT_INDEFINITE));
	rtassert!(eventset & EV_UNPARK == EV_UNPARK);
	}
	}
	}
	/// Adds the calling thread to the `WaitVariable`'s wait queue, then wait
	/// until a wakeup event or timeout. If event was observed, returns true.
	/// If not, it will remove the calling thread from the wait queue.
	pub fn wait_timeout<T, F: FnOnce()>(
	lock: &SpinMutex<WaitVariable<T>>,
	timeout: Duration,
	before_wait: F,
	) -> bool {
	// very unsafe: check requirements of UnsafeList::push
	unsafe {
	let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry {
	tcs: thread::current(),
	wake: false,
	}));
	let entry_lock = lock.lock().queue.inner.push(&mut entry);
	before_wait();
	usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake);
	// acquire the wait queue's lock first to avoid deadlock.
	let mut guard = lock.lock();
	let success = entry_lock.lock().wake;
	if !success {
	// nobody is waking us up, so remove our entry from the wait queue.
	guard.queue.inner.remove(&mut entry);
	}
	success
	}
	}
	/// Either find the next waiter on the wait queue, or return the mutex
	/// guard unchanged.
	///
	/// If a waiter is found, a `WaitGuard` is returned which will notify the
	/// waiter when it is dropped.
	pub fn notify_one<T>(
	mut guard: SpinMutexGuard<'_, WaitVariable<T>>,
	) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
	unsafe {
	if let Some(entry) = guard.queue.inner.pop() {
	let mut entry_guard = entry.lock();
	let tcs = entry_guard.tcs;
	entry_guard.wake = true;
	drop(entry);
	Ok(WaitGuard { mutex_guard: Some(guard), notified_tcs: NotifiedTcs::Single(tcs) })
	} else {
	Err(guard)
	}
	}
	}