using selectorFamily in a loop is slow

[novykh]

Hello I'm experiencing a weird performance issue regarding atomFamily and selectorFamily when used with big collections.
Here is my code:

const resourcesByIdAtom = atomFamily({ key: "resourcesByIdAtom", default: initialState })

const resourceState = selectorFamily({
  key: "resourceState",
  get: (id) => ({ get }) => get(resourcesByIdAtom(id))
})

const resourcesState = selectorFamily({
  key: "resourcesState",
  get: ids => ({ get }) => {
    return ids.map(id => get(resourceState(id)))
  },
})

const useResourcesValue = ids => useRecoilValue(resourcesState(ids))

The idea is that we fetch and keep a lot of resources inside this atom resourcesByIdAtom, and we are using atomFamily in order to be able to access them also directly, based on their id.
But, there is a case we need to pass a lot of ids directly and read the data for all of them.

So using useResourcesValue with a big array of ids, it takes a lot of time to calculate, cpu is on 100% and the main thread is completely bloated.

Is there any suggestions? Am I doing something completely wrong?

I'd expect selectors, especially for something that haven't change to be super fast.

[drarmstr]

The resourceState selector family in your example doesn't seem to be doing anything. You could save overhead by simply avoiding that and using the resourcesByIdAtom atom family directly and in resourcesState.

You may also want to try skipping the resourcesState level and just mapping in your hook directly. That may be a quick operation and cheaper than going through the selector. The issue may also be related to memory usage if you use a lot of unique combination of ids. The selectors provide caching, but currently don't evict. Garbage collection is being worked on, but not yet in the release.

[novykh]

@drarmstr thank you for your answer. Please, keep in mind that this is a minimal working example - thats why you feel that the selector does nothing.
The same problem occurs even when skipping one of the above selectors. Is there a branch with the work done on gc? I could try it out and maybe even help.

The way to fix that on our side, was to keep all the byId state inside one atom (not a family) with custom memoization. But, this is weird since it should have been handled by the recoil in the first place.

[42shadow42]

@novykh I was able to replicate your concern with the snippets you sent, and have a theory on the root cause which should help in instrumenting a fix.

For my analysis, I attempted some variations of accessing all items from your resourcesByIdAtom.

The first was using your resourcesState selector family:

const loadResourcesHelper = useRecoilCallback(({snapshot}) => () => {
  return snapshot.getLoadable(resourcesState(snapshot.getLoadable(resourceIds).contents)).contents;
})

I admit I was not impressed by the results, as accessing 1000 atoms took almost a second. Yet alone the 5000 I attempted the benchmark with.

Second I used the resourceState selector family:

const loadResourcesSelector = useRecoilCallback(({snapshot}) => () => {
  return snapshot.getLoadable(resourceIds).contents.map((id) => snapshot.getLoadable(resourceState(id)).contents);
})

This implementation ran much faster easily passing a reasonable benchmark of 5000 atoms in mere milliseconds.

Third I used the resourcesByIdAtom directly:

const loadResourcesDirectly = useRecoilCallback(({snapshot}) => () => {
  return snapshot.getLoadable(resourceIds).contents.map((id) => snapshot.getLoadable(resourcesByIdAtom(id)).contents);
})

This implementation was indistinguishable from the second implementation.

The results suggest that you are correct there is an issue with the implementation of the resources state selector family. The examples I show suggest that by extracting the map outside of the selector you should be able to resolve the performance issues.

[42shadow42]

