[ricardoasouz]

Summary

Part of #10444. This adds EXECUTORCH_USE_GENERIC_JNI, an opt-in build option that is OFF by default, and uses it to move ExecuTorchRuntime and Tensor from fbjni to standard JNI. With the option OFF the bindings build from the same fbjni code paths as on main, so the default build keeps its current behavior. libfbjni.so stays a dependency in both modes, since Module, LlmModule, TrainingModule and SGD still use HybridClass.

The option is exposed as a CMake option, as the executorch.use_generic_jni Buck config (applied to every target that compiles the JNI sources, selective_jni_target included), and as an environment variable for scripts/build_android_library.sh.

With the option ON, ExecuTorchRuntime's natives are exported as Java_* symbols, which version_script.txt already keeps global, instead of being registered through fbjni. JTensor also becomes a plain JavaClass: as a HybridClass its C++ instance held no state, since the constructor discarded its argument and the HybridData attached to each Java Tensor was never read back. Native code then creates tensors through a nativeNewTensor overload without that parameter, so no C++ object is allocated for each tensor returned to Java. The dim order support from #23820 is carried into both paths.

Review order: the first commit moves the JTensor and JEValue declarations into jni_layer_types.h and defines the members out of line in jni_layer.cpp. jni_layer.cpp and jni_layer_training.cpp each declared these classes with different definitions, which violated the ODR, and the training translation unit depended on jni_layer.cpp emitting the inline definitions. The function bodies are unchanged; the size of the diff comes from indentation and the TensorHybrid → JTensor rename. The second commit adds the option and the ExecuTorchRuntime path, and the third adds the Tensor path. In both, the existing fbjni code stays as it is under #else.

The Buck changes can't be built in OSS because fb_android_cxx_library is Meta-only, so they are worth a check during the import.

Test plan

On 8aae748, based on a84676e:

Built with scripts/build_android_library.sh for x86_64 with EXECUTORCH_USE_GENERIC_JNI unset (OFF) and set to ON; the Gradle build (lint and spotless) and the unit tests pass in both. With ON, llvm-nm -D lists Java_org_pytorch_executorch_ExecuTorchRuntime_getRegisteredOps and getRegisteredBackends; with OFF they are absent. libfbjni.so is NEEDED in both.

Instrumentation tests on an x86_64 API 30 emulator (RuntimeInstrumentationTest, ModuleInstrumentationTest, ModuleE2ETest, TrainingModuleE2ETest): 27/29 pass with OFF and 27/29 with ON. The two failures are testMissingPteFile and testMissingPtdFile, whose expected messages are stale on main and are updated in #23480.

ModuleInstrumentationTest#testNonPteFile is excluded from those runs because it aborts the test process, on main (a84676e) as well. The abort comes from fbjni's getJavaExceptionForCppException receiving a null std::exception_ptr (abort message ptr), called from translatePendingCppExceptionToJavaException. libexecutorch.so links libc++ statically while the prebuilt libfbjni.so uses libc++_shared, which would explain it. This is independent of this PR.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22959

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ You can merge normally! (1 Unrelated Failure)

As of commit 8aae748 with merge base a84676e ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

• Test Vulkan Backend (specialized runners) / test-model-vulkan-e2e / linux-job (gh) (trunk failure)
  RuntimeError: Command bash /__w/_temp/exec_script failed with exit code 1

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[ricardoasouz]

@pytorchbot label "release notes: none"

[nil-is-all]

Hi @ricardoasouz could you update the branch and give a ping when ready for review?

[kirklandsign]

@claude review

[claude]

Claude finished @kirklandsign's task in 1m 54s —— View job

---

The direction looks right, and I found no correctness bugs; the remaining items are a style mismatch in jni_layer_runtime.cpp and a request to run the existing instrumentation tests on a device.

Detailed review

PR Review: #22959

Summary

The PR moves ExecuTorchRuntime from fbjni registerNatives to exported Java_* symbols. It also removes the stateless HybridData from Tensor, so TensorHybrid becomes a plain JavaClass called JTensor. I didn't find any correctness bugs:

• version_script.txt already exports Java_*.
• Nothing else in the repo refers to register_natives_for_runtime, TensorHybrid or mHybridData on Tensor.
• The new nativeNewTensor signature matches on both sides.

Code Quality

