Improve daemon startup times #7322

arlyon · 2024-02-08T19:42:01Z

Description

This is a PR for speeding up the daemon startup and setting up good practices for differentiating between data and service dependencies. To clarify the difference, file watching service should start up immediately in an unavailable state, while things that need it can immediately take dependencies on its 'potentially-available' data. This also has a side-effect of fixing up our package discovery watcher as it makes the data flow a lot cleaner and ensures that the daemon can report data is unavailable when it is recalculating (using FutureExt::now_or_never) as well as cascading unavailable when parts of the system are no longer up to date. If a dependency wishes to wait for the data, then it can do so by simply awaiting the future. Great we've reimplemented async salsa by hand... though our data-dependency-graph will probably not really ever be so large that it outgrows this DIY solution.

The part that specifically handles package discovery availability is demonstrated here:

turbo/crates/turborepo-filewatch/src/package_watcher.rs

Lines 589 to 593 in 048d5f5

    
           // make sure package data is unavailable while we are updating 
        
           // if this fails, we have no subscribers so we can just exit 
        
           if self.package_data_tx.send(None).is_err() { 
        
               return; 
        
           }

Downstream dependencies who attempt a 'now or never' request on the data will receive an empty value, which translates to package discovery being unavailable

turbo/crates/turborepo-filewatch/src/package_watcher.rs

Lines 43 to 63 in 048d5f5

    
           async fn discover_packages(&self) -> Result<DiscoveryResponse, discovery::Error> { 
        
               tracing::debug!("discovering packages using watcher implementation"); 
        
               // this can either not have a value ready, or the sender has been dropped. in 
        
               // either case rust report that the value is unavailable 
        
               let package_manager = self 
        
                   .watcher 
        
                   .get_package_manager() 
        
                   .and_then(Result::ok) 
        
                   .ok_or(discovery::Error::Unavailable)?; 
        
               let workspaces = self 
        
                   .watcher 
        
                   .get_package_data() 
        
                   .and_then(Result::ok) 
        
                   .ok_or(discovery::Error::Unavailable)?; 
        
               Ok(DiscoveryResponse { 
        
                   workspaces, 
        
                   package_manager, 
        
               }) 
        
           }

This mechanism also powers package hash discovery, which depends on multiple components all being active to produce accurate answers.

Testing Instructions

Existing tests should suffice

Closes TURBO-2296

vercel · 2024-02-08T19:42:14Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
examples-svelte-web	🔄 Building (Inspect)	Visit Preview	💬 Add feedback	Feb 14, 2024 11:16pm
rust-docs	❌ Failed (Inspect)			Feb 14, 2024 11:16pm

9 Ignored Deployments

Name	Status	Preview	Updated (UTC)
examples-basic-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-designsystem-docs	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-gatsby-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-kitchensink-blog	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-native-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-nonmonorepo	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-tailwind-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
examples-vite-web	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm
turbo-site	⬜️ Ignored (Inspect)	Visit Preview	Feb 14, 2024 11:16pm

github-actions · 2024-02-08T19:43:55Z

🟢 Turbopack Benchmark CI successful 🟢

Thanks

github-actions · 2024-02-08T19:53:10Z

🟢 CI successful 🟢

Thanks

NicholasLYang

Overall looks quite good. I am a little concerned about the potential for deadlocking with the use of OptionalWatch. Are we ever holding a Ref across an await?

crates/turborepo-filewatch/Cargo.toml

crates/turborepo-filewatch/src/cookies.rs

crates/turborepo-filewatch/src/optional_watch.rs

arlyon · 2024-02-12T16:50:26Z

Tokio requires all futures to be Send meaning that we the compiler ensures that a Ref is never held across an await.

gsoltis

Initial Review, primarily on cookies.rs and lib.rs / file-watching startup. Will continue to review more of the PR, but wanted to make sure these comments get out.

crates/turborepo-filewatch/src/optional_watch.rs

crates/turborepo-filewatch/src/cookies.rs

crates/turborepo-filewatch/src/globwatcher.rs

crates/turborepo-filewatch/src/lib.rs

gsoltis

A few more comments. Going to wait on reviewing package_watcher.rs more thoroughly since I don't think this PR re-enables its use.

crates/turborepo-filewatch/src/optional_watch.rs

gsoltis · 2024-02-12T21:38:50Z

crates/turborepo-lib/src/daemon/server.rs

