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

Perf: allow useMemoAll to cache results independently from deps #5172

Merged
merged 9 commits into from
May 8, 2024

Conversation

OEvgeny
Copy link
Collaborator

@OEvgeny OEvgeny commented May 7, 2024

Fixes #

Changelog Entry

  • Improved performance for useActivityWithRenderer, in PR #5172, by @OEvgeny

Description

Thanks to the optimization, now subsequent activity re-renders are cached. The downside is that the useMemoAll hook cached results are not discarded when useMemo dependencies are updated.

Design

The initial implementation provided in the PR was caching per functions globally (see the first commit). Now we cache per hook call to align better with useMemo.

Note that if we aim to maintain full parity with useMemo, we shouldn't make this optimization. But it'd make the implementation similar to defining and using the memoized function directly in useMemo hook making this hook obsolete.

The updated implementation is rather not as trivial and solves use-cases for caching results even after dependencies change,

Specific Changes

  • Updated useMemoAll hook implementation
  • Renamed: useMemoize -> useMemoAll as suggested
  • Updated tests
  • Added activity render count test

-

  • I have added tests and executed them locally
  • I have updated CHANGELOG.md
  • I have updated documentation

Review Checklist

This section is for contributors to review your work.

  • Accessibility reviewed (tab order, content readability, alt text, color contrast)
  • Browser and platform compatibilities reviewed
  • CSS styles reviewed (minimal rules, no z-index)
  • Documents reviewed (docs, samples, live demo)
  • Internationalization reviewed (strings, unit formatting)
  • package.json and package-lock.json reviewed
  • Security reviewed (no data URIs, check for nonce leak)
  • Tests reviewed (coverage, legitimacy)

@OEvgeny OEvgeny marked this pull request as ready for review May 7, 2024 01:53
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
packages/component/src/hooks/internal/useMemoAll.ts Outdated Show resolved Hide resolved
packages/component/src/hooks/internal/useMemoAll.ts Outdated Show resolved Hide resolved
packages/component/src/hooks/internal/useMemoAll.spec.jsx Outdated Show resolved Hide resolved
__tests__/html/renderActivity.performance.html Outdated Show resolved Hide resolved
@compulim
Copy link
Contributor

compulim commented May 7, 2024

Literally we need to select one of 2 routes:

  • useMemo(fn, deps), which change to fn will not re-run. Change to deps will cause a re-run
  • useSyncExternalStore(subscribe, getSnapshot), which change to subscribe or getSnapshot will always cause a re-run

I think we should follow useMemo.

What I observed from React API:

  • useSyncExternalStore do not accept DependencyList so it invalidate solely based on changes to subscribe and getSnapshot
  • In contrast, useMemo accept DependencyList so it invalidate solely based on deps and ignore changes to fn

@OEvgeny OEvgeny requested a review from compulim May 7, 2024 16:26
@OEvgeny
Copy link
Collaborator Author

OEvgeny commented May 7, 2024

Instead of useMemoAll we probably could go with useMemo plus the memoized function to retrieve activity related data.

compulim
compulim previously approved these changes May 8, 2024
Copy link
Contributor

@compulim compulim left a comment

Choose a reason for hiding this comment

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

Nitpicking.

OEvgeny and others added 2 commits May 8, 2024 21:45
…itiesWithRenderer.ts

Co-authored-by: William Wong <compulim@users.noreply.github.com>
@OEvgeny OEvgeny requested a review from compulim May 8, 2024 21:49
@compulim compulim enabled auto-merge (squash) May 8, 2024 21:49
@compulim compulim merged commit d3a6f52 into main May 8, 2024
25 checks passed
@compulim compulim deleted the perf/activity-render branch May 8, 2024 22:10
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.

2 participants