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

feat: Add react-compiler safe API to mutables #6312

Merged
merged 16 commits into from
Sep 2, 2024

Conversation

tjzel
Copy link
Collaborator

@tjzel tjzel commented Jul 22, 2024

Summary

This PR adds a secondary API for Reanimated mutables (useSharedValue hook) that wouldn't be troublesome for react-compiler static analysis. More details can be found here.

New API example

const height = useSharedValue(100);
const width = useSharedValue(100);

const animatedStyle = useAnimatedStyle(() => ({
  height: height.get(),
  width: width.get(),
}));

useEffect(() => {
  height.set(200);
  width.set((value) => value * 2);
});

Performance

I measured the time of 4096 set vs value+= calls on a single frame.

Profiling code
export default function EmptyExample() {
  const oldSvArray: SharedValue<number>[] = [];
  const newSvArray: SharedValue<number>[] = [];

  const size = 4096;
  const frames = 600;
  for (let i = 0; i < size; i++) {
    oldSvArray.push(useSharedValue(0));
    newSvArray.push(useSharedValue(0));
  }

  function runOldAPI() {
    runOnUI(() => {
      let framesLeft = frames;
      let total = 0;

      function update() {
        const now = performance.now();
        for (let i = 0; i < size; i++) {
          oldSvArray[i].value += 1;
        }
        const end = performance.now();
        const elapsed = end - now;
        total += elapsed;
        framesLeft -= 1;
        if (framesLeft > 0) {
          requestAnimationFrame(update);
        } else {
          console.log(`Elapsed old: ${total.toFixed(2)}`);
          console.log(`Average old: ${(total / frames).toFixed(2)}`);
        }
      }

      update();
    })();
  }

  function runNewAPI() {
    runOnUI(() => {
      let framesLeft = frames;
      let total = 0;

      function update() {
        const now = performance.now();
        for (let i = 0; i < size; i++) {
          newSvArray[i].set((value) => value + 1);
        }
        const end = performance.now();
        const elapsed = end - now;
        total += elapsed;
        framesLeft -= 1;
        if (framesLeft > 0) {
          requestAnimationFrame(update);
        } else {
          console.log(`Elapsed new: ${total.toFixed(2)}`);
          console.log(`Average new: ${(total / frames).toFixed(2)}`);
        }
      }

      update();
    })();
  }

  useEffect(() => {
    // Warmup.
    runOldAPI();
    runNewAPI();
  }, []);

  return (
    <View style={styles.container}>
      <Button title="Run old API" onPress={runOldAPI} />
      <Button title="Run new API" onPress={runNewAPI} />
    </View>
  );
}

From my measurements I got:

  • 4.47±0.04ms (per 4096 assignments per frame) with .value
  • 4.51±0.10ms (per 4096 assignments per frame) with set

Those values are definitely within margin errors, so performance loss is negligible with the new API.

Test plan

  • Runtime tests pass.
  • Add new runtime test suite.
  • Add TypeScript tests if necessary.

@tjzel tjzel changed the base branch from main to @tjzel/less-error-prone-_value July 23, 2024 08:44
@tjzel tjzel force-pushed the @tjzel/get-set-API-for-mutables branch from 10fa7e2 to df2d534 Compare July 23, 2024 08:46
@kirillzyusko
Copy link
Contributor

@tjzel I have few questions about this approach - I can not write comments in original thread discussion, so will drop them here if you don't mind 😅

  1. Is there any plans for backporting these change to older versions of reanimated?
  2. If there is no such plans, and my library depends on REA, then there is no way to successfully compile files that uses useSharedValue with react-compiler?

@tjzel
Copy link
Collaborator Author

tjzel commented Jul 24, 2024

Hey there @kirillzyusko!

React Compiler isn't recommended to roll-out for production yet. This API is an anticipation step made with cooperation with Meta. We want to meet the standards of React 19 already so the community can embrace it seamlessly when it launches.

Is there any plans for backporting these change to older versions of reanimated?

We don't have such plans. Some time ago we decided to follow a stricter approach with version compatibility in Reanimated. This is why we think we shouldn't backport these changes to older versions of Reanimated - their supported versions of React Native aren't meant to work with the Compiler.

If there is no such plans, and my library depends on REA, then there is no way to successfully compile files that uses useSharedValue with react-compiler?

I don't think it's a good idea to use older versions of Reanimated with the Compiler. This PR isn't the only part of the integration, i.e. this one was way more important internally and without it you're likely to run into errors at some point. That's another reason we don't want to backport this PR.

If it's more about the new Rules of React and linting, you can make wrapper hooks on useSharedValue on older versions of Reanimated.

If you go through the Compiler's (or ESLint plugin's) code you can see it has special rules for Reanimated types. Therefore operations on "raw shared values" aren't considered errors. It won't handle composition however - there you should add your own layer of safe API.

On what older versions exactly are you stuck and why can't you upgrade?

@kirillzyusko
Copy link
Contributor

On what older versions exactly are you stuck and why can't you upgrade?

@tjzel I'm maintaining react-native-keyboard-controller and minimal supported REA version is 2.11.0. I totally understand why you are not going to backport these changes - thank you for detailed explanation!

