Turbopack: Rework watcher data structures, store watched list and watcher behind the same RwLock (#82258)

Reworks the data structures used for the watcher to achieve a few goals:

- I want to avoid multiple locks, as I don't think we actually need two different locks, and multiple locks leaves more room for bugs (e.g. lock inversion).
- The information about if we're in non-recursive mode or recursive mode needs to be available outside of the lock for performance reasons.
- The `watching`/`watched` set needs to be ordered. We don't have a convenient concurrent BTreeSet or Trie/Radix Tree pulled in, and I'd like to avoid adding more dependencies. We end up locking `notify_watcher` anywhere we'd need a write lock anyways, so it might block a few reads, but otherwise it's not that bad.

Fixes in this PR:

- Fixes an issue where multiple concurrent calls to `ensure_watching` for the same directory could cause one of those calls to exit early. This may cause a file in that directory to be read before the filesystem watcher is set up, potentially missing events.
- Improves the time complexity of `restore_if_watching` using an ordered set to avoid iterating over all of `watched` to find child directories. See `OrderedPathSetExt`. This time complexity problem is introduced in the previous PR, #82130.

Other changes:

- It turns out that we don't need to explicitly set up any watchers during `start_watching` except for the `root_dir`. If paths have already been read, they'll set up watchers before re-reading the file when we invalidate it! This simplifies things, and means there's less stuff we need to track if you never start the watcher (i.e. `next build`).
- Added a `--start-watching-late` option to the fuzzer to test calling `start_watching` after all the initial reads happen, to help validate the safety of the claim in the previous bullet point.

Minor (aesthetic) changes:

- Use past tense: `watching` is now `watched` and `ensure_watching` is now `ensure_watched`, as the `watched` list includes both currently active paths *and* previously watched but deleted paths.
- Shorten the names of non-public types, because we have a lot more of them, and the names would get long otherwise

## Benchmarking

This replaces a `DashSet<_>` with (essentially) a `RwLock<BTreeSet<_>>`. There's some risk of increased contention, particularly in the read-only fast-path for `ensure_watched`/`ensure_watching`.

Under the assumption that the most heavily lock-contended use-case for this code is after restoring from persistent cache on a large repository:

1. Check out `vercel-site` in `front`.
2. Hack the new tracing span onto the canary version.
3. Build a release version with `pnpm pack-next --project ~/front/apps/vercel-site --no-js-build -- --release`
4. `rm -rf .next`
5. `NEXT_TURBOPACK_TRACING='turbo_tasks_fs' TURBO_ENGINE_IGNORE_DIRTY=1 pnpm next dev --turbopack`
6. Load `http://localhost:3000/vercel`. Wait for it to fully finish loading. Stop the server with `ctrl+c`.
7. Restart the server, wait for it to finish initializing, and then exit.
8. Start the trace viewer: `pnpm next internal trace .next/trace-turbopack`, and look at the aggregated times of `ensure_watched`/`ensure_watching`.

Results:

before (canary): 313ms ensure_watching (2235x spans)
after (this pr): 279ms ensure_watched (2235x spans)

So no significant difference.

Author: bgw

turbopack/crates/turbo-tasks-fs/src/lib.rs

@@ -261,11 +261,7 @@ impl DiskFileSystemInner {
         let invalidator = turbo_tasks::get_invalidator();
         self.invalidator_map
             .insert(path.to_owned(), invalidator, None);
-        if let Some(non_recursive) = &self.watcher.non_recursive_state
-            && let Some(dir) = path.parent()
-        {
-            non_recursive.ensure_watching(&self.watcher, dir, self.root_path())?;
-        }
+        self.watcher.ensure_watched_file(path, self.root_path())?;
         Ok(())
     }
 
@@ -291,11 +287,7 @@ impl DiskFileSystemInner {
             .collect::<Vec<_>>();
         invalidators.insert(invalidator, Some(write_content));
         drop(invalidator_map);
-        if let Some(non_recursive) = &self.watcher.non_recursive_state
-            && let Some(dir) = path.parent()
-        {
-            non_recursive.ensure_watching(&self.watcher, dir, self.root_path())?;
-        }
+        self.watcher.ensure_watched_file(path, self.root_path())?;
         Ok(old_invalidators)
     }
 
@@ -305,9 +297,7 @@ impl DiskFileSystemInner {
         let invalidator = turbo_tasks::get_invalidator();
         self.dir_invalidator_map
             .insert(path.to_owned(), invalidator, None);
-        if let Some(non_recursive) = &self.watcher.non_recursive_state {
-            non_recursive.ensure_watching(&self.watcher, path, self.root_path())?;
-        }
+        self.watcher.ensure_watched_dir(path, self.root_path())?;
         Ok(())
     }
 

turbopack/crates/turbo-tasks-fs/src/path_map.rs

@@ -1,5 +1,5 @@
 use std::{
-    collections::{BTreeMap, btree_map::CursorMut},
+    collections::{BTreeMap, BTreeSet, btree_map, btree_set},
     ops::Bound,
     path::{Path, PathBuf},
 };
@@ -9,24 +9,31 @@ use std::{
 ///
 /// In the future, this may use a more efficient representation, like a radix tree or trie.
 pub trait OrderedPathMapExt<V> {
-    fn extract_path_with_children<'a>(&'a mut self, path: &'a Path) -> ExtractWithChildren<'a, V>;
+    fn extract_path_with_children<'a>(
+        &'a mut self,
+        path: &'a Path,
+    ) -> PathMapExtractPathWithChildren<'a, V>;
 }
 
 impl<V> OrderedPathMapExt<V> for BTreeMap<PathBuf, V> {
-    fn extract_path_with_children<'a>(&'a mut self, path: &'a Path) -> ExtractWithChildren<'a, V> {
-        ExtractWithChildren {
+    /// Iterates over and removes `path` and all of its children.
+    fn extract_path_with_children<'a>(
+        &'a mut self,
+        path: &'a Path,
+    ) -> PathMapExtractPathWithChildren<'a, V> {
+        PathMapExtractPathWithChildren {
             cursor: self.lower_bound_mut(Bound::Included(path)),
             parent_path: path,
         }
     }
 }
 
-pub struct ExtractWithChildren<'a, V> {
-    cursor: CursorMut<'a, PathBuf, V>,
+pub struct PathMapExtractPathWithChildren<'a, V> {
+    cursor: btree_map::CursorMut<'a, PathBuf, V>,
     parent_path: &'a Path,
 }
 
