From e438d27d80112be72d5eeb7dc261866bb7d5a7d6 Mon Sep 17 00:00:00 2001 From: Alex Kirszenberg Date: Mon, 7 Aug 2023 14:23:07 +0200 Subject: [PATCH] Hide some turbo_tasks internals (vercel/turbo#5584) ### Description This hides _some_ turbo_tasks internals from its public API. It's currently not possible to completely hide them (hence `Vc::into_raw`/unsafe `Vc::>::from_task_id`) because turbo-tasks-memory still depends on some internal types, but this is a step in the right direction. --- crates/node-file-trace/src/lib.rs | 4 +- .../turbo-tasks-fs/examples/hash_directory.rs | 2 +- crates/turbo-tasks-fs/examples/hash_glob.rs | 2 +- .../benches/scope_stress.rs | 2 +- crates/turbo-tasks-memory/benches/stress.rs | 2 +- crates/turbo-tasks-memory/src/task.rs | 6 +- crates/turbo-tasks/src/lib.rs | 1 + crates/turbo-tasks/src/manager.rs | 45 ++++++------- crates/turbo-tasks/src/primitives.rs | 4 +- crates/turbo-tasks/src/raw_vc.rs | 63 ++++--------------- crates/turbo-tasks/src/raw_vc_set.rs | 24 +++++++ crates/turbo-tasks/src/vc/mod.rs | 30 +++++++-- crates/turbopack-cli/src/build/mod.rs | 2 +- crates/turbopack-core/src/issue/mod.rs | 2 +- .../src/introspect/mod.rs | 12 +--- crates/turbopack-ecmascript/src/lib.rs | 8 +-- crates/turbopack-tests/tests/snapshot.rs | 2 +- crates/turbopack/benches/node_file_trace.rs | 2 +- crates/turbopack/examples/turbopack.rs | 2 +- 19 files changed, 109 insertions(+), 106 deletions(-) create mode 100644 crates/turbo-tasks/src/raw_vc_set.rs diff --git a/crates/node-file-trace/src/lib.rs b/crates/node-file-trace/src/lib.rs index e785f25fcf937..b4c4b92c5c721 100644 --- a/crates/node-file-trace/src/lib.rs +++ b/crates/node-file-trace/src/lib.rs @@ -496,7 +496,7 @@ async fn run>( resolve_options, ); - let source = TransientValue::new(output.node); + let source = TransientValue::new(Vc::into_raw(output)); let issues = output .peek_issues_with_path() .await? @@ -518,7 +518,7 @@ async fn run>( sender.send(output_iter.collect::>()).await?; drop(sender); } - Ok(unit().node) + Ok(unit()) }) }); finish(tt, task).await?; diff --git a/crates/turbo-tasks-fs/examples/hash_directory.rs b/crates/turbo-tasks-fs/examples/hash_directory.rs index 457b83ac8be0e..53eb643f9d300 100644 --- a/crates/turbo-tasks-fs/examples/hash_directory.rs +++ b/crates/turbo-tasks-fs/examples/hash_directory.rs @@ -38,7 +38,7 @@ async fn main() -> Result<()> { let input = fs.root().join("demo".to_string()); let dir_hash = hash_directory(input); print_hash(dir_hash).await?; - Ok(unit().node) + Ok(unit()) }) }); tt.wait_task_completion(task, true).await.unwrap(); diff --git a/crates/turbo-tasks-fs/examples/hash_glob.rs b/crates/turbo-tasks-fs/examples/hash_glob.rs index 5bae962e510c6..f3baa5e74b402 100644 --- a/crates/turbo-tasks-fs/examples/hash_glob.rs +++ b/crates/turbo-tasks-fs/examples/hash_glob.rs @@ -37,7 +37,7 @@ async fn main() -> Result<()> { let glob_result = input.read_glob(glob, true); let dir_hash = hash_glob_result(glob_result); print_hash(dir_hash).await?; - Ok(unit().node) + Ok(unit()) }) }); tt.wait_task_completion(task, true).await.unwrap(); diff --git a/crates/turbo-tasks-memory/benches/scope_stress.rs b/crates/turbo-tasks-memory/benches/scope_stress.rs index f589a0a8da39e..6caaf8553219e 100644 --- a/crates/turbo-tasks-memory/benches/scope_stress.rs +++ b/crates/turbo-tasks-memory/benches/scope_stress.rs @@ -47,7 +47,7 @@ pub fn scope_stress(c: &mut Criterion) { async move { let task = tt.spawn_once_task(async move { rectangle(a, b).strongly_consistent().await?; - Ok(unit().node) + Ok(unit()) }); tt.wait_task_completion(task, false).await } diff --git a/crates/turbo-tasks-memory/benches/stress.rs b/crates/turbo-tasks-memory/benches/stress.rs index 5ba22e0a81c72..0c6e07d6a9107 100644 --- a/crates/turbo-tasks-memory/benches/stress.rs +++ b/crates/turbo-tasks-memory/benches/stress.rs @@ -41,7 +41,7 @@ pub fn fibonacci(c: &mut Criterion) { // size >= 1 => + fib(0) = 1 // size >= 2 => + fib(1) = 2 (0..size).map(|i| fib(i, i)).try_join().await?; - Ok(unit().node) + Ok(unit()) }); tt.wait_task_completion(task, false).await.unwrap(); tt diff --git a/crates/turbo-tasks-memory/src/task.rs b/crates/turbo-tasks-memory/src/task.rs index 65f80dd1cd8f0..e0468f7ac9821 100644 --- a/crates/turbo-tasks-memory/src/task.rs +++ b/crates/turbo-tasks-memory/src/task.rs @@ -2269,7 +2269,7 @@ impl Task { read_task_id, &*turbo_tasks, ); - RawVc::TaskOutput(task).into_read::>() + unsafe { >>::from_task_id(task) } }) }) }) @@ -2280,7 +2280,9 @@ impl Task { current.add(*v); } } - Ok(Vc::>::cell(current.iter().copied().collect()).node) + Ok(Vc::into_raw(Vc::>::cell( + current.iter().copied().collect(), + ))) } pub(crate) fn read_task_collectibles( diff --git a/crates/turbo-tasks/src/lib.rs b/crates/turbo-tasks/src/lib.rs index 5b7e89aca9155..072f6192e745b 100644 --- a/crates/turbo-tasks/src/lib.rs +++ b/crates/turbo-tasks/src/lib.rs @@ -58,6 +58,7 @@ mod once_map; pub mod persisted_graph; pub mod primitives; mod raw_vc; +mod raw_vc_set; mod read_ref; pub mod registry; pub mod small_duration; diff --git a/crates/turbo-tasks/src/manager.rs b/crates/turbo-tasks/src/manager.rs index 66c0f1a3b981c..46bf7f6aa9cf0 100644 --- a/crates/turbo-tasks/src/manager.rs +++ b/crates/turbo-tasks/src/manager.rs @@ -339,16 +339,18 @@ impl TurboTasks { } /// Creates a new root task - pub fn spawn_root_task( - &self, - functor: impl Fn() -> Pin> + Send>> - + Sync - + Send - + 'static, - ) -> TaskId { - let id = self - .backend - .create_transient_task(TransientTaskType::Root(Box::new(functor)), self); + pub fn spawn_root_task(&self, functor: F) -> TaskId + where + F: Fn() -> Fut + Send + Sync + Clone + 'static, + Fut: Future>> + Send, + { + let id = self.backend.create_transient_task( + TransientTaskType::Root(Box::new(move || { + let functor = functor.clone(); + Box::pin(async move { Ok(functor().await?.node) }) + })), + self, + ); self.schedule(id); id } @@ -357,13 +359,14 @@ impl TurboTasks { /// Creates a new root task, that is only executed once. /// Dependencies will not invalidate the task. #[track_caller] - pub fn spawn_once_task( - &self, - future: impl Future> + Send + 'static, - ) -> TaskId { - let id = self - .backend - .create_transient_task(TransientTaskType::Once(Box::pin(future)), self); + pub fn spawn_once_task(&self, future: Fut) -> TaskId + where + Fut: Future>> + Send + 'static, + { + let id = self.backend.create_transient_task( + TransientTaskType::Once(Box::pin(async move { Ok(future.await?.node) })), + self, + ); self.schedule(id); id } @@ -377,7 +380,7 @@ impl TurboTasks { let result = future.await?; tx.send(result) .map_err(|_| anyhow!("unable to send result"))?; - Ok(Completion::new().node) + Ok(Completion::new()) }); // INVALIDATION: A Once task will never invalidate, therefore we don't need to // track a dependency @@ -837,7 +840,7 @@ impl TurboTasksCallApi for TurboTasks { ) -> TaskId { self.spawn_once_task(async move { future.await?; - Ok(Completion::new().node) + Ok(Completion::new()) }) } @@ -853,7 +856,7 @@ impl TurboTasksCallApi for TurboTasks { } self.spawn_once_task(async move { future.await?; - Ok(Completion::new().node) + Ok(Completion::new()) }) } @@ -867,7 +870,7 @@ impl TurboTasksCallApi for TurboTasks { this.finish_primary_job(); future.await?; this.begin_primary_job(); - Ok(Completion::new().node) + Ok(Completion::new()) }) } } diff --git a/crates/turbo-tasks/src/primitives.rs b/crates/turbo-tasks/src/primitives.rs index 209f4bae6d00b..ccb89fc209fe8 100644 --- a/crates/turbo-tasks/src/primitives.rs +++ b/crates/turbo-tasks/src/primitives.rs @@ -1,13 +1,12 @@ use std::{future::IntoFuture, ops::Deref}; use anyhow::Result; -use auto_hash_map::AutoSet; use futures::TryFutureExt; // This specific macro identifier is detected by turbo-tasks-build. use turbo_tasks_macros::primitive as __turbo_tasks_internal_primitive; use crate::{ - RawVc, TryJoinIterExt, Vc, {self as turbo_tasks}, + TryJoinIterExt, Vc, {self as turbo_tasks}, }; __turbo_tasks_internal_primitive!(()); @@ -72,7 +71,6 @@ __turbo_tasks_internal_primitive!(i64); __turbo_tasks_internal_primitive!(i128); __turbo_tasks_internal_primitive!(usize); __turbo_tasks_internal_primitive!(isize); -__turbo_tasks_internal_primitive!(AutoSet); __turbo_tasks_internal_primitive!(serde_json::Value); __turbo_tasks_internal_primitive!(Vec); diff --git a/crates/turbo-tasks/src/raw_vc.rs b/crates/turbo-tasks/src/raw_vc.rs index 820e4991ada39..1f079de8dd12b 100644 --- a/crates/turbo-tasks/src/raw_vc.rs +++ b/crates/turbo-tasks/src/raw_vc.rs @@ -63,13 +63,13 @@ pub enum RawVc { } impl RawVc { - pub fn into_read(self) -> ReadRawVcFuture> { + pub(crate) fn into_read(self) -> ReadRawVcFuture> { // returns a custom future to have something concrete and sized // this avoids boxing in IntoFuture ReadRawVcFuture::new(self) } - pub fn into_strongly_consistent_read( + pub(crate) fn into_strongly_consistent_read( self, ) -> ReadRawVcFuture> { // returns a custom future to have something concrete and sized @@ -77,7 +77,7 @@ impl RawVc { ReadRawVcFuture::new_strongly_consistent(self) } - pub fn into_trait_read( + pub(crate) fn into_trait_read( self, ) -> ReadRawVcFuture> { // returns a custom future to have something concrete and sized @@ -85,17 +85,9 @@ impl RawVc { ReadRawVcFuture::new(self) } - pub fn into_strongly_consistent_trait_read( - self, - ) -> ReadRawVcFuture> { - // returns a custom future to have something concrete and sized - // this avoids boxing in IntoFuture - ReadRawVcFuture::new_strongly_consistent(self) - } - /// INVALIDATION: Be careful with this, it will not track dependencies, so /// using it could break cache invalidation. - pub fn into_read_untracked_with_turbo_tasks( + pub(crate) fn into_read_untracked_with_turbo_tasks( self, turbo_tasks: &dyn TurboTasksApi, ) -> ReadRawVcFuture> { @@ -104,21 +96,13 @@ impl RawVc { /// INVALIDATION: Be careful with this, it will not track dependencies, so /// using it could break cache invalidation. - pub fn into_trait_read_untracked( + pub(crate) fn into_trait_read_untracked( self, ) -> ReadRawVcFuture> { ReadRawVcFuture::new_untracked(self) } - /// INVALIDATION: Be careful with this, it will not track dependencies, so - /// using it could break cache invalidation. - pub fn into_strongly_consistent_read_untracked( - self, - ) -> ReadRawVcFuture> { - ReadRawVcFuture::new_strongly_consistent_untracked(self) - } - - pub async fn resolve_trait( + pub(crate) async fn resolve_trait( self, trait_type: TraitTypeId, ) -> Result, ResolveTypeError> { @@ -154,7 +138,7 @@ impl RawVc { } } - pub async fn resolve_value( + pub(crate) async fn resolve_value( self, value_type: ValueTypeId, ) -> Result, ResolveTypeError> { @@ -191,7 +175,7 @@ impl RawVc { } /// See [`crate::Vc::resolve`]. - pub async fn resolve(self) -> Result { + pub(crate) async fn resolve(self) -> Result { let tt = turbo_tasks(); let mut current = self; let mut notified = false; @@ -208,8 +192,9 @@ impl RawVc { } } } + /// See [`crate::Vc::resolve_strongly_consistent`]. - pub async fn resolve_strongly_consistent(self) -> Result { + pub(crate) async fn resolve_strongly_consistent(self) -> Result { let tt = turbo_tasks(); let mut current = self; let mut notified = false; @@ -227,18 +212,11 @@ impl RawVc { } } - pub fn connect(&self) { + pub(crate) fn connect(&self) { let tt = turbo_tasks(); tt.connect_task(self.get_task_id()); } - pub fn is_resolved(&self) -> bool { - match self { - RawVc::TaskOutput(_) => false, - RawVc::TaskCell(_, _) => true, - } - } - pub fn get_task_id(&self) -> TaskId { match self { RawVc::TaskOutput(t) | RawVc::TaskCell(t, _) => *t, @@ -250,8 +228,7 @@ impl CollectiblesSource for RawVc { fn peek_collectibles(self) -> CollectiblesFuture { let tt = turbo_tasks(); tt.notify_scheduled_tasks(); - let set: Vc> = - tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id()); + let set = tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id()); CollectiblesFuture { turbo_tasks: tt, inner: set.into_future(), @@ -263,8 +240,7 @@ impl CollectiblesSource for RawVc { fn take_collectibles(self) -> CollectiblesFuture { let tt = turbo_tasks(); tt.notify_scheduled_tasks(); - let set: Vc> = - tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id()); + let set = tt.read_task_collectibles(self.get_task_id(), T::get_trait_type_id()); CollectiblesFuture { turbo_tasks: tt, inner: set.into_future(), @@ -349,19 +325,6 @@ impl ReadRawVcFuture { _cast: PhantomData, } } - - fn new_strongly_consistent_untracked(vc: RawVc) -> Self { - let tt = turbo_tasks(); - ReadRawVcFuture { - turbo_tasks: tt, - strongly_consistent: true, - current: vc, - untracked: true, - listener: None, - phantom_data: PhantomData, - _cast: PhantomData, - } - } } impl Future for ReadRawVcFuture { diff --git a/crates/turbo-tasks/src/raw_vc_set.rs b/crates/turbo-tasks/src/raw_vc_set.rs new file mode 100644 index 0000000000000..8f1942b3f6a07 --- /dev/null +++ b/crates/turbo-tasks/src/raw_vc_set.rs @@ -0,0 +1,24 @@ +use std::marker::PhantomData; + +use auto_hash_map::AutoSet; +// This specific macro identifier is detected by turbo-tasks-build. +use turbo_tasks_macros::primitive as __turbo_tasks_internal_primitive; + +use crate as turbo_tasks; +use crate::{RawVc, TaskId, Vc}; + +__turbo_tasks_internal_primitive!(AutoSet); + +impl Vc> { + /// Casts a `TaskId` to a `Vc>`. + /// + /// # Safety + /// + /// The `TaskId` must be point to a valid `AutoSet`. + pub unsafe fn from_task_id(task_id: TaskId) -> Self { + Vc { + node: RawVc::TaskOutput(task_id), + _t: PhantomData, + } + } +} diff --git a/crates/turbo-tasks/src/vc/mod.rs b/crates/turbo-tasks/src/vc/mod.rs index d907e89e9ef53..85ea1af9b0125 100644 --- a/crates/turbo-tasks/src/vc/mod.rs +++ b/crates/turbo-tasks/src/vc/mod.rs @@ -19,8 +19,9 @@ pub use self::{ }; use crate::{ debug::{ValueDebug, ValueDebugFormat, ValueDebugFormatString}, + registry, trace::{TraceRawVcs, TraceRawVcsContext}, - CollectiblesFuture, CollectiblesSource, RawVc, ReadRawVcFuture, ResolveTypeError, + CellId, CollectiblesFuture, CollectiblesSource, RawVc, ReadRawVcFuture, ResolveTypeError, }; /// A Value Cell (`Vc` for short) is a reference to a memoized computation @@ -40,10 +41,7 @@ pub struct Vc where T: ?Sized, { - // TODO(alexkirsz) Should be private (or undocumented), but turbo-tasks-memory needs it to be - // accessible. - #[doc(hidden)] - pub node: RawVc, + pub(crate) node: RawVc, #[doc(hidden)] pub(crate) _t: PhantomData, } @@ -296,6 +294,28 @@ impl Vc where T: ?Sized, { + /// Connects the operation pointed to by this `Vc` to the current task. + pub fn connect(vc: Self) { + vc.node.connect() + } + + /// Returns a debug identifier for this `Vc`. + pub async fn debug_identifier(vc: Self) -> Result { + let resolved = vc.resolve().await?; + let raw_vc: RawVc = resolved.node; + if let RawVc::TaskCell(_, CellId { type_id, index }) = raw_vc { + let value_ty = registry::get_value_type(type_id); + Ok(format!("{}#{}", value_ty.name, index)) + } else { + unreachable!() + } + } + + /// Returns the `RawVc` corresponding to this `Vc`. + pub fn into_raw(vc: Self) -> RawVc { + vc.node + } + /// Upcasts the given `Vc` to a `Vc>`. /// /// This is also available as an `Into`/`From` conversion. diff --git a/crates/turbopack-cli/src/build/mod.rs b/crates/turbopack-cli/src/build/mod.rs index b81424367dcb0..fa473358ba37f 100644 --- a/crates/turbopack-cli/src/build/mod.rs +++ b/crates/turbopack-cli/src/build/mod.rs @@ -143,7 +143,7 @@ impl TurbopackBuildBuilder { ) .await?; - Ok(unit().node) + Ok(unit()) }); self.turbo_tasks.wait_task_completion(task, true).await?; diff --git a/crates/turbopack-core/src/issue/mod.rs b/crates/turbopack-core/src/issue/mod.rs index 51dbd451a3cf7..c5a5a7bd40932 100644 --- a/crates/turbopack-core/src/issue/mod.rs +++ b/crates/turbopack-core/src/issue/mod.rs @@ -759,7 +759,7 @@ pub async fn handle_issues( let has_fatal = issue_reporter.report_issues( TransientInstance::new(issues.clone()), - TransientValue::new(source.node), + TransientValue::new(Vc::into_raw(source)), min_failing_severity, ); diff --git a/crates/turbopack-dev-server/src/introspect/mod.rs b/crates/turbopack-dev-server/src/introspect/mod.rs index e37d7ba6bedfb..bf5c24736b944 100644 --- a/crates/turbopack-dev-server/src/introspect/mod.rs +++ b/crates/turbopack-dev-server/src/introspect/mod.rs @@ -1,7 +1,7 @@ use std::{borrow::Cow, collections::HashSet, fmt::Display}; use anyhow::Result; -use turbo_tasks::{registry, CellId, RawVc, ReadRef, TryJoinIterExt, Vc}; +use turbo_tasks::{ReadRef, TryJoinIterExt, Vc}; use turbo_tasks_fs::{json::parse_json_with_source_context, File}; use turbopack_core::{ asset::AssetContent, @@ -103,16 +103,8 @@ impl GetContentSourceContent for IntrospectionSource { } } else { parse_json_with_source_context(path)? - } - .resolve() - .await?; - let raw_vc: RawVc = introspectable.node; - let internal_ty = if let RawVc::TaskCell(_, CellId { type_id, index }) = raw_vc { - let value_ty = registry::get_value_type(type_id); - format!("{}#{}", value_ty.name, index) - } else { - unreachable!() }; + let internal_ty = Vc::debug_identifier(introspectable).await?; fn str_or_err(s: &Result>) -> Cow<'_, str> { s.as_ref().map_or_else( |e| Cow::<'_, str>::Owned(format!("ERROR: {:?}", e)), diff --git a/crates/turbopack-ecmascript/src/lib.rs b/crates/turbopack-ecmascript/src/lib.rs index 71a68ea40ba6d..906ba188766d8 100644 --- a/crates/turbopack-ecmascript/src/lib.rs +++ b/crates/turbopack-ecmascript/src/lib.rs @@ -49,7 +49,7 @@ pub use transform::{ CustomTransformer, EcmascriptInputTransform, EcmascriptInputTransforms, OptionTransformPlugin, TransformContext, TransformPlugin, UnsupportedServerActionIssue, }; -use turbo_tasks::{trace::TraceRawVcs, RawVc, ReadRef, TryJoinIterExt, Value, ValueToString, Vc}; +use turbo_tasks::{trace::TraceRawVcs, ReadRef, TryJoinIterExt, Value, ValueToString, Vc}; use turbo_tasks_fs::{rope::Rope, FileSystemPath}; use turbopack_core::{ asset::{Asset, AssetContent}, @@ -119,7 +119,7 @@ fn modifier() -> Vc { #[derive(PartialEq, Eq, Clone, TraceRawVcs)] struct MemoizedSuccessfulAnalysis { - operation: RawVc, + operation: Vc, references: ReadRef, exports: ReadRef, async_module: ReadRef, @@ -295,7 +295,7 @@ impl EcmascriptModuleAsset { if result_value.successful { this.last_successful_analysis .set(Some(MemoizedSuccessfulAnalysis { - operation: result.node, + operation: result, // We need to store the ReadRefs since we want to keep a snapshot. references: result_value.references.await?, exports: result_value.exports.await?, @@ -310,7 +310,7 @@ impl EcmascriptModuleAsset { { // It's important to connect to the last operation here to keep it active, so // it's potentially recomputed when garbage collected - operation.connect(); + Vc::connect(*operation); return Ok(AnalyzeEcmascriptModuleResult { references: ReadRef::cell(references.clone()), exports: ReadRef::cell(exports.clone()), diff --git a/crates/turbopack-tests/tests/snapshot.rs b/crates/turbopack-tests/tests/snapshot.rs index aa449bb7059ab..2ee311ce5361b 100644 --- a/crates/turbopack-tests/tests/snapshot.rs +++ b/crates/turbopack-tests/tests/snapshot.rs @@ -156,7 +156,7 @@ async fn run(resource: PathBuf) -> Result<()> { snapshot_issues(plain_issues, out.join("issues".to_string()), &REPO_ROOT) .await .context("Unable to handle issues")?; - Ok(unit().node) + Ok(unit()) }); tt.wait_task_completion(task, true).await?; diff --git a/crates/turbopack/benches/node_file_trace.rs b/crates/turbopack/benches/node_file_trace.rs index a66c6682fc8a9..c1937bdbebca5 100644 --- a/crates/turbopack/benches/node_file_trace.rs +++ b/crates/turbopack/benches/node_file_trace.rs @@ -103,7 +103,7 @@ fn bench_emit(b: &mut Bencher, bench_input: &BenchInput) { emit_with_completion(Vc::upcast(rebased), output_dir).await?; - Ok(unit().node) + Ok(unit()) }); tt.wait_task_completion(task, true).await.unwrap(); } diff --git a/crates/turbopack/examples/turbopack.rs b/crates/turbopack/examples/turbopack.rs index 64d75a2b64b07..896d2146b737b 100644 --- a/crates/turbopack/examples/turbopack.rs +++ b/crates/turbopack/examples/turbopack.rs @@ -72,7 +72,7 @@ async fn main() -> Result<()> { let rebased = RebasedAsset::new(module, input, output); emit_with_completion(Vc::upcast(rebased), output).await?; - Ok(unit().node) + Ok(unit()) }) }); spawn({