-
-
Notifications
You must be signed in to change notification settings - Fork 32
Feat/worklet runner performance improvement #729
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
base: main
Are you sure you want to change the base?
Conversation
|
||
/// @note We want to avoid automatic destruction as | ||
/// when runtime is destroyed, underlying pointer will be invalid | ||
char unsafeWorklet[sizeof(jsi::Function)]; |
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.
You could store <worklets::SerializableWorklet>
as it does the same memoization as well (see RetainingSerializable
super class).
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.
ill check this out
packages/react-native-audio-api/common/cpp/audioapi/core/utils/worklets/WorkletsRunner.cpp
Show resolved
Hide resolved
#if RN_AUDIO_API_ENABLE_WORKLETS | ||
unsafeRuntimePtr = &strongRuntime->getJSIRuntime(); | ||
strongRuntime->executeSync( | ||
[this, shareableWorklet](jsi::Runtime &rt) -> jsi::Value { |
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.
The use of this
here might result in crashes during reloads and in brownfield apps.
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.
it is calling lambda synchronously do you still think that it would cause a crash ?
std::optional<jsi::Value> WorkletsRunner::executeOnRuntimeUnsafe( | ||
const std::function<jsi::Value(jsi::Runtime &)> &&job) const | ||
noexcept(noexcept(job)) { | ||
#if RN_AUDIO_API_ENABLE_WORKLETS | ||
return job(*unsafeRuntimePtr); | ||
#else | ||
return std::nullopt; | ||
#endif | ||
}; |
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.
If WorkletsRunner
is not tied to the lifetime of the runtime this will crash during reloads and in brownfields.
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.
It is
Improved performance of worklet runner.
Introduced changes
Checklist