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

Optimize Shadow Tree cloning #6214

Merged

Conversation

bartlomiejbloniarz
Copy link
Contributor

@bartlomiejbloniarz bartlomiejbloniarz commented Jul 4, 2024

Summary

This PR optimizes the algorithm used to clone the Shadow Tree used in ReanimatedCommitHook and performOperations.

Current approach

The current algorithm works as follows. After receiving a batch of updates, we iterate over the list and apply the changes one by one. To apply changes we have to:

  1. calculate the path from the affected ShadowNode to the root using ShadowNodeFamily::getAncestors
  2. traverse the path upwards cloning all the nodes up to the root

This way we unfortunately clone some ShadowNodes multiple times. For example for a batch of size n we will clone the root node n times.

Cloning ShadowNodes is expensive, so we had implemented an optimization - whenever a node is unsealed we would change it in place instead of cloning it. Unfortunately this didn't work, since the getSealed method always returns true in Production mode. This is not a bug, but the intended behavior, as sealing is only intended to help finding bugs in the Debug Mode. This still could be salvaged by memoizing which nodes were already cloned by us, but this approach still wouldn't be perfect, as modyfing nodes in place is still a heavy operation.

New approach

To mitigate those issues we split the process into two phases:

  1. calculate the subtree of the ShadowTree that contains all the nodes that we want to update
  2. traverse the ShadowTree and clone nodes (that belong to the subtree) in the (reversed) topological order

By calculating the subtree first we ensure that in the second phase:

  1. we traverse only nodes that absolutely have to be traversed
  2. we clone only nodes that absolutely have to be cloned
  3. we clone every node at most once

With this approach the second phase is performed in the optimal number of operations.

Limitations

The current implementation of phase one (building the subtree) is not optimal. It is implemented by simply calling getAncestors on every node from the batch. This is fortunately not a huge problem, because cloning had a much heavier impact on the performance. To optimize this there will have to be some changes done in RN (because the parent field in ShadowNodeFamily is private, so traversing the tree upwards is only possible through getAncestors). I hope to soon open a suitable PR.

Some examples

I checked the performance of our heavier examples on some devices in the Release Mode. For the BokehExample.tsx the results are:

Phone Example size Before [FPS] After [FPS]
iPhone 12 mini 200 30-40 60
One+ A6 100 10-20 30-40
iPhone 15 Pro 250 30-40 120
Samsung Galaxy S23 100 55-70 120

I also tested through Xcode Instruments how much time does the performOperations function take on the same example. Tests were conducted on the iPhone simulator, but they should give an idea on the order of the number of operations this function makes (and how fast that number grows in relation to the example size).

Example size Before [ms] After [ms] Before/After
1 1.95 2.1 0.92
20 2.4 2.1 1.14
100 5.3 2.3 2.3
250 22 4 5.5
500 77 7.7 10

Test plan

Check the behavior of examples in the FabricExample app. Verify that heavy examples have improved, while simpler examples have not regressed.

@tomekzaw tomekzaw requested review from tomekzaw and removed request for tomekzaw July 8, 2024 19:57
@ReactorCSA
Copy link

Thanks for this great library!!!
My question may be stupid but anyways, will this fix also improve performance on Paper architecture?
Thanks in advance :)

@bartlomiejbloniarz
Copy link
Contributor Author

Hi @ReactorCSA!
This optimization is only for the New Architecture, since Shadow Tree is only a New Architecture concept.

@janicduplessis
Copy link
Contributor

@bartlomiejbloniarz I've tested this in my app and it causes a crash on screens that use @gorhom/bottom-sheet. Sorry I don't have an exact repro, but here's the crash report I get.

image

@bartlomiejbloniarz
Copy link
Contributor Author

@janicduplessis Thank you for testing this PR 🙏

@bartlomiejbloniarz
Copy link
Contributor Author

bartlomiejbloniarz commented Jul 17, 2024

@janicduplessis could you test your app again? I changed the implementation so that we don't allocate RawProps on the heap (sadly @tomekzaw was right 😭). From my tests @gorhom/bottom-sheet doesn't crash anymore, but it would be cool to see if your app is fine now 🙏

@bartlomiejbloniarz
Copy link
Contributor Author

The issue wasn't caused by the usage of RawProps on its own. It was actually due to using the same RawProps multiple times. This happened because the ChildMap was populated wrongly (i.e. my skill issue). I decided to remove the abstraction and go back to using RawProps.

@mrousavy
Copy link
Contributor

A bit unrelated, but looking at this code I can see that you can access the props of a component as jsi::Values - how did you get to that point?

I'm trying to build a simple component (iOS/Android) where the props layer is implemented in C++/JSI, and I manually pass down the props to ObjC/Java myself - but I couldn't figure out how to get access to raw props as jsi::Values...

@janicduplessis
Copy link
Contributor

I’ll test it again in the next few days

@janicduplessis
Copy link
Contributor

@mrousavy I think here the props as jsi::Value is new props to update the components with, not the current props of the component.

@bartlomiejbloniarz
Copy link
Contributor Author

@mrousavy As @janicduplessis mentioned, the props here are the new props. They come from the reanimated updateProps function

Copy link
Collaborator

@tjzel tjzel left a comment

Choose a reason for hiding this comment

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

Looks good! 🚢 🇮🇹

@bartlomiejbloniarz bartlomiejbloniarz added this pull request to the merge queue Jul 29, 2024
Merged via the queue into main with commit a4c5c5d Jul 29, 2024
12 checks passed
@bartlomiejbloniarz bartlomiejbloniarz deleted the @bartlomiejbloniarz/improve-shadow-tree-cloning branch July 29, 2024 08:57
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.

None yet

8 participants