refactor(container,lazy,hooks): avoid blocking JS thread by refactoring SharedValue.value access #423

AndreiCalazans · 2024-06-18T17:53:24Z

Hey @andreialecu and @gkartalis (tagging bc I can't add reviewers), I was introducing this library to solve a UI pattern and noticed blocking JavaScript thread behavior that caused a regression of over 500ms to our initial screen render.

The flame graph above shows that the JavaScript thread is blocked by executeOnUIRuntimeSync for quite some time impacting the initial render time of a screen by over 500ms.

On closer analysis I found that the problem is caused by reading a SharedValue.value in the main JS thread. This library does this quite often in places that could avoid it. This PR refactors most of the places where it does not need to be a SharedValue while trying to not cause any regressions.

I understand there isn't unit or e2e tests for this library so I tried my best to test the Example app. I posted multiple recordings below of how it currently works with these changes.

I also wrote a detailed blogpost explaining this Reanimated blocking behavior.

You can also inspect the before and after CPU profilings, I stored them here. The CPU profile will show that before these changes there were 57 calls to executeOnUIRuntimeSync when navigating from the main menu to the FlashList example in the Example app and now there are only 14 calls.

Before (57 calls)	After (14 calls)

Demos

One	Two
demo-1-reduced.mp4	demo-2-reduced.mp4

Three	Four
demo-3-reduced.mp4	demo-4-reduced.mp4

Five	Six
demo-5-reduced.mp4	demo-6-reduced.mp4

Seven	Eight
demo-7-reduced.mp4	demo-8-reduced.mp4

AndreiCalazans · 2024-06-18T18:19:46Z

src/Container.tsx

- React.useEffect(() => {
- if (index.value >= tabNamesArray.length) {
- onTabPress(tabNamesArray[tabNamesArray.length - 1])
+ useAnimatedReaction(
+ () => tabNamesArray.length,
+ (tabLength) => {
+ if (index.value >= tabLength) {
+ runOnJS(onTabPress)(tabNamesArray[tabLength - 1])
+ }
 }
- }, [index.value, onTabPress, tabNamesArray])
+ )


This is essentially the same as was just instead of running inside a useEffect it runs inside a useAnimatedReaction to avoid blocking the JS thread.

AndreiCalazans · 2024-06-18T18:20:18Z

src/Container.tsx


 const focusedTab: ContextType['focusedTab'] =
 useDerivedValue<TabName>(() => {
 return tabNames.value[index.value]
 }, [tabNames])
- const calculateNextOffset = useSharedValue(index.value)
+ const calculateNextOffset = useSharedValue(initialIndex)


This change is safe to do because the initial value passed to useSharedValue will only be captured once on first pass, this is not reactive.

AndreiCalazans · 2024-06-18T18:21:05Z

src/hooks.tsx

+ (event: LayoutChangeEvent) => {
+ const latestHeight = event.nativeEvent.layout.height
+ if (latestHeight !== height) {
+ setHeight(latestHeight)


It's safe make a layout value a regular React state because updates to this will not cause an animation change, these values are only used as static values within animation triggers.

Is this check necessary? doesn't react not even re-render if the value is the same?

I believe this is actually an anti-pattern, since now getHeight relies on the height variable, causing it to update its reference based on its parent state when this isn't necessary.

For example, you could just dispatch it with a callback too, right?

setHeight(height => height === latestHeight ? height : latestHeight)

However, that becomes the same as just doing setHeight(latestHeight).

Maybe I'm missing something, but I believe that's the case.

Maybe relevant: facebook/react#28779

Also see:
https://legacy.reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update

Bailing out of a state update
If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects. (React uses the Object.is comparison algorithm.)

Note that React may still need to render that specific component again before bailing out. That shouldn’t be a concern because React won’t unnecessarily go “deeper” into the tree. If you’re doing expensive calculations while rendering, you can optimize them with useMemo.

The problem here is that all the code in the component will run again unnecessarily, even though it doesn't go "deeper" or actually render anything. By preventing setting the state, we can optimize against that.

andreialecu · 2024-06-18T19:52:47Z

Will look over this tomorrow.

Thanks for contributing!

andreialecu · 2024-06-19T10:15:24Z

Very interesting, I didn't know about this behavior of Reanimated. Is it documented anywhere?

Could it be a bug in Reanimated, maybe a recent regression? I don't remember running into an issue before or hearing about SharedValue blocking JS threads.

@piaskowyk could you kindly shed some light? See Andrei Calazan's blog post and reddit posts:
https://andrei-calazans.com/posts/reanimated-blocking-js-thread/
https://www.reddit.com/r/reactnative/comments/1diy3pc/reanimated_can_block_the_main_javascript_thread/

I remember introducing SharedValue here specifically to improve performance in the past. For example to be able to do this on the UI thread:
9378af6#diff-739a99e40f851900557fbc98026c870d8ac3c2da65ec6fc29d60c161effaf20dR240-R248

AndreiCalazans · 2024-06-19T12:29:11Z

Hey @andreialecu, thanks for reviewing.

I also didn't know this until now, not sure if it's a regression since the behavior kinda of makes sense if you think about it. Any time you need to read a SharedValue.value from within the JavaScript thread it needs to bridge the value synchronously from the HostObject so that can have a non-trivial cost (anywhere from 5 to 20ms depending on device).

I want to highlight that the problem is not using SharedValue but repeatedly needing to read it from within the JS thread. The example diff you shared is a good example of the correct pattern which is to take something from the JS thread and run it in worklet to save CPU cost in the JS thread.

In the blogpost I tried to come up with a mental model of when something should be a SharedValue.

andreialecu · 2024-06-19T12:35:43Z

Are you using the new architecture?

With Paper/old architecture, as far as I remember, calling SharedValue.value in the past did not guarantee you'd get the actual last value. It was sometimes out of sync with the value from a worklet. Hence why I needed to create the useConvertAnimatedToValue hook, to try to ensure the value is kept in sync as much as possible.

AndreiCalazans · 2024-06-19T13:09:31Z

@andreialecu I'm not using the new arch nor did I test in it.

AndreiCalazans · 2024-06-19T13:11:23Z

@andreialecu note that my changes avoids this interchange of SharedValue to regular React state and solely relies on React state where possible.

andreialecu · 2024-06-19T13:13:56Z

src/Container.tsx

@@ -242,7 +236,7 @@ export const Container = React.memo(
 )

 useAnimatedReaction(
- () => headerHeight.value,
+ () => headerHeight,


Does this actually work right? Can reanimated trigger a reaction based on a react state change?

Yea, it does work, I added logs to validate.

andreialecu · 2024-06-19T13:18:31Z

@andreialecu note that my changes avoids this interchange of SharedValue to regular React state and solely relies on React state where possible.

I understand.

It's just that this transition to animated values was actually done on purpose, and it greatly improved performance. I'm trying to understand what changed recently for this to no longer be the case.

If accessing shared values from JS was indeed such performance-intensive, I think that it would've been discovered by others in the past. I find it odd that this is the first time I'm hearing about this, and I couldn't find other mentions of this potential problem anywhere else.

Reanimated has regressions some times, so it would be great to understand more about this from an upstream maintainer. Perhaps we can ping one on Twitter/X

andreialecu · 2024-06-19T13:25:42Z

What I think would help is if you could create a separate, minimal repro of this SharedValue issue, as a quick benchmark. Then we can perhaps run it on various reanimated versions and see if it's indeed a regression.

AndreiCalazans · 2024-06-19T16:16:01Z

@andreialecu I understand the pushback. It will be helpful to hear from @piaskowyk to see if my understanding is correct.

I think the Example app is enough of a minimal repro. I pushed here a simple setup that allows us to run performance tests using Flashlight.

I ran these e2e tests with 10 iterations for main and for this branch. All tests were ran on the same Samsung a23 device (considered low end). These are the results:

At a first glance it seems like nothing has changed and this branch is just slightly better, but take a look at when the FPS starts dipping when we navigate to the FlashList example. On main the dip happens at around 3 seconds for the 10 iterations while on the refactored branch it happens around 2.5 seconds this means this branch is half a second faster to load the components and dispatch the paint to UI natively.

FPS Dip on Main	FPS Dip on this branch

Flashlight reports:

To visualize these yourself install Flashlight and run flashlight report fileone filetwo

andreialecu · 2024-06-21T19:03:13Z

I released this as 8.0.1-rc.0 to npm for the time being, so that we can all test it more easily.

andreialecu · 2024-06-24T16:37:23Z

src/Lazy.tsx

+ const shouldStartMounted =
+ typeof _startMounted === 'boolean'
+ ? _startMounted
+ : focusedTab.value === name


It seems that reading .value in render is not recommended, according to: https://x.com/kzzzf/status/1805222107011317861

This is not your fault though, the code previously did this too.

andreialecu · 2024-07-01T08:19:19Z

Released latest changes as 8.0.1-rc.1

CHANGELOG.md

This is a performance optimization to avoid blocking the JavaScript thread. BREAKING CHANGE: headerHeight, tabBarHeight, containerHeight, and contentInset are no longer SharedValues. If you consume useHeaderMeasurements and/or useTabsContext expect this to impact you.

AndreiCalazans marked this pull request as ready for review June 18, 2024 18:16

AndreiCalazans commented Jun 18, 2024

View reviewed changes

andreialecu reviewed Jun 19, 2024

View reviewed changes

andreialecu reviewed Jun 24, 2024

View reviewed changes

AndreiCalazans commented Jul 5, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

AndreiCalazans force-pushed the andrei/optimize-tab-view branch from 40fb293 to d8be0d9 Compare July 5, 2024 14:03

AndreiCalazans force-pushed the andrei/optimize-tab-view branch from d8be0d9 to 08a1c51 Compare July 5, 2024 14:17

andreialecu merged commit 6e9cbd4 into PedroBern:main Jul 5, 2024
1 check failed

AndreiCalazans deleted the andrei/optimize-tab-view branch July 5, 2024 14:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(container,lazy,hooks): avoid blocking JS thread by refactoring SharedValue.value access #423

refactor(container,lazy,hooks): avoid blocking JS thread by refactoring SharedValue.value access #423

AndreiCalazans commented Jun 18, 2024 •

edited

Loading

AndreiCalazans Jun 18, 2024

AndreiCalazans Jun 18, 2024

AndreiCalazans Jun 18, 2024

nandorojo Jun 21, 2024 •

edited

Loading

andreialecu Jun 21, 2024

andreialecu Jun 21, 2024

andreialecu commented Jun 18, 2024

andreialecu commented Jun 19, 2024 •

edited

Loading

AndreiCalazans commented Jun 19, 2024 •

edited

Loading

andreialecu commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024

andreialecu Jun 19, 2024

AndreiCalazans Jun 20, 2024

andreialecu commented Jun 19, 2024

andreialecu commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024 •

edited

Loading

andreialecu commented Jun 21, 2024

andreialecu Jun 24, 2024

andreialecu commented Jul 1, 2024

refactor(container,lazy,hooks): avoid blocking JS thread by refactoring SharedValue.value access #423

refactor(container,lazy,hooks): avoid blocking JS thread by refactoring SharedValue.value access #423

Conversation

AndreiCalazans commented Jun 18, 2024 • edited Loading

Demos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandorojo Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreialecu commented Jun 18, 2024

andreialecu commented Jun 19, 2024 • edited Loading

AndreiCalazans commented Jun 19, 2024 • edited Loading

andreialecu commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreialecu commented Jun 19, 2024

andreialecu commented Jun 19, 2024

AndreiCalazans commented Jun 19, 2024 • edited Loading

andreialecu commented Jun 21, 2024

Choose a reason for hiding this comment

andreialecu commented Jul 1, 2024

AndreiCalazans commented Jun 18, 2024 •

edited

Loading

nandorojo Jun 21, 2024 •

edited

Loading

andreialecu commented Jun 19, 2024 •

edited

Loading

AndreiCalazans commented Jun 19, 2024 •

edited

Loading

AndreiCalazans commented Jun 19, 2024 •

edited

Loading