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

Enable New Architecture in Bluesky on RN 0.76 #6755

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

WoLewicki
Copy link

PR enabling New Architecture in the app. Based on work from @haileyok in #6211 and a bit on #4853. Great work on those PRs so far 💪

There are still some issues like small input text on iOS, glitches with bottomsheet on both platforms and probably some more.

@WoLewicki
Copy link
Author

I'm happy to help investigate some of the issues if you'd like it @haileyok. We've already got much experience with all things New Arch related, like:

@WoLewicki WoLewicki changed the title @wolewicki/new arch 76 Enable New Architecture in Bluesky on RN 0.76 Nov 26, 2024
@mozzius mozzius marked this pull request as ready for review November 26, 2024 15:08
@mozzius mozzius marked this pull request as draft November 26, 2024 15:08
@gaearon
Copy link
Collaborator

gaearon commented Dec 19, 2024

Makes sense. Thanks for the details.

@bartlomiejbloniarz
Copy link

@gaearon We came back to the office yesterday and I was finally able to dig deeper into the issue. As I said before the laggy animation is a result of the JS thread being heavily utilized (since we need to synchronize the updates applied on the JS thread with the animation). This is what it looks like:
Screenshot 2025-01-08 at 12 02 41
The complete root blocks can last up to 150ms, which is scary because they seem to be continuously invoked. These calls are usually a result of a rerender, but in this case something weirder is happening:

  • as you scroll the _onScroll method of VirtualizedList is called. It invokes _scheduleCellsToRenderUpdate which is basically a Batchinator wrapper for _updateCellsToRender
  • _updateCellsToRender calls setState here. The state manages which elements of the list should be rendered. The function that is passed to setState here often returns null - which means that there is no update - so no rerenders should happen.
  • but the complete root function is called here anyway

After some digging it seems that the <AppProfiler/> component used here is the reason this happens. Somehow the renderer decides that even if the state update of the list bails out with no updates, this component needs to be cloned and a new ShadowTree needs to be commited. It's possibly an issue with the react Profiler component. I also didn't see the any updates from the Profiler component in my console. Is it not supposed to run in debug builds?

This is the trace after I removed it from the app:
Screenshot 2025-01-08 at 12 04 23

The trace looks better now (and the animation seems to work better) - there is still some room for improvement here (but on the reanimated side). The JS thread now is mostly occupied with c++ state updates that are invoked to synchronize the scroll position with the ShadowTree. Those calls could probably be made faster by avoiding some unnecessary reanimated calculations.

@gaearon
Copy link
Collaborator

gaearon commented Jan 8, 2025

Somehow the renderer decides that even if the state update of the list bails out with no updates, this component needs to be cloned and a new ShadowTree needs to be commited.

Could be an RN bug! But not sure how to file it. Can you reproduce the same in a minimal app? I'm not sure what the reproducing instructions would be.

@gaearon
Copy link
Collaborator

gaearon commented Jan 8, 2025

We can definitely remove it to work around.

@gaearon gaearon mentioned this pull request Jan 8, 2025
@bartlomiejbloniarz
Copy link

bartlomiejbloniarz commented Jan 9, 2025

Could be an RN bug! But not sure how to file it. Can you reproduce the same in a minimal app? I'm not sure what the reproducing instructions would be.

Yes, I was able to reproduce it in an expo app with simply this code.
I checked and the completeRoot function is called even if the state wasn't updated only when the Profiler is used.

Code
import { Profiler } from "react";
import { StyleSheet, Text, View } from "react-native";
import Animated from "react-native-reanimated";

const data = new Array(200);

function onRender(id: string, phase: string, actualDuration: number) {}

export default function App() {
  return (
    <Profiler id="app" onRender={onRender}>
      <View style={styles.container}>
        <Animated.FlatList
          data={data}
          renderItem={(x) => {
            return (
              <View style={{ backgroundColor: "red", width: 200, height: 300 }}>
                <Text>{x.index}</Text>
              </View>
            );
          }}
        />
      </View>
    </Profiler>
  );
}

const styles = StyleSheet.create({
  container: {
    flex: 1,
    justifyContent: "center",
  },
  message: {
    textAlign: "center",
    paddingBottom: 10,
  },
  camera: {
    flex: 1,
  },
  buttonContainer: {
    flex: 1,
    flexDirection: "row",
    backgroundColor: "transparent",
    margin: 64,
  },
  button: {
    flex: 1,
    alignSelf: "flex-end",
    alignItems: "center",
  },
  text: {
    fontSize: 24,
    fontWeight: "bold",
    color: "white",
  },
});

@WoLewicki
Copy link
Author

Ok perfect! I will merge main here soon and check the performance with Profiler removed.

@WoLewicki
Copy link
Author

Ok I merged the newest main and Android header works better now. What are the next steps here then?

@gaearon
Copy link
Collaborator

gaearon commented Jan 10, 2025

I'm working on a few gesture-related fixes so we'll need to make sure those don't regress with the transition. Let me finish up that work first and then I'll give this PR another spin.

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2025

I'm done with the gesture fixes so we can revive this. Would it be a good idea to bump RN here as well? Either to 0.76.6 or 0.77? This got me a bit worried: facebook/react-native#47490 (comment)

@gaearon

This comment was marked as resolved.

@gaearon

This comment was marked as resolved.

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2025

Hmm, dismissing the composer keeps the keyboard on for me

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2025

Pressables in post thread (e.g. click into a post, try to click like/repost/comment buttons) are strangely unresponsive. They get hover states but not generating presses

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2025

Swiping the drawer left and right is very laggy (this is all on low-end Android)

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2025

FWIW feed scrolling perf feels improved to me

@WoLewicki
Copy link
Author

I'm done with the gesture fixes so we can revive this. Would it be a good idea to bump RN here as well? Either to 0.76.6 or 0.77?

I'd start with 0.76.6 since it will be easier. Also 0.77 is not yet officially supported by expo, but it should come any moment.

@WoLewicki
Copy link
Author

Swiping the drawer left and right is very laggy (this is all on low-end Android)

We looked into it a bit but haven't got to the root of the problem. We can see some really have rerenders on animation start and end (lasting about 200ms on fast device). It shouldn't happen since we simply set isDrawerOpen state, which is not used almost anywhere. Interestingly, when we change the button to a setInterval, the lag disappears after about 6 seconds (probably the time for rest of UI to render).

Or are we missing something and isDrawerOpen or some other state connected to opening/closing of drawer is used elsewhere?

@aimachinedream
Copy link

On iOS, I've been finding that any animations that are not native are very slow. The drawer seems a particular problem. Unless there is absolutely nothing going on, the drawer will stutter.

A simple test is to have a video playing in the app. Perfectly smooth until you enable the new architecture.

@gaearon
Copy link
Collaborator

gaearon commented Jan 25, 2025

Or are we missing something and isDrawerOpen or some other state connected to opening/closing of drawer is used elsewhere?

The simplest way to find out would probably be to use the Profiler tab in React DevTools and to check the commits, have you tried that?

@WoLewicki
Copy link
Author

Also 0.77 is not yet officially supported by expo, but it should come any moment.

Ok that one is not true anymore: https://expo.dev/changelog/2025/01-21-react-native-0.77 so we could try with the newest RN version too.

@bartlomiejbloniarz
Copy link

@gaearon We only profiled with the Android Studio Profiler, and have seen that there were some heavy ShadowTree commits happening. We should check with DevTools.

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