Skip to content
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

Add setImmediate implementation to the UI runtime #3970

Merged
merged 26 commits into from
Feb 22, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
229262b
Stashing work
kmagiera Dec 23, 2022
651538c
Reset unrelated changes
kmagiera Jan 11, 2023
37fdaf7
Breaking things, please don't use this commit, just trying differenty…
kmagiera Jan 11, 2023
6a1dba4
Moar updates
kmagiera Jan 12, 2023
2e41569
Even moar updates
kmagiera Jan 13, 2023
4208b6d
A bit of cleanup
kmagiera Jan 13, 2023
924a1b1
Fix Android builds
kmagiera Jan 18, 2023
f1a7860
Revert "Fix Android builds"
kmagiera Jan 18, 2023
35fe5dc
Fix android builds for realz
kmagiera Jan 18, 2023
cf7e2b3
Merge branch 'main' into set-immediate
kmagiera Jan 18, 2023
1cadbbe
Handle events with timestamp
kmagiera Jan 18, 2023
e024b5f
Minor fixes
kmagiera Jan 18, 2023
81a7ef1
Fix cpplint
kmagiera Jan 19, 2023
096c34a
Trying to fix tests (not yet passing)
kmagiera Jan 20, 2023
0428f0c
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera Feb 21, 2023
ba92fbb
Fix some files after merging main
kmagiera Feb 21, 2023
e4db047
Lint/type fixes
kmagiera Feb 21, 2023
6f43b36
''Fix'' tests
kmagiera Feb 21, 2023
b445d51
fix typoz
kmagiera Feb 21, 2023
616db16
Moar test fixes
kmagiera Feb 21, 2023
77b0151
Update comments
kmagiera Feb 21, 2023
eea48bf
Moar cleanup
kmagiera Feb 21, 2023
e3dc780
Further cleanups + update to node 16 because performance.now cannot b…
kmagiera Feb 21, 2023
2c29b10
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera Feb 21, 2023
914fa25
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera Feb 21, 2023
bfb4a93
Remove timestamp from mapper worklet agrument as it is not being used
kmagiera Feb 22, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/static-root-checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,10 @@ jobs:
steps:
- name: checkout
uses: actions/checkout@v2
- name: Use Node.js 14
- name: Use Node.js 16
uses: actions/setup-node@v2
with:
node-version: 14
node-version: 16
cache: 'yarn'
- name: Install node dependencies
run: yarn
Expand Down
10 changes: 6 additions & 4 deletions Common/cpp/AnimatedSensor/AnimatedSensorModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,12 @@ jsi::Value AnimatedSensorModule::registerSensor(
}

