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

[android][newarch] performance / requestAnimationFrame behavior changes #47888

Closed
lovegaoshi opened this issue Nov 21, 2024 · 11 comments
Closed
Labels
Needs: Version Info Platform: Android Android applications. Resolution: Answered When the issue is resolved with a simple answer Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)

Comments

@lovegaoshi
Copy link

lovegaoshi commented Nov 21, 2024

Description

I've noticed requestAnimationFrame might behave differently between the old arch (snack expo 51) and the new arch (snack expo 52). in the provided snack, requestAnimationFrame is used to count the number of frames per second, which counts 60fps in old, 30fps in new.

on a real S21, a similar fps counter repo (repo init with expo cli https://github.com/lovegaoshi/rnbug-newarch-perf ) shows ~40-50 fps in new, ~70-100 fps in old. it all seems like requestAnimationFrame has only half the frame count in new vs old.

I'm investigating about the newarch performance @ #47490, theres the off chance this is due to the new arch's performance. worth noting the new arch's animation is visibly choppier than the old arch too.

Steps to reproduce

look at snack's top left corner

React Native Version

0.76.3

Affected Platforms

Runtime - Android

Areas

Fabric - The New Renderer, TurboModule - The New Native Module System

Output of npx react-native info

snack/expo 52

Stacktrace or Logs

n/a

Reproducer

https://snack.expo.dev/@lovegaoshi/thrilled-indigo-pizza

Screenshots and Videos

No response

@lovegaoshi lovegaoshi added Needs: Triage 🔍 Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules) labels Nov 21, 2024
@react-native-bot
Copy link
Collaborator

Warning

Could not parse version: We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.76.2.

@react-native-bot
Copy link
Collaborator

Warning

Could not parse version: We could not find or parse the version number of React Native in your issue report. Please use the template, and report your version including major, minor, and patch numbers - e.g. 0.76.2.

@lovegaoshi
Copy link
Author

after forcing 60fps on s21 the choppiness is very easy to identify. and the nrw arch is also capped at 30, while old is at 60.

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Nov 21, 2024
@lovegaoshi lovegaoshi changed the title [android][newarch] requestAnimationFrame behavior changes? [android][newarch] performance / requestAnimationFrame behavior changes Nov 21, 2024
@brentvatne
Copy link
Collaborator

brentvatne commented Nov 21, 2024

you're not actually logging the fps as per rAF here, that would be done with something like this:

import { StyleSheet, Text, View } from 'react-native';

const start = performance.now();
let frames = 0;

const logFps = () => {
  frames++;
  console.log(frames / (performance.now() - start) * 1000);
  requestAnimationFrame(logFps);
}

logFps();

export default function App() {
  return (
    <View style={{ flex: 1, alignItems: 'center', justifyContent: 'center' }}>
      <Text>look at the console</Text>
    </View>
  );
}

if you run this, you will notice that you get the fps that you expect.

what you are observing is likely the effect of using batched updates with the new react scheduler. state updates inside of timers are automatically batched. read more about batched state updates.

in your example, you put setState inside of a timeout inside of a requestAnimationFrame:

image

you shouldn't depend on setState executing synchronously here. maybe @rickhanlonii or @cipolleschi can explain how to flush state updates synchronously, since flushSync doesn't appear to be exposed by react-native

@brentvatne
Copy link
Collaborator

if you remove the setTimeout(() => {}, 0) wrapping the above code from your example, it is once again 60fps.

it seems like what is desirable here is to perhaps document better some of the behavior changes in the scheduler. but i'm not convinced that this is necessarily a regression

@lovegaoshi
Copy link
Author

hi! appreciate the insight. that explanation makes sense. however i squinted my eyes and think the performance degradation is still present, or there is a migration guide i missed.

the repo is https://github.com/lovegaoshi/rnbug-newarch-perf. its a expo cli init project and the motionlayout animation built with reanimated. the video is recorded on a s21 60fps mode. first is old, second is new.

Screen_Recording_20241121_153708_rnbugNew.mp4

@brentvatne
Copy link
Collaborator

@lovegaoshi - it seems like that is a better issue to post to react-native-reanimated, no?

@lovegaoshi
Copy link
Author

actually i dont believe it's reanimated, as i did have smooth animation using the exact same implementation. the video below is the linked repo just one commit previous, also new arch; alas its smooth after putting to background then restored, somehow.

Screen_Recording_20241119_060028_rnbug.mp4

i have to admit i believed the choppiness was due to whatever the fps logic is literally cutting the frame rate by half, as it does look like it. though this still seems like a reproducible example to show the new arch performance.

@brentvatne
Copy link
Collaborator

@lovegaoshi - that may be the case, but keep in mind that reanimated also has some different codepaths and interactions with react-native when using the new architecture. just because a problem is apparent when you enable the new architecture doesn't immediately mean there is an issue in the new arch itself, it could be in the way that a library is integrating with it. we don't have enough information at this point to say where it is but i've pinged the reanimated team to see if they can take a look

@bartlomiejbloniarz
Copy link
Contributor

Hi @lovegaoshi. I tested this using the Android Studio Profiler on a Pixel 4a and got a comparable performance between architectures (with the New one being better). However on a lower-end device (Oppo A16) the experience was better (in Debug) on the Old Arch. I don't think there is a regression here that can be pinpointed to a singular change.

I tried to figure out whether reanimated is doing something out of ordinary here (cc @brentvatne), but I didn't really find anything significant. In reanimated we use the RN api for updating the ShadowTree on every animation frame, which is very different from what we did on the Old Arch.

@blakef
Copy link
Contributor

blakef commented Dec 2, 2024

@lovegaoshi for a more public note on this, see @rubennorte's comment here. The implementation of requestAnimationFrame isn't to web-spec, it's basically the equivalent of calling setTimeout(callback, 0):

// The current implementation of requestAnimationFrame is the same
// as setTimeout(0). This isn't exactly how requestAnimationFrame
// is supposed to work on web, and may change in the future.

What this means is React Native doesn't called these at framerate intervals (but as part of the "regular" event loop), so you can't use it to measure frame rate.

@cortinico cortinico added Resolution: Answered When the issue is resolved with a simple answer and removed Needs: Attention Issues where the author has responded to feedback. labels Dec 3, 2024
@blakef blakef added Resolution: Answered When the issue is resolved with a simple answer and removed Resolution: Answered When the issue is resolved with a simple answer labels Dec 3, 2024
@blakef blakef closed this as completed Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Version Info Platform: Android Android applications. Resolution: Answered When the issue is resolved with a simple answer Type: New Architecture Issues and PRs related to new architecture (Fabric/Turbo Modules)
Projects
None yet
Development

No branches or pull requests

6 participants