Improve performance of recomputeDependents

[samkline]

Related Issues or Discussions

N/A

Summary

Hi. I've been working with @edkimmel on performance improvements to our app. It's a React Native ecommerce app that heavily relies on Jotai for managing most state and data. A hotspot that stood out recently was recomputeDependents—that's how Eddie identified the Hermes memory leak (#2355) as a source of our problems.

Even with Eddie's workaround in place for that memory leak, we still found that recomputeDependents was very slow for us, so we took a closer look at what recomputeDependents is doing. It's something like this:

• loop1 is O(n*m) where n is the number of atoms and m is the number of dependency edges.
• loop2 is O(n*n*m) because it does the same thing as loop1 but also iterates over the dependencyMap for each iteration where the atom is unchanged (that is, roughly n*m times).

This can be improved by doing a topological sort of the atom dependency graph, which is O(n). This PR changes the recomputeDependents implementation to do that topsort. All existing tests pass.

I ran through a test case for our app that represents a common customer flow. The total accumulated time in recomputeDependents is as follows:

Platform	Time before	Time after	Percent improvement
Web/Chrome	1,483ms	295ms	80%
iOS React Native (Hermes)	3,581ms	367ms	90%
Android React Native (Hermes)	4,137ms	402ms	90%

Thanks again for all of your work on Jotai, we're big fans!

Check List

• yarn run prettier for formatting code and docs

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
jotai	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jan 31, 2024 2:56pm

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0433733:

Sandbox	Source
React	Configuration
React TypeScript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
Next.js	Configuration
Next.js with custom Babel config	Configuration
React with custom Babel config	Configuration

[dai-shi]

Thanks for working on this!
Yes, I was pretty sure that there should be better algorithm then loop1&loop2.
So great to see this.
Some comments below:

[samkline]

Thanks for the quick review @dai-shi ! I updated the branch with your comments, and this should fix the lint violations as well.

[dai-shi]

Looks great. A few minor comments:

[samkline]

For context on the latest commit (4cb7b83), see my comment in #2368 (reply in thread):

Nice find @cltnschlosser ! I just updated the PR #2363 with a fix. For those out of the loop, the issue applies when the code is transpiled to support older JS versions. The transpiled code has an issue with using Array.prototype.push with the Set iterator, such that the push does nothing. It worked fine in modern JS environments. This was the fix: 4cb7b83

[dai-shi]

For context on the latest commit (4cb7b83), see my comment in #2368 (reply in thread):

Nice. Can you please add comment in source code so that we know it's the workaround for transpilation?

[samkline]

All set @dai-shi , and I've rebased on top of the latest main

[dai-shi]

LGTM
Thanks so much for your contribution!

I will merge this when I prepare the next release.

[samkline]

@dai-shi Could you please take one more look? I found another performance optimization, specifically, step 1 where we build allAtomsInDependencyGraph was entirely unnecessary. This is because recomputeDependents only ever has exactly one "root" atom in the graph (that is, one node without incoming edges), and we already know which atom it is. This is a nice simplification and avoids the code that had the transpilation issue earlier as well. The change is 94fe613 .

[dai-shi]

It looks simplified!

[yf-yang]

jotai-scope is good with 94e356b