-
Notifications
You must be signed in to change notification settings - Fork 682
Summary: Use javaClassStatic() for class references stored in static … #14744
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/14744
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unrelated FailuresAs of commit e6f4a47 with merge base f24351a ( NEW FAILURE - The following job has failed:
FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but was present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
This PR needs a
|
extension/android/jni/jni_helper.cpp
Outdated
// Define the JavaClass wrapper | ||
struct JExecutorchRuntimeException : public facebook::jni::JavaClass<JExecutorchRuntimeException> { | ||
static constexpr auto kJavaDescriptor = "Lorg/pytorch/executorch/ExecutorchRuntimeException;"; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts on moving this definition to the jni_helper.h file
extension/android/jni/jni_helper.cpp
Outdated
// Find the Java ExecutorchRuntimeException class | ||
static auto exceptionClass = facebook::jni::findClassLocal( | ||
"org/pytorch/executorch/ExecutorchRuntimeException"); | ||
static const auto exceptionClass = JExecutorchRuntimeException::javaClassStatic(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
auto env = facebook::jni::Environment::current();
if (!env) {
// Handle case where JNI environment is not available
return;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't hurt to add defensive check (although checking for null is not very typical I think), but handling cases where it is null can be tricky, We can't talk to java layer as jni env is not valid, and it is not ideal to throw c++ exception from here.
extension/android/jni/jni_helper.cpp
Outdated
// Find the Java ExecutorchRuntimeException class | ||
static auto exceptionClass = facebook::jni::findClassLocal( | ||
"org/pytorch/executorch/ExecutorchRuntimeException"); | ||
static const auto exceptionClass = JExecutorchRuntimeException::javaClassStatic(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, any thoughts on catching C++ exceptions? Something like this:
try {
static const auto exceptionClass = JExecutorchRuntimeException::javaClassStatic();
// ... rest of the code
} catch (const std::exception& e) {
// Log the error or handle gracefully
// Avoid throwing C++ exceptions across JNI boundary
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think catching C++ exceptions inside a helper like this can mask real initialization bugs and make debugging harder.
…variables - creates global references safe for persistence findClassLocal() returns a local reference. Storing it in static auto exceptionClass = ... could result in potential 'invalid local reference:' as local references become invalid when the JNI frame ends Test Plan: Reviewers: Subscribers: Tasks: Tags:
@pytorchbot cherry-pick --onto release/1.0 -c critical |
#14744) …variables - creates global references safe for persistence findClassLocal() returns a local reference. Storing it in static auto exceptionClass = ... could result in potential 'invalid local reference:' as local references become invalid when the JNI frame ends Test Plan: Reviewers: Subscribers: Tasks: Tags: ### Summary [PLEASE REMOVE] See [CONTRIBUTING.md's Pull Requests](https://github.com/pytorch/executorch/blob/main/CONTRIBUTING.md#pull-requests) for ExecuTorch PR guidelines. [PLEASE REMOVE] If this PR closes an issue, please add a `Fixes #<issue-id>` line. [PLEASE REMOVE] If this PR introduces a fix or feature that should be the upcoming release notes, please add a "Release notes: <area>" label. For a list of available release notes labels, check out [CONTRIBUTING.md's Pull Requests](https://github.com/pytorch/executorch/blob/main/CONTRIBUTING.md#pull-requests). ### Test plan [PLEASE REMOVE] How did you test this PR? Please write down any manual commands you used and note down tests that you have written if applicable. Co-authored-by: Github Executorch <[email protected]> (cherry picked from commit 3b16bc1)
Cherry picking #14744The cherry pick PR is at #14811 and it is recommended to link a critical cherry pick PR with an issue. The following tracker issues are updated: Details for Dev Infra teamRaised by workflow job |
…variables - creates global references safe for persistence
findClassLocal() returns a local reference. Storing it in static auto exceptionClass = ... could result in potential 'invalid local reference:' as local references become invalid when the JNI frame ends
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Summary
[PLEASE REMOVE] See CONTRIBUTING.md's Pull Requests for ExecuTorch PR guidelines.
[PLEASE REMOVE] If this PR closes an issue, please add a
Fixes #<issue-id>
line.[PLEASE REMOVE] If this PR introduces a fix or feature that should be the upcoming release notes, please add a "Release notes: " label. For a list of available release notes labels, check out CONTRIBUTING.md's Pull Requests.
Test plan
[PLEASE REMOVE] How did you test this PR? Please write down any manual commands you used and note down tests that you have written if applicable.