Skip to content

Commit

Permalink
Don't update shared value unless the config has changed (#2037)
Browse files Browse the repository at this point in the history
This PR slightly modifies how gesture callbacks are updated when using Reanimated. Instead of updating them every time, it will now be updated only when the gesture config has changed. In combination with `useMemo`, this will make optimizing performance-critical use cases easier.

It accomplishes this by adding `gestureId` property to every gesture instance, which then is checked before updating the shared value. The update happens only when the id is different (which will happen only when the gesture inside `useMemo` is recalculated, i.e. only when the dependencies change).
  • Loading branch information
j-piasecki authored May 4, 2022
1 parent 19a6e0f commit 1217039
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 1 deletion.
14 changes: 14 additions & 0 deletions docs/docs/api/gestures/gesture.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,20 @@ sidebar_label: Gesture

`Gesture` is the object that allows you to create and compose gestures.

:::tip
Consider wrapping your gesture configurations with `useMemo`, as it will reduce the amount of work Gesture Handler has to do under the hood when updating gestures. For example:
```jsx
const gesture = useMemo(
() =>
Gesture.Tap().onStart(() => {
console.log('Number of taps:', tapNumber + 1);
setTapNumber((value) => value + 1);
}),
[tapNumber, setTapNumber]
);
```
:::

### Gesture.Tap()

Creates a new instance of [`TapGesture`](./tap-gesture.md) with its default config and no callbacks.
Expand Down
25 changes: 24 additions & 1 deletion src/handlers/gestures/GestureDetector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,34 @@ function updateHandlers(
}

