Recoil: Atom count scalability

[Ruthenz]

Hey,

I'm not sure if this is a performance bug, or maybe too many atoms just overloads the system but I created a simple example of a Canvas component with width and height that each point is a Pixel component that gets it's state from an atom (with a specific id).

It works great with 20x20 canvas, a bit slow with 50x50 and just crashes completely with 100x100.

I created a sandbox example : https://codesandbox.io/s/crimson-thunder-e45pj?file=/src/Canvas.tsx

You can change the width and height and see what happens.

Would love to understand why this happens and if I'm doing something wrong :)

[acutmore]

Hi @Ruthenz.

I can't see anything specifically that you're doing wrong with Recoil. Only the affected Atoms are being triggered. I tried your setup with React's useState instead of useRecoilState and the performance was still not great.

I think the DOM updates might be the slowest part here. All the <label> are positioned with flex. I would try absolute positioning each pixel so the browser has less work to do.

[Ruthenz]

The problem was that the app completely crashes as opposed to working slow.
I also tried using React hooks and the performance was terrible (even with 50x50 compared to Recoil's much better performance), but it didn't crash.

So I wondered if it has something to do with too many atoms.

[acutmore]

Hi @Ruthenz

Yes I think you're right. The crash is being caused by Recoil. I downloaded the sandbox so it would be easier to inspect in devtools and am seeing an out-of-memory exception happening from Recoil's subscribeComponentToNode.

[jwdinker]

@Ruthenz I can't mess with it because my browser is freezing, but from a glance have you tried memozing your onMouseEnter callback instead of an anon fn? You are potentially creating and destroying tens of thousands of functions with every pixel change.

[JimLiu]

Hi @Ruthenz

Yes I think you're right. The crash is being caused by Recoil. I downloaded the sandbox so it would be easier to inspect in devtools and am seeing an out-of-memory exception happening from Recoil's subscribeComponentToNode.

Yes, you are right👍, it's caused by subscribeComponentToNode, one of the reasons is mapByUpdatingInMap

function mapByUpdatingInMap(map, k, updater) {
  const next = new Map(map);
  next.set(k, updater(next.get(k)));
  return next;
}

if you have 100x100x2 components which call useRecoilState, that means it creates new Map for 20,000 times, which causes the performance issue.

I think no need to create new Map in each subscribeComponentToNode.

function mapByUpdatingInMap(map, k, updater) {
  const next = map; // Do not create new Map(map);
  next.set(k, updater(next.get(k)));
  return next;
}

only need to create a new Map inside Batcher component:

function Batcher(props) {
  ...
  useEffect(() => {
  ...
    Queue.enqueueExecution('Batcher', () => {
    ...
      // create a new Map before replace the state.
      nextTree.nodeToComponentSubscriptions = new Map(nextTree.nodeToComponentSubscriptions);
      storeState.currentTree = nextTree;
    ...
}

cc @mondaychen

[stephanoparaskeva]

Not sure if related but if you inspect a React Native app using Recoil you'll see this in safari after taking a snapshot. There are hundreds of such objects. I am only using useRecoilState with 2 items.

export const usersAtom = atom({ key: 'usersAtom', default: [] });

export const userAtom = atom({
  key: 'userAtom',
  default: { experiences: [], roles: [] },
} as any);

[Shmew]

I haven't run into the error you're getting, but I was able to get a component that originally started with a thousand atoms being initialized, to render almost instantly
(compared to ~200ms before) by making the initialization lazy.

I did this by making a rudimentary excel clone.

[stephanoparaskeva]

I haven't run into the error you're getting, but I was able to get a component that originally started with a thousand atoms being initialized, to render almost instantly
(compared to ~200ms before) by making the initialization lazy.

I did this by making a rudimentary excel clone.

I wish your code was JS or TS, it looks like you have made many many useful things

[Shmew]

I can compile it to normal es format, but the code is a bit hard to read due to name mangling because of the way namespaces and modules work in .Net. Supposedly we just landed typescript transpilation support, which if that works properly it should be quite readable, I'll look into it. If that works as expected, it may be a lot easier to publish actual npm packages for others to use.

[davidmccabe]

There are several places where Recoil does unnecessary copying of data structures. This is something I'm working on actively as we have some projects that depend on being able to scale up to more atoms. Sorry about this.

[davidmccabe]

Some of the copies are just unnecessary and can be removed. For others we are going to move to some kind of persistent data structures, because not all of the copies can be eliminated in Concurrent.

[fc1943s]

Hi @Ruthenz
Yes I think you're right. The crash is being caused by Recoil. I downloaded the sandbox so it would be easier to inspect in devtools and am seeing an out-of-memory exception happening from Recoil's subscribeComponentToNode.

Yes, you are right👍, it's caused by subscribeComponentToNode, one of the reasons is mapByUpdatingInMap

function mapByUpdatingInMap(map, k, updater) {
  const next = new Map(map);
  next.set(k, updater(next.get(k)));
  return next;
}

if you have 100x100x2 components which call useRecoilState, that means it creates new Map for 20,000 times, which causes the performance issue.

I think no need to create new Map in each subscribeComponentToNode.

function mapByUpdatingInMap(map, k, updater) {
  const next = map; // Do not create new Map(map);
  next.set(k, updater(next.get(k)));
  return next;
}

only need to create a new Map inside Batcher component:

function Batcher(props) {
  ...
  useEffect(() => {
  ...
    Queue.enqueueExecution('Batcher', () => {
    ...
      // create a new Map before replace the state.
      nextTree.nodeToComponentSubscriptions = new Map(nextTree.nodeToComponentSubscriptions);
      storeState.currentTree = nextTree;
    ...
}

cc @mondaychen

@JimLiu WTF, this reduced my memory consumption a lot and its super fast now, from unbearable slow.
Thanks for the fix! I will keep this change local until a tested release comes out.

[drarmstr]

Can you please confirm if your performance concern remains with the 0.1.1 release?