-
-
Notifications
You must be signed in to change notification settings - Fork 636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prototype switching to the notify
crate for file invalidation
#4999
Comments
thinking that this is probably a longer-term goal in the face of our current priorities, but agree it would be nice to fold filesystem event monitoring directly into the daemon if we can do it as good or better than watchman. we should probably also attempt to make a case upstream for feature flagging the behavior we want if that's the primary driving factor here. |
### Problem Watchman `4.9.0` does not send events for a parent when a child is created or deleted, which causes issues like #4662. ### Solution Revert to explicitly invalidating the parent directory when a child has changed. More precision is possible here, but it's possible that re-gaining the precision by using the `notify` crate would be preferable (see #4999). ### Result Fixes #4662.
Another relevant bit here is that |
Assigning @jsirois : updated the description to make it clear that even if we decide to ship this, we'll want to support toggling between watchman and native-notify via a global flag. |
@illicitonion : Assuming you're not taking this one, let's leave it assigned to @jsirois . @jsirois : If you're able to work on this one next week, it would be much appreciated. I can lay some groundwork this week to unblock you (basically, moving the "fork lock" to the rust side in order to make the background thread that @illicitonion determined would be necessary here viable). |
Yeah, enjoy @jsirois! |
Ack. I'll be back on it tomorrow morning. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Notes about API: It would be nice to just be able to ask for changes since X for a root. Even better would be where X is not a time but a token and an initial scan of a root returns a token. This would allow any weirdness with time to be held behind the API. |
I kicked off a small branch for this over the holiday: master...twitter:stuhood/notify-crate ... it does not yet work, but it compiles. It has a number of important TODOs before it is landable:
The first thing is purely a performance improvement, but I'd suggest looking at it first because it will make debugging the second thing easier not to have to wait long periods for pantsd to start up. Fixing those two things would leave us in a place where we are using two systems to invalidate, but hopefully in a way that has no noticeable impact for end users. Then, in a second PR, we should allow for watchman to be disabled, most likely by making edits to the python-side FSEventService
|
Keith got a draft of this out here: #9089: it contains parts of the first two items above. Thanks Keith! |
The implementation of notify watcher in #9318 leads to some user noticeable performance hits for users who don't have pantsd enabled. It's possible that there are some improvements we can make by ignoring files that are already gitignored that don't need to be watched for events. This would be an ad-hoc solution to make notify faster for us but does nothing for consumers, who would likely have to do the same thing. (Link to issue about ignoring based on project .gitignore?) Questions that will affect how we move forward:
Next Steps |
Without having had looked closely at the implementation, that does indeed sound difficult and error-prone to implement. I would think either notify or watchman would be sufficient? If our goal is to remove |
Error prone yes, just because watchman is already error-prone, and we would have to maintain more code. Difficult, not really, currently in #9318 the default is to use both simultaneously. They both hook in to the same code path in the engine, but we get redundant invalidation. |
Based on changes implemented in 3f52ee5 to improve latency of setting up file watches it seems like we are no longer between a rock and a hard place with how to move forward! Watching the entire build root at startup time adds minimal overhead to pants (#9318 (comment)). We can safely land #9318 without any penalty to users, so we don't need to put it behind a feature gate. This means the next steps are much clearer: Revised Next Steps
|
### Problem Using the notify crate for file invalidation is now feature complete, and so we are using two file watching implementations simultaneously under `pantsd`. ### Solution Disable watchman by default, and deprecate it. Additionally, remove the implementation of the `experimental` flag from notify. Fixes #8710, fixes #8198, fixes #7375, fixes #5487, fixes #4999, fixes #3479.
Watchman no longer gives us useful events when the children of directories are created/deleted, which is a large and surprising change in behaviour that means we need to over-invalidate (or add additional syscalls to differentiate creates/updates/deletes).
We should explore whether using something like the notify crate would result in fewer assumptions... we already know that it would result in one fewer daemon and less memory usage (as we have no need to track anything that isn't warm in the graph).
The primary uncertainty is whether consuming a queue of events in process would require filtering to decide which events to ignore, and which require additional syscalls to determine their actual meaning: for example:
accesstime
updates?Also, even if this looks like a good approach, it should likely be enabled behind a pants global option, so that both watchman and the notify crate can be used at once (EDIT at @illicitonion's suggestion) for a while.
The text was updated successfully, but these errors were encountered: