-
-
Notifications
You must be signed in to change notification settings - Fork 888
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
kqueue: Make watcher.Close() O(n) instead of O(n^2) #233
Conversation
kqueue.go
Outdated
@@ -119,6 +121,16 @@ func (w *Watcher) Remove(name string) error { | |||
w.mu.Lock() | |||
isDir := w.paths[watchfd].isDir | |||
delete(w.watches, name) | |||
|
|||
parentName := filepath.Clean(filepath.Dir(name)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the filepath.Clean() is unneeded. From my look at https://golang.org/src/path/filepath/path.go?s=13376:13404#L452 it seems Dir() already cleans it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right! fixed
does this PR work if you try and watch |
afaict it should still work fine if you watch "/"? filepath.Dir("/") == "/". So it will add an entry to the map on Add, and remove one on Remove. |
@nicks thanks so much for this change <3 😍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two years may have passed, but we're still using this code and interested in this getting merged! Is there anything I need to do to get it approved? |
|
yep, it's rebased. we've been using this in deployed binaries for a while now with no issues. if there are tests and/or benchmarks you'd like me to add that would be helpful, happy to add them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks to work on macOS 10.15.4. I see there are some open comments, but it seems like they are largely resolved. 👍 from me.
rebased on latest main branch! |
What does this pull request do?
Fixes a performance problem in the kqueue-based implementation of watcher.
In the old implementation, watcher.Close() would clone the list of watches, then run Remove. Each run of Remove would also iterate over the full list of watches.
This indexes the watches better so that Remove doesn't need to iterate over the full list.
How should this be manually tested?
There should be no functional changes in this PR