[client] Replace Unsafe with Netty direct memory in ChunkedAllocationManager and expose metrics. - #4448
[client] Replace Unsafe with Netty direct memory in ChunkedAllocationManager and expose metrics.#4448loserwang1024 wants to merge 2 commits into
Conversation
|
@platinumhamburg @naivedogger , Would you like to help review this PR? |
c2ffa27 to
02e701a
Compare
platinumhamburg
left a comment
There was a problem hiding this comment.
LGTM, only one minor suggestion.
|
@platinumhamburg I'm thinking about switching |
…Manager and expose metrics.
02e701a to
af52a13
Compare
|
@platinumhamburg thanks for your throughout review. I have also replace Unsafe with Netty direct memory in ChunkedAllocationManager , would you like to help review it? @platinumhamburg @leonardBang @AndreaBozzo , CC |
There was a problem hiding this comment.
@loserwang1024 Thanks for the update. I reviewed the final implementation against the Arrow Netty allocator that ChunkedAllocationManager originally replaced.
The bump-pointer design still retains its intended advantage: small Arrow allocations share a small number of backing chunks instead of creating one Netty allocation per ArrowBuf. In local allocation-only measurements, the current implementation was faster for small and chunk-sized allocations, while 8 MiB and 16 MiB oversized allocations were broadly comparable with the original Arrow Netty allocator. I therefore did not find evidence of a direct-allocation performance regression relative to that baseline.
BTW, this is a pre-existing issue introduced by PR #3026 , rather than by this PR: that PR changed the convenience methods in LogRecordReadContext to internally create a ChunkedFactory, but the context only retains and closes the BufferAllocator. Closing the allocator does not close the factory. A simple allocation leaves 4 MiB retained after bufferAllocator.close() and releases it only after factory.close().
This ownership gap also affects the existing server-side call sites in KV recovery and server-side filtering. On a Cleaner-backed runtime it may delay direct-memory release until GC; on a Netty no-cleaner path it may result in a native-memory leak. I suggest tracking the ownership and explicit factory-close behavior in a separate issue rather than expanding the scope of this PR.
Overall, the changes in this PR look good to me apart from the naming nit above.
Co-Authored-By: Qoder <noreply@qoder.com> Co-Authored-By: Codex <noreply@openai.com> AI-Model: gpt-6-astra AI-Contributed/Feature: 24/24 AI-Contributed/UT: 46/46
|
please not merge it(still in testing) |
Purpose
Linked issue: close #4447
Brief change log
Tests
API and Format
Documentation