Use shared FSEvents streams#4447
Open
jakebailey wants to merge 6 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the macOS FSEvents watcher implementation to reduce the number of FSEvents streams created per process by sharing (and chunking) streams across multiple logical directory watches, addressing failures when many --watch instances run in parallel and hit system/process stream limits.
Changes:
- Reworks the FSEvents backend to maintain a shared set of stream “chunks” (up to
fseventsPathsPerStreamwatch roots per stream) and rebuild them on subscribe/close. - Updates FSEvents callback routing to deliver each event to all matching logical watches instead of a single per-watch stream.
- Adds darwin-only tests to verify stream sharing and correct event routing/isolation between sibling watches.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/fswatch/fsevents_darwin.go | Implements shared/chunked stream management and routes callback events to matching watches. |
| internal/fswatch/fsevents_darwin_shared_test.go | Adds tests asserting a shared stream is used and that events route only to the correct watch. |
| internal/fswatch/fsevents_darwin_ffi.go | Updates stream callback wiring to reference the backend (for shared routing) rather than a single dirWatch. |
Member
Author
|
I'm still doing some testing here; it seems like perf-wise that it's actually bad to chunk, but we still need to make sure to not overflow one stream with too many paths, and therefore do need to at least make more than one if needed. But, the setup time for the stream is the major factor here, not adding paths in. |
Comment on lines
+403
to
+405
| streams, err := b.startStreams(watches) | ||
| if err != nil { | ||
| b.mu.Lock() |
Comment on lines
+470
to
+471
| watches := cb.backend.activeWatchesSnapshot(cb.paths) | ||
| touched := map[*dirWatch]struct{}{} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4441
Parcel's watcher would create a single fsevents stream. But, there's a max stream count per process and per system. Since our watch code is a port of it, we inherited that problem.
VS Code had been working around this by registering parent dir streams. This is what #4379 does up to a point.
libuv, what we used via Node previously, avoids this problem by only ever making one fsevents stream per event loop. That makes it much harder to hit the global max.
The problem with reusing streams for more than one watch is that you have to provide the list of dirs to watch at create time. Adding another one means you have to create a whole new stream.
This PR tries to take a hybrid approach, instead chunking fsevents streams so we still have more than one, but way fewer, so can avoid the limit quite a bit more.