auto &rt = *runtimeHelper->uiRuntime();
auto handler =
shareableHandler->getJSValue(rt).asObject(rt).asFunction(rt);
auto handler = shareableHandler->getJSValue(rt);
if (sensorType == SensorType::ROTATION_VECTOR) {
jsi::Object value(rt);
// TODO: timestamp should be provided by the platform implementation
// such that the native side has a chance of providing a true event
// timestamp
value.setProperty(rt, "qx", newValues[0]);
value.setProperty(rt, "qy", newValues[1]);
value.setProperty(rt, "qz", newValues[2]);
Expand All @@ -54,14 +56,14 @@ jsi::Value AnimatedSensorModule::registerSensor(
value.setProperty(rt, "pitch", newValues[5]);
value.setProperty(rt, "roll", newValues[6]);
value.setProperty(rt, "interfaceOrientation", orientationDegrees);
handler.call(rt, value);
runtimeHelper->runOnUIGuarded(handler, value);
} else {
jsi::Object value(rt);
value.setProperty(rt, "x", newValues[0]);
value.setProperty(rt, "y", newValues[1]);
value.setProperty(rt, "z", newValues[2]);
value.setProperty(rt, "interfaceOrientation", orientationDegrees);
handler.call(rt, value);
runtimeHelper->runOnUIGuarded(handler, value);
}
});
if (sensorId != -1) {
Expand Down
57 changes: 17 additions & 40 deletions Common/cpp/NativeModules/NativeReanimatedModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -309,11 +309,13 @@ jsi::Value NativeReanimatedModule::registerEventHandler(

scheduler->scheduleOnUI([=] {
jsi::Runtime &rt = *runtimeHelper->uiRuntime();
auto handlerFunction =
handlerShareable->getJSValue(rt).asObject(rt).asFunction(rt);
auto handlerFunction = handlerShareable->getJSValue(rt);
auto handler = std::make_shared<WorkletEventHandler>(
newRegistrationId, eventName, std::move(handlerFunction));
eventHandlerRegistry->registerEventHandler(handler);
runtimeHelper,
newRegistrationId,
eventName,
std::move(handlerFunction));
eventHandlerRegistry->registerEventHandler(std::move(handler));
});

return jsi::Value(static_cast<double>(newRegistrationId));
Expand Down Expand Up @@ -393,19 +395,11 @@ jsi::Value NativeReanimatedModule::configureLayoutAnimation(
}

void NativeReanimatedModule::onEvent(
double eventTimestamp,
const std::string &eventName,
const jsi::Value &payload) {
try {
eventHandlerRegistry->processEvent(*runtime, eventName, payload);
} catch (std::exception &e) {
std::string str = e.what();
this->errorHandler->setError(str);
this->errorHandler->raise();
} catch (...) {
std::string str = "OnEvent error";
this->errorHandler->setError(str);
this->errorHandler->raise();
}
eventHandlerRegistry->processEvent(
*runtime, eventTimestamp, eventName, payload);
}

bool NativeReanimatedModule::isAnyHandlerWaitingForEvent(
Expand All @@ -421,20 +415,10 @@ void NativeReanimatedModule::maybeRequestRender() {
}

void NativeReanimatedModule::onRender(double timestampMs) {
try {
std::vector<FrameCallback> callbacks = frameCallbacks;
frameCallbacks.clear();
for (auto &callback : callbacks) {
callback(timestampMs);
}
} catch (std::exception &e) {
std::string str = e.what();
this->errorHandler->setError(str);
this->errorHandler->raise();
} catch (...) {
std::string str = "OnRender error";
this->errorHandler->setError(str);
this->errorHandler->raise();
std::vector<FrameCallback> callbacks = frameCallbacks;
frameCallbacks.clear();
Comment on lines +418 to +419
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to just move-construct callbacks from frameCallbacks here, right?

Suggested change
std::vector<FrameCallback> callbacks = frameCallbacks;
frameCallbacks.clear();
std::vector<FrameCallback> callbacks = std::move(frameCallbacks);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't my code so prefer not to make this change. Is this behavior defined that the original vector gets reset when moved? Typically move constructors does not specify behavior of the moved object as normally the assumption is that the moved object is never used again. Here we still want to use the vector

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be fine.
C++ standard says that the moved-from object is left in a "valid but unspecified state" (so it should be fine to use the old vector later), and the std::vector spec further specifies that "After the move, other is guaranteed to be empty()." when move-constructing a vector, so it should clear it as well.

In case you don't feel safe using a vector after moving from it, I would still suggest doing something like

Suggested change
std::vector<FrameCallback> callbacks = frameCallbacks;
frameCallbacks.clear();
std::vector<FrameCallback> callbacks;
std::swap(callbacks, frameCallbacks)

since this is unambiguous in what it does, but still avoids copies/allocations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just like to avoid copying the callbacks here, since they can potentially capture a bunch of things - in this case it's a couple of shared_ptrs, which means unnecessary work on the atomic reference counters inside.

for (auto &callback : callbacks) {
callback(timestampMs);
}
}

Expand Down Expand Up @@ -485,13 +469,7 @@ bool NativeReanimatedModule::handleEvent(
const std::string &eventName,
const jsi::Value &payload,
double currentTime) {
jsi::Runtime &rt = *runtime.get();
jsi::Object global = rt.global();
jsi::String eventTimestampName =
jsi::String::createFromAscii(rt, "_eventTimestamp");
global.setProperty(rt, eventTimestampName, currentTime);
onEvent(eventName, payload);
global.setProperty(rt, eventTimestampName, jsi::Value::undefined());
onEvent(currentTime, eventName, payload);

// TODO: return true if Reanimated successfully handled the event
// to avoid sending it to JavaScript
Expand Down Expand Up @@ -674,13 +652,12 @@ jsi::Value NativeReanimatedModule::subscribeForKeyboardEvents(
const jsi::Value &handlerWorklet,
const jsi::Value &isStatusBarTranslucent) {
auto shareableHandler = extractShareableOrThrow(rt, handlerWorklet);
auto uiRuntime = runtimeHelper->uiRuntime();
return subscribeForKeyboardEventsFunction(
[=](int keyboardState, int height) {
jsi::Runtime &rt = *uiRuntime;
jsi::Runtime &rt = *runtimeHelper->uiRuntime();
auto handler = shareableHandler->getJSValue(rt);
handler.asObject(rt).asFunction(rt).call(
rt, jsi::Value(keyboardState), jsi::Value(height));
runtimeHelper->runOnUIGuarded(
handler, jsi::Value(keyboardState), jsi::Value(height));
},
isStatusBarTranslucent.getBool());
}
Expand Down
5 changes: 4 additions & 1 deletion Common/cpp/NativeModules/NativeReanimatedModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,10 @@ class NativeReanimatedModule : public NativeReanimatedModuleSpec,

void onRender(double timestampMs);

void onEvent(const std::string &eventName, const jsi::Value &payload);
void onEvent(
double eventTimestamp,
const std::string &eventName,
const jsi::Value &payload);

bool isAnyHandlerWaitingForEvent(std::string eventName);

Expand Down
3 changes: 2 additions & 1 deletion Common/cpp/Registries/EventHandlerRegistry.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ void EventHandlerRegistry::unregisterEventHandler(uint64_t id) {

void EventHandlerRegistry::processEvent(
jsi::Runtime &rt,
double eventTimestamp,
const std::string &eventName,
const jsi::Value &eventPayload) {
std::vector<std::shared_ptr<WorkletEventHandler>> handlersForEvent;
Expand All @@ -40,7 +41,7 @@ void EventHandlerRegistry::processEvent(
eventPayload.asObject(rt).setProperty(
rt, "eventName", jsi::String::createFromUtf8(rt, eventName));
for (auto handler : handlersForEvent) {
handler->process(rt, eventPayload);
handler->process(eventTimestamp, eventPayload);
}
}

Expand Down
1 change: 1 addition & 0 deletions Common/cpp/Registries/EventHandlerRegistry.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class EventHandlerRegistry {

void processEvent(
jsi::Runtime &rt,
double eventTimestamp,
const std::string &eventName,
const jsi::Value &eventPayload);

Expand Down
39 changes: 14 additions & 25 deletions Common/cpp/Tools/RuntimeDecorator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,28 +71,6 @@ void RuntimeDecorator::decorateRuntime(
#endif // DEBUG

jsi_utils::installJsiFunction(rt, "_log", logValue);

auto chronoNow = [](jsi::Runtime &rt,
const jsi::Value &thisValue,
const jsi::Value *args,
size_t count) -> jsi::Value {
double now = std::chrono::system_clock::now().time_since_epoch() /
std::chrono::milliseconds(1);
return jsi::Value(now);
};

rt.global().setProperty(
rt,
"_chronoNow",
jsi::Function::createFromHostFunction(
rt, jsi::PropNameID::forAscii(rt, "_chronoNow"), 0, chronoNow));
jsi::Object performance(rt);
performance.setProperty(
rt,
"now",
jsi::Function::createFromHostFunction(
rt, jsi::PropNameID::forAscii(rt, "now"), 0, chronoNow));
rt.global().setProperty(rt, "performance", performance);
}

void RuntimeDecorator::decorateUIRuntime(
Expand Down Expand Up @@ -144,9 +122,20 @@ void RuntimeDecorator::decorateUIRuntime(
jsi_utils::installJsiFunction(
rt, "_updateDataSynchronously", updateDataSynchronously);

jsi_utils::installJsiFunction(rt, "_getCurrentTime", getCurrentTime);
rt.global().setProperty(rt, "_frameTimestamp", jsi::Value::undefined());
rt.global().setProperty(rt, "_eventTimestamp", jsi::Value::undefined());
auto performanceNow = [getCurrentTime](
jsi::Runtime &rt,
const jsi::Value &thisValue,
const jsi::Value *args,
size_t count) -> jsi::Value {
return jsi::Value(getCurrentTime());
};
jsi::Object performance(rt);
performance.setProperty(
rt,
"now",
jsi::Function::createFromHostFunction(
rt, jsi::PropNameID::forAscii(rt, "now"), 0, performanceNow));
rt.global().setProperty(rt, "performance", performance);

// layout animation
jsi_utils::installJsiFunction(
Expand Down
5 changes: 3 additions & 2 deletions Common/cpp/Tools/WorkletEventHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
namespace reanimated {

void WorkletEventHandler::process(
jsi::Runtime &rt,
double eventTimestamp,
const jsi::Value &eventValue) {
handler.callWithThis(rt, handler, eventValue);
_runtimeHelper->runOnUIGuarded(
_handlerFunction, jsi::Value(eventTimestamp), eventValue);
}

} // namespace reanimated
16 changes: 12 additions & 4 deletions Common/cpp/Tools/WorkletEventHandler.h
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
#pragma once

#include <jsi/jsi.h>
#include <memory>
#include <string>
#include <utility>

#include "Shareables.h"

using namespace facebook;

namespace reanimated {
Expand All @@ -14,17 +17,22 @@ class WorkletEventHandler {
friend EventHandlerRegistry;

private:
std::shared_ptr<JSRuntimeHelper> _runtimeHelper;
uint64_t id;
std::string eventName;
jsi::Function handler;
jsi::Value _handlerFunction;

public:
WorkletEventHandler(
const std::shared_ptr<JSRuntimeHelper> &runtimeHelper,
uint64_t id,
std::string eventName,
jsi::Function &&handler)
: id(id), eventName(eventName), handler(std::move(handler)) {}
void process(jsi::Runtime &rt, const jsi::Value &eventValue);
jsi::Value &&handlerFunction)
: _runtimeHelper(runtimeHelper),
id(id),
eventName(eventName),
_handlerFunction(std::move(handlerFunction)) {}
void process(double eventTimestamp, const jsi::Value &eventValue);
};

} // namespace reanimated
17 changes: 15 additions & 2 deletions __tests__/Animation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,20 @@ const getDefaultStyle = () => ({
margin: 30,
});

const originalAdvanceTimersByTime = jest.advanceTimersByTime;

jest.advanceTimersByTime = (timeMs) => {
// This is a workaround for an issue with using setImmediate that's in the jest
// environment implemented as a 0-second timeout. Because of the fact we use
// setImmediate for scheduling runOnUI tasks as well as executing matters,
// starting new animaitons gets delayed be three frames. To compensate for that
// we execute pending timers three times before advancing the timers.
jest.runOnlyPendingTimers();
jest.runOnlyPendingTimers();
jest.runOnlyPendingTimers();
originalAdvanceTimersByTime(timeMs);
};

describe('Tests of animations', () => {
beforeEach(() => {
jest.useFakeTimers();
Expand Down Expand Up @@ -95,7 +109,7 @@ describe('Tests of animations', () => {

fireEvent.press(button);
jest.advanceTimersByTime(250);
jest.runOnlyPendingTimers(); // timers scheduled for the exact 250ms won't run without this additional call

style.width = 50; // value of component width after 150ms of animation
expect(view).toHaveAnimatedStyle(style);
});
Expand All @@ -109,7 +123,6 @@ describe('Tests of animations', () => {

fireEvent.press(button);
jest.advanceTimersByTime(250);
jest.runOnlyPendingTimers();
style.width = 50; // value of component width after 250ms of animation
expect(view).toHaveAnimatedStyle(style, true);
});
Expand Down
16 changes: 14 additions & 2 deletions __tests__/InterpolateColor.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,20 @@ import Animated, {
withTiming,
} from '../src';

const originalAdvanceTimersByTime = jest.advanceTimersByTime;

jest.advanceTimersByTime = (timeMs) => {
// This is a workaround for an issue with using setImmediate that's in the jest
// environment implemented as a 0-second timeout. Because of the fact we use
// setImmediate for scheduling runOnUI tasks as well as executing matters,
// starting new animaitons gets delayed be three frames. To compensate for that
// we execute pending timers three times before advancing the timers.
jest.runOnlyPendingTimers();
jest.runOnlyPendingTimers();
jest.runOnlyPendingTimers();
originalAdvanceTimersByTime(timeMs);
};

describe('colors interpolation', () => {
it('interpolates rgb without gamma correction', () => {
const colors = ['#105060', '#609020'];
Expand Down Expand Up @@ -157,15 +171,13 @@ describe('colors interpolation', () => {

fireEvent.press(button);
jest.advanceTimersByTime(250);
jest.runOnlyPendingTimers();

expect(view).toHaveAnimatedStyle(
{ backgroundColor: 'rgba(71, 117, 73, 1)' },
true
);

jest.advanceTimersByTime(250);
jest.runOnlyPendingTimers();

expect(view).toHaveAnimatedStyle(
{ backgroundColor: 'rgba(96, 144, 32, 1)' },
Expand Down
19 changes: 2 additions & 17 deletions android/src/main/cpp/NativeProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -137,24 +137,9 @@ void NativeProxy::installJSIBindings(
return static_cast<double>(output);
};

auto requestRender = [this, getCurrentTime](
auto requestRender = [this](
std::function<void(double)> onRender,
jsi::Runtime &rt) {
// doNoUse -> NodesManager passes here a timestamp from choreographer which
// is useless for us as we use diffrent timer to better handle events. The
// lambda is translated to NodeManager.OnAnimationFrame and treated just
// like reanimated 1 frame callbacks which make use of the timestamp.
auto wrappedOnRender = [getCurrentTime, &rt, onRender](double doNotUse) {
jsi::Object global = rt.global();
jsi::String frameTimestampName =
jsi::String::createFromAscii(rt, "_frameTimestamp");
double frameTimestamp = getCurrentTime();
global.setProperty(rt, frameTimestampName, frameTimestamp);
onRender(frameTimestamp);
global.setProperty(rt, frameTimestampName, jsi::Value::undefined());
};
this->requestRender(std::move(wrappedOnRender));
};
jsi::Runtime &rt) { this->requestRender(onRender); };

#ifdef RCT_NEW_ARCH_ENABLED
auto synchronouslyUpdateUIPropsFunction =
Expand Down
4 changes: 0 additions & 4 deletions ios/native/NativeProxy.mm
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,7 @@ static CFTimeInterval calculateTimestampWithSlowAnimations(CFTimeInterval curren
auto requestRender = [nodesManager, &module](std::function<void(double)> onRender, jsi::Runtime &rt) {
[nodesManager postOnAnimation:^(CADisplayLink *displayLink) {
double frameTimestamp = calculateTimestampWithSlowAnimations(displayLink.targetTimestamp) * 1000;
jsi::Object global = rt.global();
jsi::String frameTimestampName = jsi::String::createFromAscii(rt, "_frameTimestamp");
global.setProperty(rt, frameTimestampName, frameTimestamp);
onRender(frameTimestamp);
global.setProperty(rt, frameTimestampName, jsi::Value::undefined());
}];
};

Expand Down
Loading