-impl<V> Iterator for ExtractWithChildren<'_, V> {
+impl<V> Iterator for PathMapExtractPathWithChildren<'_, V> {
     type Item = (PathBuf, V);
 
     fn next(&mut self) -> Option<Self::Item> {
@@ -44,20 +51,61 @@ impl<V> Iterator for ExtractWithChildren<'_, V> {
     }
 }
 
+/// A thin wrapper around [`BTreeSet<PathBuf>`] that provides efficient iteration of child paths.
+///
+/// In the future, this may use a more efficient representation, like a radix tree or trie.
+pub trait OrderedPathSetExt {
+    /// Iterates over the children of `path`, excluding `path` itself.
+    fn iter_path_children<'a>(&'a mut self, path: &'a Path) -> PathSetIterPathChildren<'a>;
+}
+
+impl OrderedPathSetExt for BTreeSet<PathBuf> {
+    fn iter_path_children<'a>(&'a mut self, path: &'a Path) -> PathSetIterPathChildren<'a> {
+        PathSetIterPathChildren {
+            // this is range written weirdly due to type inference limitations:
+            // https://stackoverflow.com/a/66130898
+            range: self.range::<Path, _>((Bound::Excluded(path), Bound::Unbounded)),
+            parent_path: path,
+        }
+    }
+}
+
+pub struct PathSetIterPathChildren<'a> {
+    // we don't need the nightly cursors API for this, the `Range` type is sufficient.
+    range: btree_set::Range<'a, PathBuf>,
+    parent_path: &'a Path,
+}
+
+impl<'a> Iterator for PathSetIterPathChildren<'a> {
+    type Item = &'a Path;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        // this simple implementation works because `Path` implements `Ord` (and `starts_with`)
+        // using path component comparision, rather than raw byte comparisions. The parent path is
+        // always guaranteed to be placed immediately before its children (pre-order traversal).
+        let current_path = self.range.next()?;
+        if !current_path.starts_with(self.parent_path) {
+            return None;
+        }
+        Some(&**current_path)
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
 
     #[test]
-    fn test_extract_with_children() {
+    fn test_map_extract_path_with_children() {
         let mut map = BTreeMap::default();
         map.insert(PathBuf::from("a"), 1);
         map.insert(PathBuf::from("a/b"), 2);
         map.insert(PathBuf::from("a/b/c"), 3);
         map.insert(PathBuf::from("a/b/d"), 4);
-        map.insert(PathBuf::from("a/c"), 5);
-        map.insert(PathBuf::from("x/y/z"), 6);
-        map.insert(PathBuf::from("z/a/b"), 7);
+        map.insert(PathBuf::from("a/b/d/e"), 5);
+        map.insert(PathBuf::from("a/c"), 6);
+        map.insert(PathBuf::from("x/y/z"), 7);
+        map.insert(PathBuf::from("z/a/b"), 8);
 
         let parent_path = PathBuf::from("a/b");
         let extracted: Vec<_> = map.extract_path_with_children(&parent_path).collect();
@@ -66,15 +114,39 @@ mod tests {
             (PathBuf::from("a/b"), 2),
             (PathBuf::from("a/b/c"), 3),
             (PathBuf::from("a/b/d"), 4),
+            (PathBuf::from("a/b/d/e"), 5),
         ];
         assert_eq!(extracted, expected_extracted);
 
         let mut expected_remaining = BTreeMap::new();
         expected_remaining.insert(PathBuf::from("a"), 1);
-        expected_remaining.insert(PathBuf::from("a/c"), 5);
-        expected_remaining.insert(PathBuf::from("x/y/z"), 6);
-        expected_remaining.insert(PathBuf::from("z/a/b"), 7);
+        expected_remaining.insert(PathBuf::from("a/c"), 6);
+        expected_remaining.insert(PathBuf::from("x/y/z"), 7);
+        expected_remaining.insert(PathBuf::from("z/a/b"), 8);
 
         assert_eq!(map, expected_remaining);
     }
+
+    #[test]
+    fn test_set_iter_path_children() {
+        let mut set = BTreeSet::default();
+        set.insert(PathBuf::from("a"));
+        set.insert(PathBuf::from("a/b"));
+        set.insert(PathBuf::from("a/b/c"));
+        set.insert(PathBuf::from("a/b/d"));
+        set.insert(PathBuf::from("a/b/d/e"));
+        set.insert(PathBuf::from("a/c"));
+        set.insert(PathBuf::from("x/y/z"));
+        set.insert(PathBuf::from("z/a/b"));
+
+        let parent_path = PathBuf::from("a/b");
+        let iterated: Vec<_> = set.iter_path_children(&parent_path).collect();
+
+        let expected_iterated = vec![
+            PathBuf::from("a/b/c"),
+            PathBuf::from("a/b/d"),
+            PathBuf::from("a/b/d/e"),
+        ];
+        assert_eq!(iterated, expected_iterated);
+    }
 }