[js/rn] Migrate to JSI implementation #25764
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hans00
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the React Native ONNX Runtime implementation from a bridge-based architecture to a JSI (JavaScript Interface) implementation to achieve zero-copy data transfer. This eliminates the need for serializing tensors through the React Native bridge and removes the dependency on blob storage for tensor data.
Key changes include:
- Complete rewrite using JSI for direct JavaScript-native communication
- Removal of blob-based tensor data transfer mechanism
- New C++ JSI binding layer with async operation support
- Simplified native module interfaces for both iOS and Android
Reviewed Changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| js/react_native/package.json | Updates package dependencies, removes buffer dependency, adds cpp source files |
| js/react_native/lib/*.ts | Rewrites TypeScript bindings to use JSI API instead of bridge calls |
| js/react_native/cpp/*.{h,cpp} | New C++ JSI implementation with tensor utilities, session management, and async workers |
| js/react_native/ios/*.{h,mm} | Simplifies iOS module to only install JSI bindings |
| js/react_native/android/src/main/java/* | Removes Java tensor helpers and bridge methods, adds JSI installation |
| js/react_native/android/CMakeLists.txt | Updates build configuration for JSI compilation |
Files not reviewed (1)
- js/react_native/package-lock.json: Language not supported
Comments suppressed due to low confidence (1)
js/react_native/lib/backend.ts:98
- The initialization logic should handle the case where OrtApi.initOrtOnce has already been called. Consider checking if initialization has already occurred to avoid potential reinitialization issues.
OnnxruntimeSessionHandler.#initialized = true;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for (size_t i = 0; i < size; ++i) { | ||
| auto item = array.getValueAtIndex(runtime, i); | ||
| static_cast<char**>(data)[i] = | ||
| strdup(item.toString(runtime).utf8(runtime).c_str()); |
There was a problem hiding this comment.
The strdup call allocates memory that is never freed, causing a memory leak. Consider using a RAII approach or ensuring proper cleanup of allocated string memory.
Suggested change
| strdup(item.toString(runtime).utf8(runtime).c_str()); | |
| try { | |
| for (size_t i = 0; i < size; ++i) { | |
| auto item = array.getValueAtIndex(runtime, i); | |
| static_cast<char**>(data)[i] = | |
| strdup(item.toString(runtime).utf8(runtime).c_str()); | |
| if (static_cast<char**>(data)[i] == nullptr) { | |
| // Free previously allocated strings | |
| for (size_t j = 0; j < i; ++j) { | |
| free(static_cast<char**>(data)[j]); | |
| } | |
| delete[] static_cast<char**>(data); | |
| throw std::bad_alloc(); | |
| } | |
| } | |
| } catch (...) { | |
| // Free any allocated strings if an exception occurs | |
| for (size_t j = 0; j < size; ++j) { | |
| if (static_cast<char**>(data)[j]) { | |
| free(static_cast<char**>(data)[j]); | |
| } | |
| } | |
| delete[] static_cast<char**>(data); | |
| throw; |
Contributor
Author
There was a problem hiding this comment.
This might FP.
After tensor create, the string pointer is controlled by Tensor and toString should no exception.
js/react_native/android/src/main/java/ai/onnxruntime/reactnative/OnnxruntimeModule.java
Show resolved
Hide resolved
Contributor
|
@hans00 could you please update the branch to latest main? |
Contributor
|
/azp run Linux QNN CI Pipeline, Win_TRT_Minimal_CUDA_Test_CI, Windows ARM64 QNN CI Pipeline, Windows GPU Doc Gen CI Pipeline |
|
Azure Pipelines successfully started running 4 pipeline(s). |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
|
need to fix the lint/format issues: lintrunner ESLint |
Contributor
|
The RN build error seems related: |
Contributor
Author
Error are fixed. And manually tested E2E are works fine. |
Contributor
|
There are still build errors in React Native CI pipeline. androidiOS E2E testsiOS E2E tests |
Contributor
Author
Oh, I missed unit test files. |
Contributor
Author
|
@fs-eire Errors might be fixed. I have fully manually tested the CI E2E workflow. |
Contributor
|
need fix this format precheck: |
Contributor
|
The React Native CI failure seems to indicate that The Windows CPU CI - it's not related. Sync up to latest main branch should fix it. |
Contributor
Author
This is not root cause.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
close #16031
Motivation and Context
Make React Native fully zero copy.