+        oneshot::Sender<()>,
+        JoinHandle<Result<(), WatchError>>,
+    ) {
+        let file_watching = FileWatching::new(repo_root.clone(), package_discovery_backup).unwrap();


Maybe kind of a nit, but I'm not sure moving filewatching startup into the Grpc server new makes sense. We'll have to move it out again if we ever want to share it with any other communication mechanism.

Is there a reason to move it in here?

Good point. Really, everything can move out (package discovery watcher, and file watcher) when the time comes. Since we are creating the package discovery in here also I think it makes sense but it makes sense to move it out if/when we decide to provide alternative APIs

crates/turborepo-filewatch/src/package_watcher.rs

NicholasLYang

I feel like we need some naming conventions around these types. Like types wrapped OptionalWatch could be suffixed with _lazy or something similar. And maybe it'd make sense to diagram or explain somewhere which values the different structs take in and which they sent out.

crates/turborepo-lib/src/daemon/server.rs

NicholasLYang · 2024-02-13T21:39:26Z

crates/turborepo-filewatch/src/package_watcher.rs

+
+        let package_json_path = repo_root.join_component("package.json");
+
+        let _task = tokio::spawn({


I'm a little confused. What is this task doing that watch cannot do? Like this appears to be initializing everything, but shouldn't that happen before the loop in watch?

I think it is nice to colocate. I have experimented with both, so I don't really have a strong opinion on which one we go with. Happy to have that be a design decision in the docs we are putting together.

crates/turborepo-filewatch/src/lib.rs

arlyon · 2024-02-14T22:50:16Z

Ok, changed convention from _rx and _tx to _lazy and _tx

Most uses actually do not need mutability anyways, and those that do can often mitigate it quite trivially.

This also makes generous use of a new type called an OptionalWatch which allows downstream data dependencies to wait for a resource to become available. All data dependencies are wrapped in an OptionalWatch, and all initialization is strictly synchronous.

gsoltis · 2024-02-15T00:38:05Z

crates/turborepo-lib/src/daemon/server.rs

+        // cleans up root watching task.
+        let (exit_root_watch, root_watch_exit_signal) = oneshot::channel();
+        let watch_root_handle = tokio::task::spawn(watch_root(
+            file_watching.clone(),


Not sure if it matters too much for this particular service, but shouldn't watch_root subscribe to the OptionalWatch for filewatching like the other services? If not, we should at least leave a comment as to why this one is different

You can only get a stream of events from the underlying FileSystemWatcher using

turbo/crates/turborepo-filewatch/src/lib.rs

Lines 168 to 175 in 0f78ba6

/// A convenience method around the sender watcher that waits for file

/// watching to be ready and then returns a handle to the file stream.

pub async fn subscribe(

&self,

) -> Result<broadcast::Receiver<Result<Event, NotifyError>>, RecvError> {

let mut receiver = self.receiver.clone();

receiver.get().await.map(|r| r.resubscribe())

}

which goes through the optional watch.

Open to passing the underlying OptionalWatch to the services rather than just passing in FileWatching directly to make the data dependencies more clear

gsoltis

Alright, let's merge this and continue on, once @NicholasLYang approves too.

arlyon requested a review from a team as a code owner February 8, 2024 19:42

arlyon requested review from gsoltis and NicholasLYang and removed request for a team February 8, 2024 19:42

turbo-orchestrator bot added created-by: turborepo owned-by: turborepo labels Feb 8, 2024

arlyon force-pushed the fix/daemon-startup branch from 0fea88a to a8f92ea Compare February 8, 2024 19:55

vercel bot had a problem deploying to Preview – rust-docs February 8, 2024 19:56 Failure

Base automatically changed from gsoltis/downstream_cookie to main February 8, 2024 21:56

arlyon force-pushed the fix/daemon-startup branch from a8f92ea to 04fd1c6 Compare February 9, 2024 15:59

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 16:00 Failure

arlyon force-pushed the fix/daemon-startup branch from 04fd1c6 to 6ca8395 Compare February 9, 2024 17:10

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 17:11 Failure

arlyon changed the title ~~Fix/daemon startup~~ Improve daemon startup times Feb 9, 2024

arlyon force-pushed the fix/daemon-startup branch from 6ca8395 to 048d5f5 Compare February 9, 2024 17:57

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 17:58 Failure

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 18:19 Failure

arlyon force-pushed the fix/daemon-startup branch from 3ca3f2a to 1a90d25 Compare February 9, 2024 18:23

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 18:25 Failure

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 18:28 Failure

arlyon force-pushed the fix/daemon-startup branch from 0df85ed to 3efde3d Compare February 9, 2024 18:51

vercel bot had a problem deploying to Preview – rust-docs February 9, 2024 18:53 Failure

NicholasLYang requested changes Feb 12, 2024

View reviewed changes

crates/turborepo-filewatch/Cargo.toml Outdated Show resolved Hide resolved

crates/turborepo-filewatch/src/cookies.rs Show resolved Hide resolved

crates/turborepo-filewatch/src/optional_watch.rs Show resolved Hide resolved

arlyon force-pushed the fix/daemon-startup branch from 3efde3d to 4929932 Compare February 12, 2024 17:01

vercel bot had a problem deploying to Preview – rust-docs February 12, 2024 17:02 Failure

arlyon force-pushed the fix/daemon-startup branch from 4929932 to 1b73c2c Compare February 12, 2024 17:07

gsoltis requested changes Feb 12, 2024

View reviewed changes

vercel bot had a problem deploying to Preview – rust-docs February 13, 2024 18:56 Failure

NicholasLYang reviewed Feb 13, 2024

View reviewed changes

refactor: make package discovery use interior mutability

5dc951d

Most uses actually do not need mutability anyways, and those that do can often mitigate it quite trivially.

vercel bot had a problem deploying to Preview – rust-docs February 14, 2024 22:58 Failure

arlyon and others added 12 commits February 14, 2024 22:59

refactor: documentation and clean up

ea6d59b

fix compile on macOS

73b0a76

var naming

7e90eb2

remove unneeded mutex

388ee29

apply the cookie timeout to the entire request

b66de20

don't allow queuing commands before the globwatcher is ready

1470eb1

fix lint

a2192bd

remove pointless watcher and clarify error exiting

be94c23

clean up naming conventions for optionalwatch

eccfb27

remove unneeded comments

7583122

fix build on macOS

ddedf28

arlyon force-pushed the fix/daemon-startup branch from ffa666e to ddedf28 Compare February 14, 2024 22:59

vercel bot had a problem deploying to Preview – rust-docs February 14, 2024 23:00 Failure

rebase fixes

0f78ba6

vercel bot had a problem deploying to Preview – rust-docs February 14, 2024 23:16 Failure

gsoltis reviewed Feb 15, 2024

View reviewed changes

NicholasLYang self-requested a review February 15, 2024 17:43

gsoltis approved these changes Feb 15, 2024

View reviewed changes

NicholasLYang approved these changes Feb 15, 2024

View reviewed changes

arlyon merged commit fa16459 into main Feb 15, 2024
57 of 58 checks passed

arlyon deleted the fix/daemon-startup branch February 15, 2024 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve daemon startup times #7322

Improve daemon startup times #7322

arlyon commented Feb 8, 2024 •

edited

vercel bot commented Feb 8, 2024 •

edited

github-actions bot commented Feb 8, 2024 •

edited

github-actions bot commented Feb 8, 2024 •

edited

NicholasLYang left a comment

arlyon commented Feb 12, 2024

gsoltis left a comment

gsoltis left a comment

gsoltis Feb 12, 2024

arlyon Feb 15, 2024

NicholasLYang left a comment

NicholasLYang Feb 13, 2024

arlyon Feb 14, 2024 •

edited

arlyon commented Feb 14, 2024

gsoltis Feb 15, 2024

arlyon Feb 15, 2024 •

edited

arlyon Feb 15, 2024

gsoltis left a comment

	// make sure package data is unavailable while we are updating
	// if this fails, we have no subscribers so we can just exit
	if self.package_data_tx.send(None).is_err() {
	return;
	}

	async fn discover_packages(&self) -> Result<DiscoveryResponse, discovery::Error> {
	tracing::debug!("discovering packages using watcher implementation");

	// this can either not have a value ready, or the sender has been dropped. in
	// either case rust report that the value is unavailable
	let package_manager = self
	.watcher
	.get_package_manager()
	.and_then(Result::ok)
	.ok_or(discovery::Error::Unavailable)?;
	let workspaces = self
	.watcher
	.get_package_data()
	.and_then(Result::ok)
	.ok_or(discovery::Error::Unavailable)?;

	Ok(DiscoveryResponse {
	workspaces,
	package_manager,
	})
	}


		let package_json_path = repo_root.join_component("package.json");

		let _task = tokio::spawn({

	/// A convenience method around the sender watcher that waits for file
	/// watching to be ready and then returns a handle to the file stream.
	pub async fn subscribe(
	&self,
	) -> Result<broadcast::Receiver<Result<Event, NotifyError>>, RecvError> {
	let mut receiver = self.receiver.clone();
	receiver.get().await.map(\|r\| r.resubscribe())
	}

Improve daemon startup times #7322

Improve daemon startup times #7322

Conversation

arlyon commented Feb 8, 2024 • edited

Description

Testing Instructions

vercel bot commented Feb 8, 2024 • edited

github-actions bot commented Feb 8, 2024 • edited

🟢 Turbopack Benchmark CI successful 🟢

github-actions bot commented Feb 8, 2024 • edited

🟢 CI successful 🟢

NicholasLYang left a comment

Choose a reason for hiding this comment

arlyon commented Feb 12, 2024

gsoltis left a comment

Choose a reason for hiding this comment

gsoltis left a comment

Choose a reason for hiding this comment

gsoltis Feb 12, 2024

Choose a reason for hiding this comment

arlyon Feb 15, 2024

Choose a reason for hiding this comment

NicholasLYang left a comment

Choose a reason for hiding this comment

NicholasLYang Feb 13, 2024

Choose a reason for hiding this comment

arlyon Feb 14, 2024 • edited

Choose a reason for hiding this comment

arlyon commented Feb 14, 2024

gsoltis Feb 15, 2024

Choose a reason for hiding this comment

arlyon Feb 15, 2024 • edited

Choose a reason for hiding this comment

arlyon Feb 15, 2024

Choose a reason for hiding this comment

gsoltis left a comment

Choose a reason for hiding this comment

arlyon commented Feb 8, 2024 •

edited

vercel bot commented Feb 8, 2024 •

edited

github-actions bot commented Feb 8, 2024 •

edited

github-actions bot commented Feb 8, 2024 •

edited

arlyon Feb 14, 2024 •

edited

arlyon Feb 15, 2024 •

edited