Skip to content

[client] Replace Unsafe with Netty direct memory in ChunkedAllocationManager and expose metrics. - #4448

Open
loserwang1024 wants to merge 2 commits into
apache:mainfrom
loserwang1024:expose-memory-metircs
Open

loserwang1024 wants to merge 2 commits into
apache:mainfrom
loserwang1024:expose-memory-metircs

Conversation

@loserwang1024

Copy link
Copy Markdown
Contributor

Purpose

Linked issue: close #4447

Brief change log

Tests

API and Format

Documentation

@loserwang1024

Copy link
Copy Markdown
Contributor Author

@platinumhamburg @naivedogger , Would you like to help review this PR?

@platinumhamburg platinumhamburg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only one minor suggestion.

Comment thread fluss-common/src/main/java/org/apache/fluss/metrics/MetricNames.java Outdated
@loserwang1024

Copy link
Copy Markdown
Contributor Author

@platinumhamburg I'm thinking about switching MemoryUtil.UNSAFE.allocateMemory to ByteBuffer.allocateDirect. The main motivation is that Unsafe-allocated memory is invisible to the JVM, while direct ByteBuffers are tracked against the JVM's direct memory limit (e.g. -XX:MaxDirectMemorySize), which gives us better memory governance.

@loserwang1024 loserwang1024 changed the title [client][metrics] Expose record accumulator memory usage metrics. [client] Replace Unsafe with Netty direct memory in ChunkedAllocationManager and expose metrics. Sep 21, 2026
@loserwang1024

Copy link
Copy Markdown
Contributor Author

@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

@leonardBang
leonardBang self-requested a review September 21, 2026 09:07

@platinumhamburg platinumhamburg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Comment thread fluss-common/src/main/java/org/apache/fluss/metrics/MetricNames.java Outdated
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

@leonardBang leonardBang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from my side

@loserwang1024

loserwang1024 commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

please not merge it(still in testing)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement] Replace Unsafe with Netty direct memory in ChunkedAllocationManager and expose metrics

3 participants