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

feat(posts): TVJS scroll performance enhancement #458

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

BernierMaxence
Copy link

@BernierMaxence BernierMaxence commented Nov 27, 2024

Article on an R&D project

We sent on R&D project to prod, to enhance performance when scroll through the TVJS app.
Here is an article on the implementation and it's gains 🚀

Find the preview here ➡️ https://pr-458.dsvmt7xpjktgx.amplifyapp.com/2024/11/22/tvjs-scroll-performance-enhancement.html

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-458.dsvmt7xpjktgx.amplifyapp.com

@BernierMaxence BernierMaxence marked this pull request as ready for review November 29, 2024 09:19
Copy link
Contributor

@CharlesMenier CharlesMenier left a comment

Choose a reason for hiding this comment

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

👏 🎉

@@ -164,6 +164,9 @@ m_alves:
m_benali:
name: Marwa Ben Ali
avatar: /images/avatar/m_benali.jpg
m_bernier:
name: Maxence Bernier
avatar: https://ca.slack-edge.com/T108ZKPMF-U01AX1WF1UZ-e7a81d9b8dec-512
Copy link
Contributor

Choose a reason for hiding this comment

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

Je crois que c'est mieux d'utiliser une photo de chez nous plutôt que depuis slack


Scrollable lists can be of various sizes and even include paginated content. In cases of paginated content, the next page is fetched preemptively during scroll, when the focus reaches a certain threshold.

Our old scroll component worked as follows: we would render a whole list of items, in a parent component handling scroll. When scrolling horizontally, the focus would switch to the next item. This would notify the parent component in charge of scroll, that would move the whole list laterally. The movement was computed from the measurements of the focused item, the size of the list, and the size of the container.
Copy link
Contributor

Choose a reason for hiding this comment

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

Un petit schéma pour illustrer ça serait parfait 🙏

Suggested change
Our old scroll component worked as follows: we would render a whole list of items, in a parent component handling scroll. When scrolling horizontally, the focus would switch to the next item. This would notify the parent component in charge of scroll, that would move the whole list laterally. The movement was computed from the measurements of the focused item, the size of the list, and the size of the container.
Our old scroll component worked as follows: we would render a whole list of items, in a parent component handling scroll. When scrolling horizontally, the focus would switch to the next item. This would then notify the parent component in charge of scrolling and move the whole list laterally. The movement was computed from the measurements of the focused item, the size of the list, and the size of the container.

Copy link
Author

@BernierMaxence BernierMaxence Nov 29, 2024

Choose a reason for hiding this comment

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

Même si c'est pas vraiment le sujet ? En gros là j'explique l'ancien pour comprendre les limitations, mais l'ancien c'est pas le propos de l'article, donc je voulais pas y passer trop de temps. T'en pense quoi ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oui pas faux. Je me disais qu'avec une illustration c'était toujours plus facile de comprendre mais c'est pas obligé. C'est très bien comme ça 👍

_posts/2024-11-22-tvjs-scroll-performance-enhancement.md Outdated Show resolved Hide resolved
# [Virtualization](#virtualization)
To address the first shortcoming of the initial approach, we introduced virtualization. Virtualization is a technique to render only the items that are visible on the screen.

For context, the content of the list is stored in a redux store, normalized: to select a specific item from the store, all you need is its index in the array of items for the corresponding list.
Copy link
Contributor

Choose a reason for hiding this comment

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

Rephrase

Suggested change
For context, the content of the list is stored in a redux store, normalized: to select a specific item from the store, all you need is its index in the array of items for the corresponding list.
For context, the content we display on each list is normalized and stored in a redux store. All the items are in an array and can be selected by their respective index.

_posts/2024-11-22-tvjs-scroll-performance-enhancement.md Outdated Show resolved Hide resolved

# [Optimised Rendering with React Keys](#optimised-rendering-with-react-keys)

The heart of the implementation is to fill each cell with a new item at each scroll. From the point of view of a single cell, when we scroll, the item it displays is new. But we know that the item already existed in the DOM, just one cell over. This is where we can leverage React's keys mechanism. Items rendered use a key that combines the original cell index with the current scroll offset. These keys help React reconcile the item in cell 1 before render as the item in cell 2 after render as the same item, thus reusing the same DOM node. As a result, we get 0 re-renders for the items that are shifting places, significantly reducing the performance impact of a scroll.
Copy link
Contributor

Choose a reason for hiding this comment

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

Un petit saut de ligne pour couper le paragraphe et fluidifier la lecture et un peu de wording
Peut-être un petit schéma aussi pour l'explication des clés vu que c'est très technique 😄 ?

