feat(java): add Panama FFM fast-path for hot byte ops#358
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds a Rust C-ABI FFI fast path (ffi_c.rs) exposing enqueue, enqueue_many, get_result, and free exports for Java's Panama/FFM on JDK 22+. Introduces a NativeTransport abstraction with FfmTransport and JniTransport implementations, updates JniQueueBackend to route through it, adds Gradle Multi-Release JAR build support, and adds parity tests. ChangesFFM Fast Path Implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JniQueueBackend
participant NativeTransport
participant FfmTransport
participant RustFFI as Rust taskito_ffi_*
JniQueueBackend->>NativeTransport: create(handle)
NativeTransport->>FfmTransport: reflective create(handle)
alt FFM unavailable
NativeTransport-->>JniQueueBackend: JniTransport fallback
end
JniQueueBackend->>NativeTransport: enqueue(task, payload, options)
NativeTransport->>FfmTransport: enqueue(...)
FfmTransport->>RustFFI: taskito_ffi_enqueue(handle, args)
RustFFI-->>FfmTransport: status + job id bytes
FfmTransport-->>JniQueueBackend: job id string
PoemA rabbit hops through native land, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
sdks/java/src/test/java/org/byteveda/taskito/internal/TransportParityTest.java (1)
22-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider failing hard instead of skipping when FFM should be active.
assumeFalsetreats "JDK <22, FFM absent" and "JDK 22+, FFM overlay present but broken" identically — both just skip. Since the Gradletesttask only addsjava22.outputto the classpath when building on JDK 22+, a broken FFM init on such a run would silently report "skipped" rather than failing, hiding a real regression on the exact JDK this feature targets.♻️ Suggested tightening
`@Test` void ffmSelectedOnJava22() { NativeTransport transport = NativeTransport.create(0L); - assumeFalse(transport instanceof JniTransport, "FFM transport not active on this JDK"); - assertEquals("FfmTransport", transport.getClass().getSimpleName()); + assumeTrue(Runtime.version().feature() < 22 || !(transport instanceof JniTransport), + "FFM transport unexpectedly inactive on JDK 22+"); + if (Runtime.version().feature() >= 22) { + assertEquals("FfmTransport", transport.getClass().getSimpleName()); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdks/java/src/test/java/org/byteveda/taskito/internal/TransportParityTest.java` around lines 22 - 27, The ffmSelectedOnJava22 test currently skips in both “JDK <22” and “JDK 22+ but FFM broken” cases because of assumeFalse, which can hide regressions. Update TransportParityTest.ffmSelectedOnJava22 to explicitly check the runtime/JDK condition before creating NativeTransport, and only skip when FFM is genuinely unavailable on this JVM; if the JDK 22+ FFM path should be active, let the test fail when NativeTransport.create(0L) does not produce an FfmTransport. Keep the existing class-name assertion and use the NativeTransport/JniTransport/FfmTransport types to make the intent clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdks/java/build.gradle.kts`:
- Around line 158-168: The native-access manifest setting in the Jar task is
being described too broadly for SDK consumers. Update the comment near the
manifest attributes in jar so it clearly states that Enable-Native-Access:
ALL-UNNAMED only applies when the artifact is launched directly with java -jar,
not when used as a classpath dependency; also add a note in the Java SDK README
that downstream users must pass --enable-native-access=ALL-UNNAMED themselves.
- Around line 145-146: The FFM overlay is currently controlled by
JavaVersion.current() in build.gradle.kts, so the java22 source set only
activates when Gradle itself runs on JDK 22+, which prevents the Multi-Release
overlay from being included in the published jar during the JDK 21 publish
workflow. Update the ffmCapable gating so the java22 overlay is driven by a JDK
22+ toolchain or change the release job to run on JDK 22+, and keep the relevant
build logic around the java22 source set and Multi-Release packaging aligned
with that choice.
In `@sdks/java/src/main/java22/org/byteveda/taskito/internal/FfmTransport.java`:
- Around line 211-223: The take(MemorySegment outData, MemorySegment outLen)
helper currently frees the native buffer only after the byte[] copy succeeds, so
a failure in the reinterpret/toArray path can leak the Rust allocation. Update
the copy/free flow in FfmTransport.take so the taskito_ffi_free call (via
FREE.invokeExact) runs in a finally block after reading the pointer and length,
ensuring the native buffer is always released even if copying throws.
---
Nitpick comments:
In
`@sdks/java/src/test/java/org/byteveda/taskito/internal/TransportParityTest.java`:
- Around line 22-27: The ffmSelectedOnJava22 test currently skips in both “JDK
<22” and “JDK 22+ but FFM broken” cases because of assumeFalse, which can hide
regressions. Update TransportParityTest.ffmSelectedOnJava22 to explicitly check
the runtime/JDK condition before creating NativeTransport, and only skip when
FFM is genuinely unavailable on this JVM; if the JDK 22+ FFM path should be
active, let the test fail when NativeTransport.create(0L) does not produce an
FfmTransport. Keep the existing class-name assertion and use the
NativeTransport/JniTransport/FfmTransport types to make the intent clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f35cdbb-19f9-45e2-99fd-8d3e41d776f6
📒 Files selected for processing (9)
crates/taskito-java/src/ffi_c.rscrates/taskito-java/src/lib.rssdks/java/build.gradle.ktssdks/java/src/main/java/org/byteveda/taskito/internal/JniQueueBackend.javasdks/java/src/main/java/org/byteveda/taskito/internal/JniTransport.javasdks/java/src/main/java/org/byteveda/taskito/internal/NativeTransport.javasdks/java/src/main/java22/org/byteveda/taskito/internal/FfmTransport.javasdks/java/src/test/java/org/byteveda/taskito/FfmRoundTripTest.javasdks/java/src/test/java/org/byteveda/taskito/internal/TransportParityTest.java
pratyush618
changed the title
1 participant
What
Adds a Project Panama (FFM) fast-path for the hot native byte ops —
enqueue,enqueueMany,getResult— selected automatically on JDK 22+, with JNI as the default and fallback. Perf-only and additive: the public floor stays Java 17 and every other native call keeps using JNI.How
internal.NativeTransportseam over the three hot ops.create(handle)reflectively resolves the FFM overlay on JDK 22+ (via the multi-release jar) and falls back toJniTransporton anyReflectiveOperationException/LinkageError— the seam can never break the 17 floor.FfmTransport(overlay,src/main/java22) calls newtaskito_ffi_*C-ABI exports throughjava.lang.foreign: confinedArenaper call, off-heap args, results copied out then freed viataskito_ffi_free. Batch payloads/ids cross as one length-prefixed LE frame.ffi_c.rs—#[no_mangle] extern "C"exports parallel to the jni-rs surface, sharing one cdylib and the sameQueueHandle. Every body is panic-guarded (catch_unwind) so a panic never unwinds across FFI; each returns anOK/ERR/ABSENTstatus with a heap buffer the caller frees.--release 17; on a build JDK ≥ 22 the FFM overlay compiles at--release 22intoMETA-INF/versions/22. Manifest getsMulti-Release: true+Enable-Native-Access: ALL-UNNAMED. Older build JDKs simply omit the overlay — same public API, faster impl where available (not feature divergence).Testing
TransportParityTest— JNI and FFM agree on all three hot ops over one shared handle (binary payload spanning every byte value); skips the FFM assertions on JDKs without the overlay.FfmRoundTripTest— end-to-end payload fidelity (empty, full-Unicode, 256 KiB) through the active transport, plus the batch path../gradlew buildon JDK 22: Spotless/Checkstyle + suite green; both FFM tests ran (not skipped) exercising the FFM path; verifiedMETA-INF/versions/22/.../FfmTransport.classand the manifest attrs in the jar. Rustfmt/check/clippyclean.Summary by CodeRabbit
New Features
Bug Fixes