Remove unecessary heap allocations when processing Epoll events#612
Open
mtjhrc wants to merge 3 commits intocontainers:mainfrom
Open
Remove unecessary heap allocations when processing Epoll events#612mtjhrc wants to merge 3 commits intocontainers:mainfrom
mtjhrc wants to merge 3 commits intocontainers:mainfrom
Conversation
This allows for platform-specific optimizations that need mutable access to internal buffers during wait. Since Epoll is Clone, callers that need to call wait from separate contexts can use a clone. Update all call sites to use `let mut epoll` / `fn work(mut self)`. Signed-off-by: Matej Hrica <mhrica@redhat.com>
mtjhrc
requested review from
MatiasVara,
dorindabassey,
jakecorrenti,
slp and
tylerfanelli
as code owners
Collaborator
Author
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes the event loops in various virtio workers by moving event vector allocations outside of the main loops and updating the Epoll::wait method to require mutable access. The macOS implementation of Epoll was refactored to reuse an internal buffer, reducing per-call allocation overhead. A high-severity issue was identified in the macOS wait implementation where the max_events parameter is not validated against the provided events slice length, which could lead to an out-of-bounds panic.
mtjhrc
force-pushed
the
dont-alloc-epoll-events
branch
2 times, most recently
from
524a418 to
a3e0e78
Compare
Replace the per-call Vec allocation in Epoll::wait with a Vec field that is allocated lazily, resized and reused across wait calls. Implement Clone manually so that each clone gets its own fresh empty buffer. This also fixes a bug: the old code sized the kevent buffer to events.len() but passed max_events as the kernel limit. The correct size is max_events, since that is what the kernel uses. In practice all call sites pass the same value for both, so no behavioral change. Signed-off-by: Matej Hrica <mhrica@redhat.com>
Move the epoll_events Vec allocation before the loop in all virtio worker threads (block, fs, net, snd, vsock muxer). The buffer is reused across iterations since wait() overwrites entries and all callers only iterate epoll_events[0..ev_cnt]. Signed-off-by: Matej Hrica <mhrica@redhat.com>
mtjhrc
force-pushed
the
dont-alloc-epoll-events
branch
from
a3e0e78 to
7132d95
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This removes unecessary heap allocations during event loop processing.
This definitely shows up in a profiler on macOS significantly (I was looking at virtio-net), though the high variance between runs makes it hard to measure a concrete improvement. Either way, it's easy to avoid the unnecessary work.