if (preparedGesture.animatedHandlers) {
preparedGesture.animatedHandlers.value = (preparedGesture.config
const previousHandlersValue =
preparedGesture.animatedHandlers.value ?? [];
const newHandlersValue = (preparedGesture.config

This comment has been minimized.

Copy link
@AndreiCalazans

AndreiCalazans Jun 21, 2024

@j-piasecki @kmagiera we stumbled onto while investigating performance issues on Android. We recently introduced GestureDetector around individual items in a list and we are seeing many performance regressions to our prod metrics.

We recently also learned that reading SharedValues.value on the main JS thread blocks the thread (I wrote an article about this finding).

When running the profiler in Release on a low end Android device, we saw this:

Screenshot 2024-06-21 at 15 42 59

By profling our app in production for a total amount of 12 seconds for us to open the app and navigate to our search screen. The profilings indicate that the JS thread is blocked by this preparedGesture.animatedHandlers.value for an average time of 900ms. This is particularly exacerbated because we render a list of items each wrapped with a GestureDetector.

image

I reverted this change you made with a patch and performance got better. I'm wondering now if this logic that checks previous handlers value is even necessary given that updateHandlers call is wrapped within a useEffect that should only run if children or the gesture prop changes. Perhaps that useEffect could be further narrowed down to just checking if props.gesture changed allowing the consumer to control the updateHandlers with a useMemo at the consumer site.

We are on version 2.14 however the logic above looks the same on latest despite the recent house clean up which split this logic in multiple files.

This comment has been minimized.

Copy link
@j-piasecki

j-piasecki Jun 24, 2024

Author Member

Perhaps that useEffect could be further narrowed down to just checking if props.gesture changed allowing the consumer to control the updateHandlers with a useMemo at the consumer site.

It very likely could be, though doing it more granularly allows for not updating the Reanimated part if only gestures running on JS thread get updated.

I was able to remove the SV read and instead use information already there on the JS thread. Could you verify that it works in your case #2957?

This comment has been minimized.

Copy link
@AndreiCalazans

AndreiCalazans Jun 24, 2024

@j-piasecki yes this works well. I slightly changed the variable names to backport it to 2.14.0

diff --git a/src/handlers/gestures/GestureDetector.tsx b/src/handlers/gestures/GestureDetector.tsx
index 71a21dece9576a2e616b5ef6338dfe6f56c715a8..b7373f6914c1b782cda9a82d41ab489486956ffd 100644
--- a/src/handlers/gestures/GestureDetector.tsx
+++ b/src/handlers/gestures/GestureDetector.tsx
@@ -274,9 +274,23 @@ function updateHandlers(
     if (!mountedRef.current) {
       return;
     }
+
+    // if amount of gesture configs changes, we need to update the callbacks in shared value
+    let shouldUpdateSharedValueIfUsed =
+      preparedGesture.config.length !== gesture.length;
+
     for (let i = 0; i < gesture.length; i++) {
       const handler = preparedGesture.config[i];
 
+      // if the gestureId is different (gesture isn't wrapped with useMemo or its dependencies changed),
+      // we need to update the shared value, assuming the gesture runs on UI thread or the thread changed
+      if (
+        handler.handlers.gestureId !== gesture[i].handlers.gestureId &&
+        (gesture[i].shouldUseReanimated || handler.shouldUseReanimated)
+      ) {
+        shouldUpdateSharedValueIfUsed = true;
+      }
+
       handler.config = gesture[i].config;
       handler.handlers = gesture[i].handlers;
 
@@ -300,33 +314,12 @@ function updateHandlers(
     }
 
     if (preparedGesture.animatedHandlers) {
-      const previousHandlersValue =
-        preparedGesture.animatedHandlers.value ?? [];
-      const newHandlersValue = preparedGesture.config
-        .filter((g) => g.shouldUseReanimated) // ignore gestures that shouldn't run on UI
-        .map((g) => g.handlers) as unknown as HandlerCallbacks<
-        Record<string, unknown>
-      >[];
-
-      // if amount of gesture configs changes, we need to update the callbacks in shared value
-      let shouldUpdateSharedValue =
-        previousHandlersValue.length !== newHandlersValue.length;
-
-      if (!shouldUpdateSharedValue) {
-        // if the amount is the same, we need to check if any of the configs inside has changed
-        for (let i = 0; i < newHandlersValue.length; i++) {
-          if (
-            // we can use the `gestureId` prop as it's unique for every config instance
-            newHandlersValue[i].gestureId !== previousHandlersValue[i].gestureId
-          ) {
-            shouldUpdateSharedValue = true;
-            break;
-          }
-        }
-      }
-
-      if (shouldUpdateSharedValue) {
-        preparedGesture.animatedHandlers.value = newHandlersValue;
+      if (shouldUpdateSharedValueIfUsed) {
+        preparedGesture.animatedHandlers.value = preparedGesture.config
+          .filter((g) => g.shouldUseReanimated) // ignore gestures that shouldn't run on UI
+          .map((g) => g.handlers) as unknown as HandlerCallbacks<
+          Record<string, unknown>
+        >[];
       }
     }
 
.filter((g) => g.shouldUseReanimated) // ignore gestures that shouldn't run on UI
.map((g) => g.handlers) as unknown) as HandlerCallbacks<
Record<string, unknown>
>[];

// if amount of gesture configs changes, we need to update the callbacks in shared value
let shouldUpdateSharedValue =
previousHandlersValue.length !== newHandlersValue.length;

if (!shouldUpdateSharedValue) {
// if the amount is the same, we need to check if any of the configs inside has changed
for (let i = 0; i < newHandlersValue.length; i++) {
if (
// we can use the `gestureId` prop as it's unique for every config instance
newHandlersValue[i].gestureId !== previousHandlersValue[i].gestureId
) {
shouldUpdateSharedValue = true;
break;
}
}
}

if (shouldUpdateSharedValue) {
preparedGesture.animatedHandlers.value = newHandlersValue;
}
}

scheduleFlushOperations();
Expand Down
16 changes: 16 additions & 0 deletions src/handlers/gestures/gesture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type TouchEventHandlerType = (
) => void;

export type HandlerCallbacks<EventPayloadT extends Record<string, unknown>> = {
gestureId: number;
handlerTag: number;
onBegin?: (event: GestureStateChangeEvent<EventPayloadT>) => void;
onStart?: (event: GestureStateChangeEvent<EventPayloadT>) => void;
Expand Down Expand Up @@ -115,17 +116,32 @@ export abstract class Gesture {
abstract prepare(): void;
}

let nextGestureId = 0;
export abstract class BaseGesture<
EventPayloadT extends Record<string, unknown>
> extends Gesture {
private gestureId = -1;
public handlerTag = -1;
public handlerName = '';
public config: BaseGestureConfig = {};
public handlers: HandlerCallbacks<EventPayloadT> = {
gestureId: -1,
handlerTag: -1,
isWorklet: [],
};

constructor() {
super();

// Used to check whether the gesture config has been updated when wrapping it
// with `useMemo`. Since every config will have a unique id, when the dependencies
// don't change, the config won't be recreated and the id will stay the same.
// If the id is different, it means that the config has changed and the gesture
// needs to be updated.
this.gestureId = nextGestureId++;
this.handlers.gestureId = this.gestureId;
}

private addDependency(
key: 'simultaneousWith' | 'requireToFail',
gesture: Exclude<GestureRef, number>
Expand Down

0 comments on commit 1217039

Please sign in to comment.