-
Notifications
You must be signed in to change notification settings - Fork 345
flate: objects allocation optimization #1106
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
base: master
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughIntroduces a pooled tokens object ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as compressLoop
participant Pool as tokensPool
participant Enc as statelessEnc
participant Dict as optional dict
Caller->>Pool: Get() -> dst
note right of Pool #e6f7ff: pooled *tokens returned
Pool-->>Caller: dst
Caller->>Caller: dst.Reset()
alt no dict
Caller->>Enc: statelessEnc(dst, src)
Enc-->>Caller: compressed block produced
else with dict
Caller->>Dict: access dict
Caller->>Enc: statelessEnc(dst, src, dict)
Enc-->>Caller: compressed block produced
end
Caller->>Pool: Put(dst) (deferred)
note right of Caller #f0ffe6: dst returned to pool after block processing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
flate/stateless.go (1)
64-69
: Consider usingany
for consistency.The
tokensPool
correctly follows the sync.Pool pattern established bybitWriterPool
. However, for consistency with line 59, consider usingany
instead ofinterface{}
as the return type.Apply this diff:
// tokensPool contains tokens struct objects that can be reused var tokensPool = sync.Pool{ - New: func() interface{} { + New: func() any { return &tokens{} }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
flate/stateless.go
(4 hunks)
🔇 Additional comments (3)
flate/stateless.go (3)
100-104
: LGTM! Proper sync.Pool usage pattern.The pooling implementation correctly follows best practices:
- Obtains a tokens instance from the pool
- Resets it immediately to ensure clean state
- Uses defer to guarantee return to the pool even on early returns or errors
This change aligns with the PR's goal of reducing allocations, as confirmed by the benchmark improvements showing dramatic allocation reductions (from ~542KB/op down to 8 B/op in some cases).
128-128
: LGTM! Consistent usage of pooled dst.The changes correctly adapt to dst being a pooled
*tokens
instance:
- Lines 128, 130:
statelessEnc
calls passdst
directly (not&dst
), matching the function signature at line 176- Line 144:
writeBlockDynamic
receivesdst
as*tokens
- Line 150:
dst.Reset()
correctly prepares the pooled instance for reuse in the next loop iterationThe integration maintains correctness while enabling the allocation optimizations described in the PR.
Also applies to: 130-130, 144-144, 150-150
64-69
: No other tokens allocations found. All pointer instantiations oftokens
in production code occur viatokensPool
; other occurrences are value declarations or in tests and don’t require pooling.
After updating GO to v1.24+, a sharp increase in CPU utilization was detected. Heap profile helped to reveal increased memory allocations by Write and Close methods of stateless gzip.Writer mode. This PR optimizes problem area by using sync.Pool and later allocation of tokens object.
Benchmarks:
BEFORE
AFTER
Summary by CodeRabbit