Could it be related to dependency tracking? I don't think the cache is causing the problem because the stableStringify is quick and I used very small key size (auto incrementing ids) and very small data size of a boolean. In the example I tested we would be tracking 5000 dependencies for a single iteration, while the quick alternative I proposed doesn't track dependencies (and by extension won't update automatically if one of the base atoms change), not sure how I feel about that drawback.

[tonyabracadabra]

The issue I had here might be related #900, I am wondering if there is any known reason behind the extremely poor performance selector + for loop.

[tonyabracadabra]

Is there any update on this thread? Do we know what might be the reason behind the lack of performance on the for loop?

[drarmstr]

There were some additional performance optimizations released with 0.2, I wonder if those may have helped this issue?

[novykh]

There were some additional performance optimizations released with 0.2, I wonder if those may have helped this issue?

Unfortunately it didn't.
@42shadow42 is right, there are some work arounds for now, like his/her suggestion above. What we did for now is move everything out of atomFamily and back to simple atoms with nested objects which works fast enough.

const resourcesByIdAtom = atom({ key: "resourcesByIdAtom", default: {} })

const resourcesState = selectorFamily({
  key: "resourcesState",
  get: ids => ({ get }) => {
    const byId = get(resourcesByIdAtom)
    return ids.map(id => byId[id])
  },
})

const useResourcesValue = ids => {
    const byId = useRecoilValue(resourcesState(ids)
}

Which makes the atomFamily and selectorFamily feel useless (and I don't really like it since I like the family logic). I will try to figure out the root problem of this.

[tonyabracadabra]

There were some additional performance optimizations released with 0.2, I wonder if those may have helped this issue?

Unfortunately it didn't.
@42shadow42 is right, there are some work arounds for now, like his/her suggestion above. What we did for now is move everything out of atomFamily and back to simple atoms with nested objects which works fast enough.

const resourcesByIdAtom = atom({ key: "resourcesByIdAtom", default: {} })

const resourcesState = selectorFamily({
  key: "resourcesState",
  get: ids => ({ get }) => {
    const byId = get(resourcesByIdAtom)
    return ids.map(id => byId[id])
  },
})

const useResourcesValue = ids => {
    const byId = useRecoilValue(resourcesState(ids)
}

Which makes the atomFamily and selectorFamily feel useless (and I don't really like it since I like the family logic). I will try to figure out the root problem of this.

why do you still need selectorFamily in this case?

[novykh]

Of course there is no reason to do in such case - hence the quote in your question has Which makes the atomFamily and selectorFamily feel useless.

[42shadow42]

I'm probably not going to code it up, but a selector might be used to aggregate the total cost of items in an order. In that case you would have an atom of order, an atom family of products, and a selector for the totals. If the number of products ordered gets high that's where the system breaks down. Not sure if this is a really realistic example though.

…

On Thu, May 6, 2021 at 4:57 PM Johnny Klironomos ***@***.***> wrote: No reason, since this is just a simplified example demonstrating the problem. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#914 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABUHIH7M5APKNOYGYFD3MODTMMNEZANCNFSM4YRIK2UA> .

-- Thanks, Nate!

[tonyabracadabra]

After some digging I found that the mergeDependencyMapIntoGraph function will be called every time a get operation in the selector is called, and the size of deps , which will be iterated, grows as the number of get function calls, hence as we call more get in the selector, the function becomes exponentially slower. Any thoughts on this? I assume this is here for a reason and is there any ways we can improve that? @drarmstr

[drarmstr]

Interesting insight! (cc @davidmccabe)

[tonyabracadabra]

Interesting insight! (cc @davidmccabe)

Do we have any ideas on that?

[tonyabracadabra]

Is there any updates on this one?

[thomaszdxsn]

I found even selector is memorized, the selectorFamily access is 10x slower than normal dict attribute access

[thomaszdxsn]

After some digging I found that the mergeDependencyMapIntoGraph function will be called every time a get operation in the selector is called, and the size of deps , which will be iterated, grows as the number of get function calls, hence as we call more get in the selector, the function becomes exponentially slower. Any thoughts on this? I assume this is here for a reason and is there any ways we can improve that? @drarmstr

exactly as you say

[thomaszdxsn]

Interesting insight! (cc @davidmccabe)

maybe we can opt not merge every times, and give a manual flush function merge the depsMap

[thomaszdxsn]

Or wrap the selector.options.get, after inner function finished then register all deps