kqueue: Make watcher.Close() O(n) instead of O(n^2) #233
Conversation
kqueue.go
Outdated
| isDir := w.paths[watchfd].isDir | ||
| delete(w.watches, name) | ||
|
|
||
| parentName := filepath.Clean(filepath.Dir(name)) |
There was a problem hiding this comment.
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.
|
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 😍 |
|
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. |
theckman
left a comment
theckman
left a comment
There was a problem hiding this comment.
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! |
7 participants
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