feat(kernel range reader): Add buffer pooling support for large reads on regional buckets#4850
feat(kernel range reader): Add buffer pooling support for large reads on regional buckets#4850abhishek10004 wants to merge 2 commits into
Conversation
github-actions
Bot
added
the
remind-reviewers
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request optimizes memory management for large and vectored read operations by implementing a buffer pooling mechanism. By reusing buffers via a sync.Pool, the system minimizes memory allocations and garbage collection pressure during file read operations. This feature is specifically targeted at regional buckets utilizing the kernel reader, with fallback mechanisms and proper error handling for unsupported configurations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces a sync.Pool of 1 MiB buffers to optimize read operations when using the kernel reader on regional buckets, along with a helper function LimitBuffers to reslice buffers in-place. The review feedback focuses on optimizing the pool usage to achieve zero-allocation pooling by storing pointers to fixed-size arrays instead of slice headers, ensuring buffers are returned to the pool on errors to prevent memory leaks, and downgrading an error log to debug level to avoid log flooding.
| logger.Errorf("ReadFile: buffer pool allocation is only supported for regional buckets with"+ | ||
| " kernel reader enabled (EnableKernelReader: %v, IsRapid: %v)", | ||
| fs.newConfig.FileSystem.EnableKernelReader, | ||
| fh.Inode().Bucket().BucketType().IsRapid()) |
There was a problem hiding this comment.
Logging this condition as an Errorf on every read request where op.Dst == nil can lead to excessive logging and severely degrade performance under high read workloads on zonal buckets or when the kernel reader is disabled. Since this is a normal fallback/unsupported path returning ENOTSUP, it should be logged at Debugf level or omitted entirely to avoid log flooding.
logger.Debugf("ReadFile: buffer pool allocation is only supported for regional buckets with kernel reader enabled (EnableKernelReader: %v, IsRapid: %v)",
fs.newConfig.FileSystem.EnableKernelReader,
fh.Inode().Bucket().BucketType().IsRapid())
abhishek10004
force-pushed
the
abhishek/sync_pool_changes
branch
from
e8dda77 to
e54d532
Compare
| op.Data = resp.Data | ||
| op.Callback = resp.Callback | ||
| if useReadBufferPool { | ||
| if len(resp.Data) > 0 { |
There was a problem hiding this comment.
why all this logic. why can't the read create buffers based on the need.
There was a problem hiding this comment.
I'm not very clear on the ask here.
IIUC, you're asking why do we have this buffer pool here instead of creating the buffers in kernel range reader. There are a couple of reasons for that.
- If we have to read from inode (in case there are local changes), then we need to allocate buffers for that which would be outside the kernel range reader implementation.
- In future, if we extend this read capability to other read paths as well, having the buffer pool at this layer would readily allow us to do that.
If you're asking about the additional handling on the returned response: Yes, I'm doing additional handling here (since currently kernel range reader does not allocate buffers on its own or sets any callback) but that is being done for future proofing the code. The ReadResponse struct says readers can return data directly from their internal buffers via Data field or can set a callback. We're not doing that currently but I've added the logic so that in case something changes in the readers (let's say we can get buffers from upper layers etc.), it would be handled automatically here without any additional changes.
Let me know if you meant something else.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4850 +/- ##
==========================================
+ Coverage 83.67% 83.69% +0.02%
==========================================
Files 170 170
Lines 20948 21006 +58
==========================================
+ Hits 17528 17582 +54
- Misses 2763 2766 +3
- Partials 657 658 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @raj-prince, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
| fs.newConfig.FileSystem.EnableKernelReader, | ||
| fh.Inode().Bucket().BucketType().IsRapid()) | ||
| fh.Inode().Unlock() | ||
| return syscall.ENOTSUP |
There was a problem hiding this comment.
Just to confirm, customer will never face this scenario. Given we are enabling the MaxRead only in case of regional bucket & kernel reader.
|
Closing this PR as we're moving towards lazy allocation based approach for the buffers - #4852 |
3 participants
Description
Description
This PR introduces buffer pooling for large and vectored read operations to optimize memory usage and reduce garbage collection overhead. By reusing allocated buffers via a sync pool, we minimize memory churn during sequential and large file reads.
Note
Buffer pooling support for larger reads is only supported for regional buckets when kernel reader is enabled.
Key Changes
Buffer Pooling & Lifecycle Management (
internal/fs/fs.go)readBufferPoolinto read operations to dynamically allocate and recycle byte buffers for read requests (supported only for regional buckets with kernel reader enabled).Buffer Limiting Utility (
internal/util/util.go)LimitBuffers(buffers [][]byte, limit int) [][]byte.Testing (
internal/fs/large_read_regional_test.go,internal/util/util_test.go)LimitBufferscovering boundary conditions, multiple buffer spans, and edge cases.large_read_regional_test.go) verifying successful vectored reads on regional buckets and confirming proper fallback/error behavior (ENOTSUP) for zonal buckets or when kernel reader support is disabled.Link to the issue in case of a bug fix.
b/530765312
Testing details
Any backward incompatible change? If so, please explain.