Skip to content

fix(transport): route NativeDirectByteBufAllocator initial allocation through native allocator - #53

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-transport-route-nativedirectbytebufallocator-i-6db819
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-transport-route-nativedirectbytebufallocator-i-6db819

Conversation

@detail-app

@detail-app detail-app Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Change

Closes #48

NativeDirectByteBufAllocator built its direct buffers via Netty's UnpooledUnsafeDirectByteBuf/UnpooledDirectByteBuf(alloc, int, int) constructors and only overrode the one-arg allocateDirectBuffer(int) hook. In Netty 4.2.18.Final that hook runs only on the capacity(int) growth path; the constructor path resolves through the package-private two-arg allocateDirectBuffer(int, boolean), a static PlatformDependent.allocateDirect. So every freshly allocated direct buffer's bytes came from Netty/JDK direct memory, not Bemo's native allocator — the dominant, never-grown request/response case was allocated and freed entirely inside Netty's machinery, never appeared in nativeAllocatedBytes()/bemo_alloc_live, and was not freed via sized nativeFree. Introduced in d7462d6; it went undetected because the contract test only asserted native backing after growth.

Fix (JVM/Netty layer only; the Rust ABI is unchanged): construct each buffer over an already-allocated native ByteBuffer — the pattern the sibling NativeByteBuf already uses. Pre-allocate the initial Allocation via allocate(initialCapacity), hand its buffer() to the UnpooledDirectByteBuf(alloc, ByteBuffer, maxCapacity) constructor (which treats it as borrowed and does no Netty allocation), keep the one-arg override as the growth hook, and free the borrowed initial CleanableDirectBuffer exactly once in capacity(int) (after growth) or deallocate() (release), whichever happens first. Applied symmetrically to UnsafeBuffer and SafeBuffer; Allocation.clean()'s AtomicLong keeps the free-once property under cross-thread release. NativeDirectAllocatorTest gains regression assertions that a fresh directBuffer(N) and never-grown buffers are native-backed before any growth.

Validation

Linux, JDK 25 (targeting --release 22), Netty 4.2.18.Final, native lib target/debug/libbemo_ffi.so.

  • Build (make build-jvm, -Xlint:all -Werror), static analysis (tools/check_java.py: Error Prone + NullAway JSpecify, Checker Framework Regex/Format), and google-java-format all pass.
  • make test-jvm — all 20 FFM/JVM contracts pass, including NativeDirectAllocatorTest and the sibling NativeByteBufTest. The strengthened NativeDirectAllocatorTest now asserts a fresh directBuffer(N) and 256 never-grown directBuffer(1024) are native-backed via nativeAllocatedBytes() before any growth, alongside the existing growth/copy/composite/cross-thread-release checks.
  • Regression proof: with the fix reverted (test assertions kept), the test fails with AssertionError: fresh direct buffer is not backed by the Bemo native allocator; restored, it passes.
  • make test-native-image — all 3 native-image contracts pass (CapiContract, CapiTlsChannelTest, ReusableGzipTest), exercising the full transport suite through the static C API binding.
  • Ownership/lifetime (verified against the real native lib on both the default SafeBuffer and forced UnsafeBuffer paths): allocate-then-release without growth, grow-then-release, double-grow-then-release, zero-capacity→grow→release, and a 512-buffer cross-thread release all produce exact nativeAllocatedBytes() deltas with no leak or double-free.

Both FFM and Native Image paths verified; Rust ABI and version handling unchanged. (This sandbox blocks Java's posix_spawn, so openssl/gcc child processes use the JDK's FORK launch mechanism; make test-native-image also needed zlib1g-dev for ld -lz. Spawn-mechanism only — the binaries run fine and test outcomes are unaffected.)


Automatic Fixes PRs can be configured here.

@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 47.05882% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.73%. Comparing base (da8dbb6) to head (086e3e2).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...e/bemo/transport/NativeDirectByteBufAllocator.java 47.05% 17 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main      #53      +/-   ##
============================================
- Coverage     86.78%   86.73%   -0.05%     
- Complexity      616      617       +1     
============================================
  Files            60       60              
  Lines         16912    16944      +32     
  Branches        490      494       +4     
============================================
+ Hits          14677    14697      +20     
- Misses         1911     1923      +12     
  Partials        324      324              
Flag Coverage Δ
jvm 57.59% <47.05%> (+0.08%) ⬆️
rust 92.99% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@codspeed

codspeed Bot commented Oct 14, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.66%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ 3 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 4 regressed benchmarks
✅ 112 untouched benchmarks
⏩ 65 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation retain-drop[1048576] 300.8 ns 354.9 ns -15.26%
❌ Simulation retain-drop[4096] 300.8 ns 354.9 ns -15.26%
❌ Simulation retain-drop[64] 300.8 ns 354.9 ns -15.26%
❌ Simulation retain-drop[65536] 300.8 ns 354.9 ns -15.26%
⚡ Simulation overwrite[64] 575.8 ns 521.6 ns +10.38%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing detail/bug-fix/fix-transport-route-nativedirectbytebufallocator-i-6db819 (086e3e2) with main (da8dbb6)

Open in CodSpeed

Footnotes

  1. 65 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

This branch has not been deployed

No deployments
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.

[Detail Bug] Netty: NativeDirectByteBufAllocator initial direct buffers bypass Bemo native allocator

1 participant