Suggested change
The heart of the implementation is to fill each cell with a new item at each scroll. From the point of view of a single cell, when we scroll, the item it displays is new. But we know that the item already existed in the DOM, just one cell over. This is where we can leverage React's keys mechanism. Items rendered use a key that combines the original cell index with the current scroll offset. These keys help React reconcile the item in cell 1 before render as the item in cell 2 after render as the same item, thus reusing the same DOM node. As a result, we get 0 re-renders for the items that are shifting places, significantly reducing the performance impact of a scroll.
The heart of the implementation is to fill each cell with a new item at each scroll. From the point of view of a single cell, when we scroll, the item it displays is new. But we know that the item already existed in the DOM, just one cell over. This is where we can leverage React's keys mechanism.
Rendered items use a key that combines the original cell index with the current scroll offset. These keys help React reconcile the item in cell 1 before rendering as the item in cell 2 after rendering as the same item, thus reusing the same DOM node. As a result, we get 0 re-renders for the items that are shifting places, significantly reducing the performance impact of a scroll.

Copy link
Author

Choose a reason for hiding this comment

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

Je sais même pas quoi faire comme schema 😆
Faute de mieux pour l'instant, j'ai mis la doc de React sur les keys :itssomething:

Copy link
Contributor

Choose a reason for hiding this comment

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

Oui c'est vrai que c'est pas simple à illustrer ça 🤔
La doc suffira j'imagine 🤷


| Before | After |
|-|-|
| 11615ms | 8631ms (-26%) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Le top du top serait d'avoir un .gif avec les deux cas où on peut voir la différence 😇

Copy link
Author

Choose a reason for hiding this comment

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

J'ai essayé, mais un gif c'est pas du tout représentatif : on se ne rend pas compte de la vitesse à laquelle on appuie sur la flèche, et il y a pas beaucoup de frames donc ça ne capture pas le lag 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah oui, dommage 😢
C'est toujours cool d'avoir le petit effet visuel avant/après mais tant pis 😅

Copy link
Author

Choose a reason for hiding this comment

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

Je vais essayer !

Copy link

@qtomasicchio qtomasicchio left a comment

Choose a reason for hiding this comment

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

Thanks for the article! 🎉
I added a few suggestions (all 🥖), feel free to accept them or not

_posts/2024-11-22-tvjs-scroll-performance-enhancement.md Outdated Show resolved Hide resolved
_posts/2024-11-22-tvjs-scroll-performance-enhancement.md Outdated Show resolved Hide resolved
_posts/2024-11-22-tvjs-scroll-performance-enhancement.md Outdated Show resolved Hide resolved

Scrolling down in a page with lighter lists is also more efficient. Here, measurements were taken during a scroll down 25 lists.

Beyond benchmarks, on-device tests were also conclusive: the scroll is smoother, we almost eliminate the lag caused by a pagination fetch. Overall, it feels better to scroll through a list.

Choose a reason for hiding this comment

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

🥖 It could be nice to have video before/after from a device to visually see the improvment, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Videos are tough : since you don't have the information of how fast the arrows are pressed, or the delay between the press and the scroll on screen, it's hard to really gauge the performance
I'll try, see what I can get, but I remember struggling with this when I first sent the scroller to tech review 😄

Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

👏

@@ -164,6 +164,9 @@ m_alves:
m_benali:
name: Marwa Ben Ali
avatar: /images/avatar/m_benali.jpg
m_bernier:
name: Maxence Bernier
avatar: /images/avatar/m_bernier.jpeg
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
avatar: /images/avatar/m_bernier.jpeg
avatar: /images/avatar/m_bernier.png

because the image won't appear if it's not the correct filename

color: rgb(251,87,66)
---

A core experience of a Bedrock app for the end user is browsing the catalogue. Scrolling vertically through blocks of content, and scrolling horizontally through lists of items. TVs do not offer high performance and provide poor user experience during heavy resource actions. Namely, we noticed that scrolling horizontally in a list was laggy and unpleasant. This article focuses on performance optimization to enhance the horizontal scroll experience.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A core experience of a Bedrock app for the end user is browsing the catalogue. Scrolling vertically through blocks of content, and scrolling horizontally through lists of items. TVs do not offer high performance and provide poor user experience during heavy resource actions. Namely, we noticed that scrolling horizontally in a list was laggy and unpleasant. This article focuses on performance optimization to enhance the horizontal scroll experience.
One of the core experiences of a Bedrock app for the end user is browsing the catalogue. Scrolling vertically through blocks of content, and scrolling horizontally through lists of items. However, TVs do not offer high performance and provide poor user experience during heavy resource actions. We especially noticed that scrolling horizontally in a list was laggy and unpleasant on TVs. This article focuses on performance optimization to enhance the horizontal scroll experience.

because I felt that this block was not easy to read/understand

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add "on TV" at the very end of the paragraph. Yes it's a repetition, but makes the goal of the article 100% clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants