From f6b72b0b0fd72fe6892efc329b927062aa57027b Mon Sep 17 00:00:00 2001 From: Jiahao XU <30436523+NobodyXu@users.noreply.github.com> Date: Sat, 13 Jul 2024 08:19:06 +0000 Subject: [PATCH 1/5] Fix cyclic compilation: Use vendored once_cell Fix #1146 --- Cargo.toml | 3 +-- src/parallel/job_token.rs | 30 ++++++++++++++++++++----- src/parallel/mod.rs | 1 + src/parallel/once_cell.rs | 47 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 74 insertions(+), 7 deletions(-) create mode 100644 src/parallel/once_cell.rs diff --git a/Cargo.toml b/Cargo.toml index a9f8c495f..e04164ae5 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -21,7 +21,6 @@ rust-version = "1.67" [dependencies] jobserver = { version = "0.1.30", default-features = false, optional = true } -once_cell = { version = "1.19", optional = true } [target.'cfg(unix)'.dependencies] # Don't turn on the feature "std" for this, see https://github.com/rust-lang/cargo/issues/4866 @@ -29,7 +28,7 @@ once_cell = { version = "1.19", optional = true } libc = { version = "0.2.62", default-features = false, optional = true } [features] -parallel = ["dep:libc", "dep:jobserver", "dep:once_cell"] +parallel = ["dep:libc", "dep:jobserver"] # This is a placeholder feature for people who incorrectly used `cc` with `features = ["jobserver"]` # so that they aren't broken. This has never enabled `parallel`, so we won't do that. jobserver = [] diff --git a/src/parallel/job_token.rs b/src/parallel/job_token.rs index da1ab737a..d5043a8ba 100644 --- a/src/parallel/job_token.rs +++ b/src/parallel/job_token.rs @@ -2,7 +2,7 @@ use std::marker::PhantomData; use crate::Error; -use once_cell::sync::OnceCell; +use super::once_cell::OnceCell; pub(crate) struct JobToken(PhantomData<()>); @@ -163,7 +163,7 @@ mod inherited_jobserver { pub(crate) struct ActiveJobServer<'a> { jobserver: &'a JobServer, - helper_thread: OnceCell, + helper_thread: OnceCell>, } impl<'a> ActiveJobServer<'a> { @@ -183,10 +183,30 @@ mod inherited_jobserver { } Ok(None) => YieldOnce::default().await, Err(err) if err.kind() == io::ErrorKind::Unsupported => { + let mut err = None; // Fallback to creating a help thread with blocking acquire - let helper_thread = self - .helper_thread - .get_or_try_init(|| HelperThread::new(self.jobserver))?; + let helper_thread = self.helper_thread.get_or_init(|| { + match HelperThread::new(self.jobserver) { + Ok(thread) => Some(thread), + Err(error) => { + err = Some(error); + None + } + } + }); + + if let Some(err) = err { + return Err(err.into()); + } + + let helper_thread = if let Some(thread) = helper_thread { + thread + } else { + return Err(Error::new( + ErrorKind::JobserverHelpThreadError, + "Creation failed", + )); + }; match helper_thread.rx.try_recv() { Ok(res) => { diff --git a/src/parallel/mod.rs b/src/parallel/mod.rs index 019eae102..7054ec867 100644 --- a/src/parallel/mod.rs +++ b/src/parallel/mod.rs @@ -1,3 +1,4 @@ pub(crate) mod async_executor; pub(crate) mod job_token; +pub(crate) mod once_cell; pub(crate) mod stderr; diff --git a/src/parallel/once_cell.rs b/src/parallel/once_cell.rs new file mode 100644 index 000000000..f30b3d2ea --- /dev/null +++ b/src/parallel/once_cell.rs @@ -0,0 +1,47 @@ +use std::{ + cell::UnsafeCell, + marker::PhantomData, + mem::MaybeUninit, + panic::{RefUnwindSafe, UnwindSafe}, + sync::Once, +}; + +pub(crate) struct OnceCell { + once: Once, + value: UnsafeCell>, + _marker: PhantomData, +} + +impl OnceCell { + pub(crate) const fn new() -> Self { + Self { + once: Once::new(), + value: UnsafeCell::new(MaybeUninit::uninit()), + _marker: PhantomData, + } + } + + pub(crate) fn get_or_init(&self, f: impl FnOnce() -> T) -> &T { + self.once.call_once_force(|_| { + unsafe { &mut *self.value.get() }.write(f()); + }); + unsafe { (&*self.value.get()).assume_init_ref() } + } +} + +unsafe impl Sync for OnceCell {} +unsafe impl Send for OnceCell {} + +impl RefUnwindSafe for OnceCell {} +impl UnwindSafe for OnceCell {} + +impl Drop for OnceCell { + #[inline] + fn drop(&mut self) { + if self.once.is_completed() { + // SAFETY: The cell is initialized and being dropped, so it can't + // be accessed again. + unsafe { (&mut *self.value.get()).assume_init_drop() }; + } + } +} From 11d214db2746f6a94c734c8ab932d820b8b22175 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 14 Jul 2024 22:03:02 +1000 Subject: [PATCH 2/5] Rename `OnceCell` to `OnceLock` Signed-off-by: Jiahao XU --- src/parallel/job_token.rs | 10 +++++----- src/parallel/mod.rs | 2 +- src/parallel/{once_cell.rs => once_lock.rs} | 14 +++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) rename src/parallel/{once_cell.rs => once_lock.rs} (76%) diff --git a/src/parallel/job_token.rs b/src/parallel/job_token.rs index d5043a8ba..8c633f78e 100644 --- a/src/parallel/job_token.rs +++ b/src/parallel/job_token.rs @@ -2,7 +2,7 @@ use std::marker::PhantomData; use crate::Error; -use super::once_cell::OnceCell; +use super::once_lock::OnceLock; pub(crate) struct JobToken(PhantomData<()>); @@ -37,7 +37,7 @@ impl JobTokenServer { /// compilation. fn new() -> &'static Self { // TODO: Replace with a OnceLock once MSRV is 1.70 - static JOBSERVER: OnceCell = OnceCell::new(); + static JOBSERVER: OnceLock = OnceLock::new(); JOBSERVER.get_or_init(|| { unsafe { inherited_jobserver::JobServer::from_env() } @@ -71,7 +71,7 @@ impl ActiveJobTokenServer { } mod inherited_jobserver { - use super::{JobToken, OnceCell}; + use super::{JobToken, OnceLock}; use crate::{parallel::async_executor::YieldOnce, Error, ErrorKind}; @@ -137,7 +137,7 @@ mod inherited_jobserver { pub(super) fn enter_active(&self) -> ActiveJobServer<'_> { ActiveJobServer { jobserver: self, - helper_thread: OnceCell::new(), + helper_thread: OnceLock::new(), } } } @@ -163,7 +163,7 @@ mod inherited_jobserver { pub(crate) struct ActiveJobServer<'a> { jobserver: &'a JobServer, - helper_thread: OnceCell>, + helper_thread: OnceLock>, } impl<'a> ActiveJobServer<'a> { diff --git a/src/parallel/mod.rs b/src/parallel/mod.rs index 7054ec867..70a56e74d 100644 --- a/src/parallel/mod.rs +++ b/src/parallel/mod.rs @@ -1,4 +1,4 @@ pub(crate) mod async_executor; pub(crate) mod job_token; -pub(crate) mod once_cell; +pub(crate) mod once_lock; pub(crate) mod stderr; diff --git a/src/parallel/once_cell.rs b/src/parallel/once_lock.rs similarity index 76% rename from src/parallel/once_cell.rs rename to src/parallel/once_lock.rs index f30b3d2ea..03971481d 100644 --- a/src/parallel/once_cell.rs +++ b/src/parallel/once_lock.rs @@ -6,13 +6,13 @@ use std::{ sync::Once, }; -pub(crate) struct OnceCell { +pub(crate) struct OnceLock { once: Once, value: UnsafeCell>, _marker: PhantomData, } -impl OnceCell { +impl OnceLock { pub(crate) const fn new() -> Self { Self { once: Once::new(), @@ -29,13 +29,13 @@ impl OnceCell { } } -unsafe impl Sync for OnceCell {} -unsafe impl Send for OnceCell {} +unsafe impl Sync for OnceLock {} +unsafe impl Send for OnceLock {} -impl RefUnwindSafe for OnceCell {} -impl UnwindSafe for OnceCell {} +impl RefUnwindSafe for OnceLock {} +impl UnwindSafe for OnceLock {} -impl Drop for OnceCell { +impl Drop for OnceLock { #[inline] fn drop(&mut self) { if self.once.is_completed() { From 3b7f2ef052a9c4678cc4ad7d255408dddb754702 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 14 Jul 2024 22:16:51 +1000 Subject: [PATCH 3/5] Use `Option` instead of `OnceLock` for falliable initialization The function could have exclusive access to the data, so it does not need any synchronization. Signed-off-by: Jiahao XU --- src/lib.rs | 2 +- src/parallel/job_token.rs | 35 +++++++++-------------------------- 2 files changed, 10 insertions(+), 27 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 05d6f6932..ca28d3f7c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1502,7 +1502,7 @@ impl Build { } // Limit our parallelism globally with a jobserver. - let tokens = parallel::job_token::ActiveJobTokenServer::new(); + let mut tokens = parallel::job_token::ActiveJobTokenServer::new(); // When compiling objects in parallel we do a few dirty tricks to speed // things up: diff --git a/src/parallel/job_token.rs b/src/parallel/job_token.rs index 8c633f78e..2640cea6c 100644 --- a/src/parallel/job_token.rs +++ b/src/parallel/job_token.rs @@ -62,8 +62,8 @@ impl ActiveJobTokenServer { } } - pub(crate) async fn acquire(&self) -> Result { - match &self { + pub(crate) async fn acquire(&mut self) -> Result { + match self { Self::Inherited(jobserver) => jobserver.acquire().await, Self::InProcess(jobserver) => Ok(jobserver.acquire().await), } @@ -71,7 +71,7 @@ impl ActiveJobTokenServer { } mod inherited_jobserver { - use super::{JobToken, OnceLock}; + use super::JobToken; use crate::{parallel::async_executor::YieldOnce, Error, ErrorKind}; @@ -137,7 +137,7 @@ mod inherited_jobserver { pub(super) fn enter_active(&self) -> ActiveJobServer<'_> { ActiveJobServer { jobserver: self, - helper_thread: OnceLock::new(), + helper_thread: None, } } } @@ -163,11 +163,11 @@ mod inherited_jobserver { pub(crate) struct ActiveJobServer<'a> { jobserver: &'a JobServer, - helper_thread: OnceLock>, + helper_thread: Option, } impl<'a> ActiveJobServer<'a> { - pub(super) async fn acquire(&self) -> Result { + pub(super) async fn acquire(&mut self) -> Result { let mut has_requested_token = false; loop { @@ -183,29 +183,12 @@ mod inherited_jobserver { } Ok(None) => YieldOnce::default().await, Err(err) if err.kind() == io::ErrorKind::Unsupported => { - let mut err = None; // Fallback to creating a help thread with blocking acquire - let helper_thread = self.helper_thread.get_or_init(|| { - match HelperThread::new(self.jobserver) { - Ok(thread) => Some(thread), - Err(error) => { - err = Some(error); - None - } - } - }); - - if let Some(err) = err { - return Err(err.into()); - } - - let helper_thread = if let Some(thread) = helper_thread { + let helper_thread = if let Some(thread) = self.helper_thread.as_ref() { thread } else { - return Err(Error::new( - ErrorKind::JobserverHelpThreadError, - "Creation failed", - )); + self.helper_thread + .insert(HelperThread::new(self.jobserver)?) }; match helper_thread.rx.try_recv() { From 32cf00530248ee90ff2305847f6208c15ad32a30 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 14 Jul 2024 22:18:42 +1000 Subject: [PATCH 4/5] Use `Once::call_once` instead of `call_once_force` Signed-off-by: Jiahao XU --- src/parallel/once_lock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parallel/once_lock.rs b/src/parallel/once_lock.rs index 03971481d..21918b3a4 100644 --- a/src/parallel/once_lock.rs +++ b/src/parallel/once_lock.rs @@ -22,7 +22,7 @@ impl OnceLock { } pub(crate) fn get_or_init(&self, f: impl FnOnce() -> T) -> &T { - self.once.call_once_force(|_| { + self.once.call_once(|| { unsafe { &mut *self.value.get() }.write(f()); }); unsafe { (&*self.value.get()).assume_init_ref() } From ec015672b6baff7dfa6e9cf7bcb8fcdb01d2bc69 Mon Sep 17 00:00:00 2001 From: Jiahao XU Date: Sun, 14 Jul 2024 22:23:26 +1000 Subject: [PATCH 5/5] Remove use of one `unsafe` fn Signed-off-by: Jiahao XU --- src/parallel/once_lock.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parallel/once_lock.rs b/src/parallel/once_lock.rs index 21918b3a4..c48dbb72e 100644 --- a/src/parallel/once_lock.rs +++ b/src/parallel/once_lock.rs @@ -41,7 +41,7 @@ impl Drop for OnceLock { if self.once.is_completed() { // SAFETY: The cell is initialized and being dropped, so it can't // be accessed again. - unsafe { (&mut *self.value.get()).assume_init_drop() }; + unsafe { self.value.get_mut().assume_init_drop() }; } } }