kqueue: remove timeout from unix.Kevent() #39
Merged
Conversation
This file contains 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
The timeout for unix.Kevent() is causing issues; every 100ms it will do a new unix.Kevent() syscall, which isn't too efficient: even if you have just one change an hour, you will still keep calling kevent() ten times per second, resulting in a needlessly high CPU usage. Without a timeout, kevent() will block indefinitely until there are some events, which is much more efficient. We can't just remove the timout however, since we can't interrupt the kevent() call on FreeBSD and NetBSD, and it will hang forever. This issue is described in more detail here: fsnotify/fsnotify#262 (comment) To solve this we register a new kevent() with the file descriptor set to the closepipe; when this pipe is closed an event is sent and kevent() will stop blocking. This is a rebased version of #124. Fixes #89 Fixes #237 Fixes #333 Supersedes and closes #124 Supersedes and closes #262 Supersedes and closes #334 Co-authored-by: Felix Lange <fjl@twurst.com>
WalkthroughThe codebase has undergone a transformative update focused on enhancing time handling and streamlining closing operations. This revamp includes a shift from the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- kqueue.go (12 hunks)
Additional comments: 10
kqueue.go (10)
- 25-28: The introduction of the
closepipefield in theWatcherstruct is a crucial part of the new mechanism to unblock thekevent()call. This change is well-aligned with the PR's objective to reduce CPU usage by eliminating constant polling. However, it's important to ensure that theclosepipeis properly initialized and closed to avoid resource leaks.- 46-53: The changes in the
NewWatcherfunction to initialize thekqandclosepipevariables are correctly implemented. This setup is essential for the new blocking behavior ofunix.Kevent()to work as intended. It's good to see error handling in place for thekqueue()call, ensuring that resources are not leaked if an error occurs during initialization.- 89-90: Closing the write-end of the
closepipein theClosemethod is a critical step to trigger an event that unblocks thekevent()call. This implementation effectively addresses the challenge of interrupting indefinitely blocking syscalls on FreeBSD and NetBSD systems. It's important to verify that the read-end of theclosepipeis also closed to prevent resource leaks.- 114-115: The use of the
registerfunction to unregister a watch by usingunix.EV_DELETEis correctly implemented. This is an important part of managing the lifecycle of watches, ensuring that resources are properly released when a watch is no longer needed. It's crucial to ensure that the file descriptor is closed after unregistering the watch to avoid file descriptor leaks.- 238-239: The call to
registerwithunix.EV_ADD|unix.EV_CLEAR|unix.EV_ENABLEflags during the addition of a watch is correctly implemented. This setup is necessary for the new event handling mechanism to function properly. It's important to ensure that error handling is in place to close the file descriptor in case the registration fails, preventing resource leaks.- 275-284: The deferred function in
readEventsto close thekqandclosepipefile descriptors and to close thedone,Events, andErrorschannels is a good practice. This ensures that resources are properly cleaned up when the event reading loop terminates. However, it's crucial to handle potential errors from these close operations, especially for the file descriptors, to ensure that any issues are properly logged or handled.- 286-311: The logic to terminate the event reading loop upon receiving a close event from the
closepipeis correctly implemented. This mechanism is essential for the new approach to unblockingkevent()calls. It's important to ensure that all events are processed before terminating the loop to avoid losing any events.- 491-522: The
kqueuefunction's implementation, which creates a new kernel event queue and registers an event on theclosepipe, is crucial for the new mechanism to work as intended. This setup allowskevent()to block indefinitely without polling, while still being able to be unblocked when necessary. It's important to ensure that all resources are properly cleaned up in case of errors to prevent leaks.- 525-534: The
registerfunction's implementation to register events with the queue is correctly done. Usingunix.SetKeventto set up the events and then callingunix.Keventto register them ensures that the watcher can monitor the specified events. Proper error handling in case the registration fails is crucial to avoid leaving the system in an inconsistent state.- 544-545: The simplification of the
readfunction by removing timeout handling and allowing it to block indefinitely until events occur is in line with the PR's objectives. This change contributes to reducing CPU usage by eliminating constant polling. It's important to ensure that error handling is robust, especially for cases whereunix.Keventmight return errors other than those expected.
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.
port fsnotify/fsnotify#480
The timeout for unix.Kevent() is causing issues; every 100ms it will do a new
unix.Kevent() syscall, which isn't too efficient: even if you have just one
change an hour, you will still keep calling kevent() ten times per second,
resulting in a needlessly high CPU usage.
Without a timeout, kevent() will block indefinitely until there are some events,
which is much more efficient. We can't just remove the timout however, since we
can't interrupt the kevent() call on FreeBSD and NetBSD, and it will hang
forever. This issue is described in more detail here:
fsnotify/fsnotify#262 (comment)
To solve this we register a new kevent() with the file descriptor set to the
closepipe; when this pipe is closed an event is sent and kevent() will stop
blocking.
Summary by CodeRabbit