• jni_layer_runtime.cpp:47-85, 89-101: the structure doesn't match the other plain-JNI files.
  • get_registered_ops / get_registered_backends have external linkage in namespace executorch_jni and are only called by one-line Java_* wrappers.
  • jni_layer_asr.cpp and jni_layer_image.cpp put the logic directly in the JNIEXPORT function.
  • Suggested fix: merge each helper into its Java_* function, or at least move it into the anonymous namespace. CLAUDE.md asks for no single-use helpers.
• jni_layer.cpp:54 / jni_layer_training.cpp:33: JTensor is defined differently in two translation units. This is an ODR violation that was already there with TensorHybrid, and the rename keeps it.
  • jni_layer_training.cpp only works because the inline member bodies in jni_layer.cpp happen to be emitted as linkable symbols. LTO or more aggressive inlining could drop them.
  • On your question: yes, a shared header for JTensor/JEValue would fix this, not just shrink the diff. Landing it as its own commit before the Module step makes sense.
• Tensor.kt:699-710: with the mHybridData assignment gone, the val tensor = when {...}; return tensor temporary is no longer needed. return when {...} is enough.

Testing

• The validation is static only. llvm-nm and javap show the symbols and signatures exist, but don't show the lazy Java_* binding resolves at runtime. Please run on a device or emulator:
  • RuntimeInstrumentationTest (calls getRegisteredOps/getRegisteredBackends).
  • The Module/EValue instrumentation tests, which send output tensors through nativeNewTensor.
  • The training instrumentation tests, if the build includes training.
• A wrong signature now shows up as UnsatisfiedLinkError / NoSuchMethodError at call time. Before, it failed in JNI_OnLoad.

Recommendation

Approve once the instrumentation tests pass on a device. Nothing above blocks the change; the helper restructuring is a small style fix.

[ricardoasouz]

@nil-is-all @kirklandsign Updated the branch again after main moved; it is mergeable now and ready for review.

I reran the Android validation locally:

• x86_64 executorch_jni build with training enabled: pass
• spotlessCheck, Android build, and unit tests: pass
• API 30 x86_64 instrumentation: 28/30, with only the two known stale missing-file assertions in TrainingModuleE2ETest
• after updating those two expected error strings locally to match ExecuTorchRuntime.validateFilePath, TrainingModuleE2ETest passes 4/4

The test fix remains separate in #23480.

The shared JTensor/JEValue header is exported through jni_headers. I also validated the jni_headers target with the repository-pinned OSS Buck2. The OSS shim does not provide the Meta-only fb_android_cxx_library macro, so I supplied the equivalent local compatibility wrapper only for target analysis; jni_headers then built successfully. The native Android build separately compiled and linked both JNI translation units using the shared header.

The branch is now stable at ceb88a95 and I don't plan any further pushes. The workflows for this head are currently waiting for approval, and the Meta internal Diff is out of sync with the PR after the latest force-push.

Could you re-sync the internal Diff with this head and approve the pending workflows here and on #23480 so CI can run?

[meta-codesync]

@psiddh has imported this pull request. If you are a Meta employee, you can view this in D123563084.

[psiddh]

Thanks @ricardoasouz for the contribution. Moving this into "Request changes" state as we need to validate this against internal Apps surface and make sure it passes internal review and CI. Thanks again and I will get back on this PR with clear next steps.

[psiddh]

@ricardoasouz Can we introduce a --disable-jni flag or some such (default value is off), If this flag is off, we continue to rely on fbjni , if enabled, rely standard JNI. that way , with the flag disabled, there is no change to existing functionality with minimum to no risk of regression. ?

[ricardoasouz]

@ricardoasouz Can we introduce a --disable-jni flag or some such (default value is off), If this flag is off, we continue to rely on fbjni , if enabled, rely standard JNI. that way , with the flag disabled, there is no change to existing functionality with minimum to no risk of regression. ?

@psiddh Done — I added an opt-in EXECUTORCH_USE_GENERIC_JNI feature flag.

The default is OFF, so the existing fbjni implementation remains the default path. Enabling the flag switches ExecuTorchRuntime and Tensor to the standard JNI implementation.

The flag is wired through both CMake and Buck. For the public Android build script, c++_shared is selected only when generic JNI is enabled; the default/OFF build keeps the existing STL configuration unchanged.

Validation after rebasing onto current main:

• default/OFF build preserves the upstream fbjni build behavior
• generic JNI ON build succeeds and exports the expected Java_* runtime symbols
• ModuleInstrumentationTest#testNonPteFile passes with generic JNI enabled
• spotlessCheck passes
• Android unit tests pass
• git diff --check is clean