Currently react-compiler eslint complains about assigning value to .value property. If you are introducing new API in 3.15 then it means that in order to make my code in library compatible with react-compiler rules I'll have to bump minimal supported version to 3.15 (which is quite big change).

Do you think that more reasonable would be to add a wrapper around useSharedValue hook and use this hook in the source code of my library? In this case it would be compliant to react-compiler rules. I understand, that people who are using my library with reanimated 2.11 version and who will try to enable compiler on this version will get errors, but it's their problem, not my.

What I want to achieve is to get rid off errors preventing react-compiler from a compilation of certain files in my library without bumping react-native-reanimated version.

@tjzel
Copy link
Collaborator Author

tjzel commented Jul 24, 2024

Making a wrapper hook seems sufficient in your case, you could even check dynamically if get and set are present. The hook itself would probably raise Compiler errors, since you'd need to directly modify the result of useSharedValue. I'd suggest skipping that one wrapper hook for the Compiler with 'use-no-memo' directive.

@kirillzyusko
Copy link
Contributor

Thank you for response @tjzel!

I'll wait for this PR to be merged and then will re-work my code accordingly! Thank you for your clarifications!

Base automatically changed from @tjzel/less-error-prone-_value to main August 23, 2024 14:06
@Latropos Latropos self-requested a review August 26, 2024 10:17
tjzel and others added 7 commits August 26, 2024 12:46
## Summary
Implement measurement of relative coordinates on Android.

## Test plan
I've added more logic to runtime tests to include parent component
changes in calculations.


<details><summary> Example code to log measurement </summary>

```tsx
import React from 'react';
import { Button, StyleSheet, View } from 'react-native';
import type { MeasuredDimensions } from 'react-native-reanimated';
import Animated, {
  measure,
  runOnUI,
  useAnimatedRef,
  useSharedValue,
} from 'react-native-reanimated';

const EXPECTED_MEASUREMENT = {
  height: 100,
  pageX: 302,
  width: 80,
  x: 120,
  y: 150,
};

export default function App() {
  const animatedRef = useAnimatedRef<Animated.View>();

  const measurement = useSharedValue<MeasuredDimensions | null>(null);

  const handlePress = () => {
    runOnUI(() => {
      'worklet';
      measurement.value = measure(animatedRef);
      console.log(measurement.value);
    })();
  };

  return (
    <View style={styles.container}>
      <View style={styles.bigBox}>
        <Animated.View ref={animatedRef} style={styles.box} />
      </View>
      <Button onPress={handlePress} title="Click me" />
    </View>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
  },
  bigBox: {
    position: 'absolute',
    top: 0,
    left: EXPECTED_MEASUREMENT.pageX - EXPECTED_MEASUREMENT.x,
    height: 300,
    width: 300,
    borderColor: '#b58df1',
    borderWidth: 2,
  },
  box: {
    top: EXPECTED_MEASUREMENT.y,
    left: EXPECTED_MEASUREMENT.x,
    height: EXPECTED_MEASUREMENT.height,
    width: EXPECTED_MEASUREMENT.width,
    backgroundColor: '#b58df1',
  },
});

```
</summary>
## Summary

This PR extracts from TestRunner logic that isn't directly related to
running tests.

## Test plan
## Summary

This PR is a much cleaner approach than proposed in #6364. It includes
metro-config modification which is essential to collapse logs from
reanimated source code, which aren't helpful to the user while tracking
down the issue. The previous approach was trimming logs from reanimated
source code completely - this approach just collapses them, so that they
are still available to the user and can be revealed above the presented
stack trace part.

## General idea

To get better logs, I had to implement the following 2 changes:
1. **metro config** - the `symbolicator` must have been added to
properly collapse stack frames that aren't meaningful to the user,
2. **logger object** - the new logger object uses `LogBox.addLog`
method, thanks to which we can get pretty stack traces when we log a
warning from the UI thread (before such warnings didn't include
meaningful stack trace as error stack was created inside `LogBox` after
`runOnJS` was called, so we were getting a bit limited JS stack - see
[example
11](#6387 (comment))
in the follow up PR).

## Example improvement
(tested on a real project to see if it works there as well)

- current logs either point to the reanimated source code or aren't
readable at all (if warning is logged from the UI thread as in the
example below)
- new logger shows correct destination of the issue culprit in the code
frame, collapses stack frames in the call stack that aren't interesting
to the user (reanimated source code) and focuses on the file where the
user included problematic code

| Before | After |
|-|-|
| <video
src="https://github.com/user-attachments/assets/a5302586-f4d0-4636-8bd8-6c406c9d8c73"
/> | <video
src="https://github.com/user-attachments/assets/3121636f-69a2-4b6f-8f38-b1889d4c62e1"
/> |

## Test plan

See the example in the next PR (#6387).

---------

Co-authored-by: Tomasz Żelawski <[email protected]>
@tjzel tjzel marked this pull request as ready for review August 28, 2024 07:25
Copy link
Contributor

@patrycjakalinska patrycjakalinska left a comment

Choose a reason for hiding this comment

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

lgtm docs ✨

@tjzel tjzel added this pull request to the merge queue Sep 2, 2024
Merged via the queue into main with commit c7f4b5f Sep 2, 2024
9 checks passed
@tjzel tjzel deleted the @tjzel/get-set-API-for-mutables branch September 2, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants