Improve Settings screen opening performance

[ilmotta]

Summary

In this PR we're rewriting utils.re-frame/delay-render to use hooks. The new implementation renders significantly better than what we have today, at least on Android.

Current (develop) on a Samsung Galaxy A71, release build, Android 13

current.mp4

New (PR) on a Samsung Galaxy A71, release build, Android 13

new.mp4

Areas that may be impacted

None.

Why not hiccup instead of a function call to delay-render?

The Settings screen is rendered slightly faster if I use delay-render as a function call instead of hiccup. My only guess is that this is just less work to be done by Reagent, since the wrapper function is not creating a wrapper component with its own lifecycle.

Naming

I would keep the name delay-render, as a verb (like any ordinary function), because it tells the developer that it shouldn't be treated as a component (which is named as a noun most of the time).

Steps to test

As a reviewer, I kindly ask you to check this PR's build and compare with the experience from the develop branch of opening the Settings screen a few times. Is it better? Could you share a screen recording on Android/iOS?

status: ready

[status-im-auto]

Jenkins Builds

Click to see older builds (1)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	17392a0	#1	2024-05-08 12:58:02	~2 min	tests	📄log

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	a657485	#3	2024-05-08 13:04:28	~4 min	tests	📄log
✔️	a657485	#3	2024-05-08 13:06:53	~6 min	android-e2e	🤖apk 📲
✔️	a657485	#3	2024-05-08 13:07:02	~6 min	android	🤖apk 📲
✔️	a657485	#3	2024-05-08 13:10:40	~10 min	ios	📱ipa 📲
✔️	e9b88ec	#4	2024-05-10 14:32:35	~4 min	tests	📄log
✔️	e9b88ec	#4	2024-05-10 14:36:12	~8 min	android	🤖apk 📲
✔️	e9b88ec	#4	2024-05-10 14:36:13	~8 min	android-e2e	🤖apk 📲
✔️	e9b88ec	#4	2024-05-10 14:36:44	~8 min	ios	📱ipa 📲

[OmarBasem]

Hey @ilmotta, I have seen the discussion on discord about this. I checked out the builds in this PR and compared to develop on iOS and Android devices. In my experience both of develop and your PR are very and equally smooth. But I see in the videos you posted it performs better on your Android device (but seems not as smooth as on my device). I can make a small suggestion here, have you tried using reagent/next-tick instead of js/setTimeout 0 it can perform slightly better as it places the function on a micro-task queue rather than a macro-task queue like setTimeout

(defn delay-render
  [content]
  (let [[render? set-render] (use-state false)]
    (use-mount
     (fn []
       (reagent/next-tick #(set-render true))))
    (when render?
      content)))

Here is an Android build that uses that

[ilmotta]

Hi @OmarBasem, thanks a lot for reviewing the build and code. I expected iOS would perform nearly the same because things are much smoother over there. Now that you confirmed this, I guess we can say this PR is really targeting Android.

I remember when delay-render was first implemented there was a comment about using next-tick (maybe @ulisesmac said that), but I don't remember why we ended up using setTimeout (maybe because it's well known among devs and the performance was the same at the end of the day).

I'll check out with next-tick and see what comes out of it! Thanks

[ilmotta]

@OmarBasem, I tried with next-tick. It works the same on average. Here's how I compared them, which I think is not super scientific, but quite practical from the user's perspective.

1. I enabled show taps and recorded the current and new implementation opening and closing the Settings screen many times.
2. After the first video frame shows the white circle (press) I count the number for frames in the video until the Settings screen is rendered and animated at the top. There's a tiny margin for error counting like this, but should be precise enough.

The results (ignore absolute frame numbers, just how close they all are):

next-tick: 12 frames, 11 frames, 9 frames
setTimeout: 10 frames, 8 frames, 13 frames, 10 frames

So, I would say they perform essentially the same for this particular component, but I agree next-tick can theoretically be better, even if no practical advantage could be measured for the Settings screen. Even if the number of frames to render is the same, maybe next-tick could consume less resources? But anyway, I'll use next-tick because it's cleaner and gives the same effect.

[seanstrom]

I think next-tick could communicate the intent more clearly, because it seems we're trying to defer the rendering to relieve the cpu during some animations (?).

[OmarBasem]

But anyway, I'll use next-tick because it's cleaner and gives the same effect.

I think next-tick could communicate the intent more clearly

@ilmotta @seanstrom I agree, when I find both of next-tick and setTimeout 0 having the same effect practically I use next-tick as I think it communicates the intent better

[J-Son89]

should we make this solution android specific so? i.e split it by platform? 🤔

[OmarBasem]

@J-Son89 the solution introduced in Icaro's PR is to use a hook instead of hiccup for delay-render

As for next-tick, as discussed above the performance in this scenario is the same but we settled for it over setTimeout 0 because it is a little cleaner. It is not platform specific.

[J-Son89]

Cool cool, it's just that in your initial comment you said this was not as smooth for you on iOS.

Anyway I guess it fixes the bigger issue which is a bad android performance so that's great 🚀

[OmarBasem]

Lgtm either way! 👍

[seanstrom]

Performance seems consistently smooth for me on my side ✅

I wonder if there's a place for us to collect some of these performance findings? Because it seems we're beginning to practically see some benefits for avoiding hiccup (in some places) and using hooks instead of reagent atoms.

Perhaps this will help shape the future of improvements around using or not using hiccup and reagent atoms.

[ilmotta]

I don't intend to invest more time in this PR because I think the solution is good enough, but I had to dig more. What I found is probably useful knowledge to more CCs.

Based on my analysis of individual frames being rendered and having investigated 3 different scenarios:

1. Scenario 1: No delay whatsoever, i.e. not using delay-render.
2. Scenario 2: Using delay-render like in develop, that is, a form-2 component with a local Reagent atom.
3. Scenario 3: Using delay-render as in this PR, using hooks.

All 3 scenarios open the Settings screen with all rendered views in the same amount of time. In terms of raw performance, they are completely identical. The absolute value doesn't matter, but in my recordings, on average, 10 frames of video after the first press on the user's profile image.

So how can it be that on Android the new solution is visibly smoother? It's all about latency and our brains are very picky about it.

Scenario 1 - Not using delay-render

The user notices a longer delay after pressing on the profile image because all components in the Settings screen are mounted in one go. This gives the impression to the user of being slower. In slower Android devices, we've seen a user even press twice because the Settings screen was taking longer to open. On newer Android devices this is not much of a problem. There's another problem in Scenario 1, on Android, with too many elements and/or too many heavy elements being mounted, the opening animation is sometimes completely cut off or very clunky (a similar problem can happen while opening the Activity Center).

Scenario 2 - Use delay-render with a form-2 component

The Settings items are always rendered after the opening animation completes. Our brains perceive this as a slight delay because we can see the empty gray background for 1-3 frames. This is quite noticeable on my physical Android device, even with a prod build.

Caption: Settings items are rendered only after the animation completes. Here we see the last frame before they finally appear all at once on screen.

Scenario 3 - Use delay-render as a hook

The optimal solution from the user's perspective, Settings items sometimes can be rendered before the animation completes. I say sometimes because other times the items are rendered only 1 frame before or right when the animation completes, which would be almost the same as Scenario 2.

Caption: Settings items are sometimes rendered before the animation completes.

---

So, what can we conclude so far? I don't know! But we can't say re-frame is slower in the particular case of rendering the Settings screen, that would be incorrect. I was tricked!

What the hooks solution gave us is a little bit of the Scenario 1 and Scenario 2 in one package, and because the Settings items can be sometimes rendered before the opening animation completes, our brains see that as being faster.

In future performance investigations, we might want to focus on manipulating latency more aggressively to see where that leads us.

Thanks for reading this far!

[ilmotta]

Performance seems consistently smooth for me on my side ✅

I wonder if there's a place for us to collect some of these performance findings? Because it seems we're beginning to practically see some benefits for avoiding hiccup (in some places) and using hooks instead of reagent atoms.

Perhaps this will help shape the future of improvements around using or not using hiccup and reagent atoms.

@seanstrom, I think we don't have such a central place to look for performance findings. Maybe we have something in Notion. The closest we have in this repo I think is in doc/ui-guidelines.md. We should just be mindful of jumping into conclusions too early. We spoke in the offsite a little bit about raising the "performance baseline", which is aligned with your comment to document performance findings.

[OmarBasem]

@ilmotta thanks for sharing the above analysis. An interesting comparison indeed.

I can add one more thing, have you tried using initial-num-to-render on the settings flatlist? May further improve the performance. The default value is 10, but having 5 items would be enough to cover the view: :initial-num-to-render 5

[ilmotta]

@ilmotta thanks for sharing the above analysis. An interesting comparison indeed.

I can add one more thing, have you tried using initial-num-to-render on the settings flatlist? May further improve the performance. The default value is 10, but having 5 items would be enough to cover the view: :initial-num-to-render 5

I haven't tried to optimize the Settings screen in any other way @OmarBasem, this is my first time touching it. Yesterday I simply rewrote the delay-render function using hooks and I was very much surprised with the results on Android. And as you read my analysis, also surprised that the performance gains were only related to when things appear on screen but not the total time to render (so not really that Reagent is measurably slower to open the Settings).

Since I'm at this PR, I tried your suggestion with initial-num-to-render, but it made things slightly worse in the device I'm testing, which has a higher vertical screen space than an iPhone for example. The device I'm developing against is a Pixel 7 Pro and I can see 12 items on screen at once. Once I reduced to 5 I perceive a very tiny delay as the list is filled with the remaining items while the animation is still running.

But you gave me an idea (not that I'm going to implement) but since the vertical space varies wildly sometimes between devices, in the future, we could consider making :initial-num-to-render variable depending on the device's height. It really is a waste of resources to process 10 items at once in a device that only shows 5 on screen as you correctly pointed out.

Thanks again for the tips and insights 💯

[shivekkhurana]

What the hooks solution gave us is a little bit of the Scenario 1 and Scenario 2 in one package, and because the Settings items can be sometimes rendered before the opening animation completes, our brains see that as being faster.

Thank you for the explanation. The settings items showing up before animation completes creates a massive impact. It also clears two misconceptions in my head:

• Reagent/Ratoms always bad React/Hooks always good
• useMemo everywhere (not directly related to this PR, but your and Ullises's explanations)