The new head is 8de9880270. The Meta Internal-Only Changes Check currently reports that the Diff and PR are out of sync, and the workflows are awaiting maintainer approval. Could you please re-import/sync the internal Diff and approve the workflows when convenient?

[ricardoasouz]

@ricardoasouz Can we introduce a --disable-jni flag or some such (default value is off), If this flag is off, we continue to rely on fbjni , if enabled, rely standard JNI. that way , with the flag disabled, there is no change to existing functionality with minimum to no risk of regression. ?

@psiddh @nil-is-all The requested opt-in EXECUTORCH_USE_GENERIC_JNI flag is in place, and the branch has since been rebased onto current main.

The current head is ab5fefab and is ready for review. The workflows are still awaiting maintainer approval.

Thanks!

[ricardoasouz]

Hi @psiddh @nil-is-all @kirklandsign,

The latest head (cd2a8a0) includes the requested opt-in EXECUTORCH_USE_GENERIC_JNI feature flag, with the default set to OFF to preserve the existing fbjni behavior.

The CI has completed with 202 successful checks, including the Android build and emulator tests.

There are two remaining blockers:

1. The Windows unittest-editable job failed in OpRandnTest.Rank, where the standard deviation differed from 1.0 by 0.102295 against a tolerance of 0.1. This appears to be a possible statistically flaky test unrelated to the JNI changes. I attempted to rerun the job, but GitHub returned HTTP 403 due to repository permissions. Could someone please rerun it?

2. The required Meta Internal-Only Changes Check reports that the internal Diff and PR are out of sync, while Import Status is still queued. Could the latest head be re-synced/imported for internal validation?

No further code changes are planned unless internal validation or review identifies an issue.

Thanks for your help!

[nil-is-all]

Hi @psiddh @nil-is-all @kirklandsign,

The latest head (cd2a8a0) includes the requested opt-in EXECUTORCH_USE_GENERIC_JNI feature flag, with the default set to OFF to preserve the existing fbjni behavior.

The CI has completed with 202 successful checks, including the Android build and emulator tests.

There are two remaining blockers:

1. The Windows unittest-editable job failed in OpRandnTest.Rank, where the standard deviation differed from 1.0 by 0.102295 against a tolerance of 0.1. This appears to be a possible statistically flaky test unrelated to the JNI changes. I attempted to rerun the job, but GitHub returned HTTP 403 due to repository permissions. Could someone please rerun it?
2. The required Meta Internal-Only Changes Check reports that the internal Diff and PR are out of sync, while Import Status is still queued. Could the latest head be re-synced/imported for internal validation?

No further code changes are planned unless internal validation or review identifies an issue.

Thanks for your help!

Thanks for the ping, @ricardoasouz. Could you update the out-of-date branch? I'll run CI and re-import the latest commit internally to do a final check.

[ricardoasouz]

@nil-is-all Thanks! I've updated the branch by rebasing onto the latest main.

The new HEAD is 998514ec (0 commits behind main), and GitHub reports the PR as mergeable. It's ready for the CI rerun and internal re-import/final check.

Thanks for your help!

[ricardoasouz]

@psiddh @kirklandsign @larryliu0820 @nil-is-all I rebuilt this PR on main (new head 8aae748, based on a84676e). Every commit now keeps the default build unchanged: the new paths only exist under #ifdef EXECUTORCH_USE_GENERIC_JNI, and with the option OFF (the default) the existing fbjni code is compiled as on main. Before, the first commits removed fbjni and the last one restored it behind the flag.

This is the opt-in flag psiddh suggested on Oct 7. I named it EXECUTORCH_USE_GENERIC_JNI rather than --disable-jni because JNI itself stays, and fbjni is still used by Module, LlmModule, TrainingModule and SGD; only the bindings migrated here switch. I'm happy to rename it if you prefer another name.

Two other changes since the last revision: the Buck flag now also reaches selective_jni_target, which compiles the same JNI sources, and the build script no longer switches the STL when the option is ON, so both modes build with the same C++ runtime as main. The dim order support from #23820 is carried into both paths. The description now has the review order and the validation in both modes: 27/29 instrumentation tests pass in each, and the two failures are the stale assertions fixed in #23480.

While validating, I found that ModuleInstrumentationTest#testNonPteFile aborts the test process on main as well (details in the description). It is unrelated to this change; I'm happy to open a separate issue for it.

The earlier run-emulator failure (adb: device offline) was infrastructure. Could you approve the workflows and re-import the internal Diff? I don't plan further pushes unless review or internal validation asks for changes.