PLAT-109381: Reduce the ImageItem render time

[ybsung]

Checklist

• I have read and understand the contribution guide
• A CHANGELOG entry is included
• At least one test case is included for this feature or bug fix
• Documentation was added or is not needed
• This is an API breaking change

Issue Resolved / Feature Added

One of the major cause for the list scroll performance is the long JS execution time of the ImageItem. So we need to reduce the ImageItem render time.

Resolution

1. Memoization of the kind computed values and some React elements.
2. Pass children for caption, label for subcaption, imageIconComponent, imageIconSrc, and src not as props but thought a context so that we don't have to render several components again.
3. Update the caption, subcaption, and data-index DOM elements directly instead of rendering components.

Additional Considerations

Links

PLAT-109381
Related PR: enactjs/sandstone#461
Previous Branch: https://github.com/enactjs/enact/tree/PLAT-109381-useMemo2

Comments

Enact-DCO-1.0-Signed-off-by: YB Sung ([email protected])

[codecov]

Codecov Report

Merging #2790 into develop will increase coverage by 0.18%.
The diff coverage is 93.33%.

@@             Coverage Diff             @@
##           develop    #2790      +/-   ##
===========================================
+ Coverage    44.61%   44.80%   +0.18%     
===========================================
  Files          163      164       +1     
  Lines         8158     8187      +29     
  Branches      1991     1994       +3     
===========================================
+ Hits          3640     3668      +28     
- Misses        3391     3392       +1     
  Partials      1127     1127              

Impacted Files	Coverage Δ
packages/ui/ImageItem/MemoPropsDecorator.js	87.50% <87.50%> (ø)
packages/ui/ImageItem/ImageItem.js	100.00% <100.00%> (ø)
packages/ui/Layout/Layout.js	100.00% <0.00%> (+5.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4e3d22...da1c155. Read the comment docs.

[MikyungKim]

Much better shape! Thank you for your efforts.
LGTM!

[webOS101]

It's not completely clear to me that each of these changes is required to get the performance necessary. In fact, there may be performance and memory regressions from this. Certainly this is much more complicated and it may not be a general solution that could be applied to different components. I'm also not certain that many of the issues might not be resolved by making portions of the components Pure or by taking other approaches other than using context.

There's not a clear breakdown by how much improvement each piece of this gives to the overall performance. Because of the complexity, I don't think we can move forward with this solution right now.

If we really need to go further, then it seems like we might want to just directly mutate the DOM in these special instances. At least, we'd know the trade-offs we're making in terms of performance for this (and other) components.

[webOS101]

You don't need to delete css, I believe it's non-enumerable.

[webOS101]

This will force the component to update every time it receives props, even if the values are exactly the same, because the object has changed and context will force the update. Some form of memoization should be done if this truly is the desired solution.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}