feat(turborepo): Process package change events asynchronously #8036
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
8 Ignored Deployments
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @NicholasLYang and the rest of your teammates on |
🟢 Turbopack Benchmark CI successful 🟢Thanks |
NicholasLYang
changed the title
| } | ||
|
|
||
| Err(Error::ConnectionClosed) | ||
| }; | ||
|
|
||
| let run_fut = async { | ||
| loop { | ||
| if !changed_packages.lock().unwrap().borrow().is_empty() { |
There was a problem hiding this comment.
There's a slight race condition here, but it's okay, since the amount of changed packages is always going to increase, not decrease (it's only decremented in this thread), so we'll always need to do a run. Also even if we don't run anything, it'll just be slightly inefficient.
🟢 CI successful 🟢Thanks |
| if !changed_packages.lock().unwrap().borrow().is_empty() { | ||
| self.execute_run(&changed_packages).await?; | ||
| } | ||
| yield_now().await; |
There was a problem hiding this comment.
Can you document what's going on here? I'm guessing it's to avoid spinning and not receiving any new events...
Maybe there's a better way to structure that? Like a watch or something so we can block until there's a change?
There was a problem hiding this comment.
Yeah we could probably do this with a watch channel. Will give that a shot
There was a problem hiding this comment.
I think with the watch, we don't need this line anymore, right?
| let run_fut = async { | ||
| loop { | ||
| changed_pkgs_rx.changed().await?; | ||
| let changed_pkgs = changed_pkgs_rx.borrow_and_update(); |
There was a problem hiding this comment.
I think this will block the producing side until changed_pkgs is dropped. You may want to clone it so that the borrow is short-lived, or at least not held for the duration of a run.
There was a problem hiding this comment.
Docs on borrowing here indicate that this is not recommended, and we should find a way to shorten the borrow.
| if !changed_packages.lock().unwrap().borrow().is_empty() { | ||
| self.execute_run(&changed_packages).await?; | ||
| } | ||
| yield_now().await; |
There was a problem hiding this comment.
I think with the watch, we don't need this line anymore, right?
NicholasLYang
force-pushed
the
nicholasyang/dedupe-events
branch
from
06b115d
to
78f33a5
Compare
### Description Like #8036, we need to split out the event processing loop from the actual logic to avoid lagging channels. In this case we process the changed files every 100ms. Uses a `Trie` to store the paths since they'll likely have a lot of overlap. ### Testing Instructions <!-- Give a quick description of steps to test your changes. --> Closes TURBO-2921
Description
We were previously processing the package change events synchronously, which ended up creating too much lagging and therefore crashes. Now we spin up a thread that reads in the package change events, and accumulates the changed packages. A separate thread handles execution by taking those changed packages and spinning up a new run.
Also I changed our logic to just send rediscover events on lagging with the channel in the daemon server. No reason to crash the server in that case.
The first commit is more of a refactor to put the watch state into
WatchClient, to clean up the state ahead of splitting into two threads. You can review commit by commit if you wish.Testing Instructions
Tested on
next.js, lagging is definitely improved.