Some idea about useSnapshot performance

[pilaoda]

After days migration trying from my own proxy to valtio, I found some performance degradation due to current subscribe logic in useSnapshot.

Current logic in useSnapshot subscribe to a big proxy, and use isChanged to check if it should update.
If proxyObject is a big Map for N Items, and all items has a seperate component that call useSnapshot(proxyObject)[uid], it would be O(N) time complexity for change comparison if any one of the item change its own property.

My suggestion (also my old proxy implementation) is register a new listerner to the proxyObject when a snapshot property getter invoked, and broadcast all listeners when the specify property setter inproxyObject being called, then we will get O(1) time complexity on nested object property setting.

[pilaoda]

That is to say, we give up the dependency on proxy-compare, and add a const propListenersMap = {} as Record<string, Set<Listener>()> with relate logic in current proxy function scope.

[dai-shi]

If I understand it correctly, this has been suggested before. It's just a different design decision. There's a trade off and we chose what it is now.

[pilaoda]

Well, I'm curious what trade offs do you make?

[dai-shi]

We would like to optimize renders for state.data = await fetch(...).then(res => res.json()); usage.

I'm not exactly sure if your idea works for it, so feel free to fork and try.

[dai-shi]

Unfortunately, many tests are missing because some of architectural decisions were very obvious to me personally. We should add more basic tests.

#1162 is the first one.

[pilaoda]

PR here: #1161

[pilaoda]

It's totally Ok for me if PR not merged, I'm just exploring the best way for my project to work.
It would be hard for me to avoid change valito but create a extension repository since some variables in function scope can't not access, it would be total rewrite if I use the unstable replace api.
So I would like to continue work on my fork which is easier for me. We can still discuss about the valtio PR merge posibility.

[dai-shi]

Yeah, forking is fine if it's your personal project.

But, I see some benefit for the ecosystem, and I would like to see an extension library if possible. I'm not sure.

it would be total rewrite if I use the unstable replace api.

Yeah, I can understand that.

since some variables in function scope can't not access

There are still something I don't yet understand. I would need to look into your code more closely, especially around subscribeKey.

Can I get your confirmation that your idea theoretically conflicts with #1162? That's what I expected.

[pilaoda]

I'm still not sure my idea can pass this test, I will try my best.

[dai-shi]

If it passes, it means my understanding was totally wrong, and I need to read it with some fresh eyes to understand it. Probably I need to create another missing test to fail. 🤣

[pilaoda]

Ok, it passed. Just add back isChange in getSnapshot. It doesn't conflict with O(1).

[pilaoda]

Now I found #1162 conflict with O(1) property, because if the proxy object reference changed, we need re-render to re-subscribe properties inside.

This is the test this PR can't pass, no matter use isChange check or not.

it('re-subscribe after nested object re-assigned', async () => {
    const state = proxy({ nested: { a: 0 } })

    const renderFn = vi.fn()
    const Component = () => {
      const snap = useSnapshot(state)
      renderFn()
      return (
        <>
          <div>Nested a: {snap.nested.a}</div>
          <button
            onClick={() => {
              state.nested = { a: 0, b: 0 } as any
            }}
          >
            re-assign nested
          </button>
          <button
            onClick={() => {
              state.nested.a++
            }}
          >
            increment a
          </button>
        </>
      )
    }

    render(<Component />)

    expect(screen.getByText('Nested a: 0')).toBeInTheDocument()
    expect(renderFn).toBeCalledTimes(1)

    fireEvent.click(screen.getByText('re-assign nested'))
    await act(() => vi.advanceTimersByTimeAsync(0))

    expect(renderFn).toBeCalledTimes(1)

    fireEvent.click(screen.getByText('increment a'))
    await act(() => vi.advanceTimersByTimeAsync(0))

	expect(screen.getByText('Nested a: 1')).toBeInTheDocument()
    expect(renderFn).toBeCalledTimes(2)
  })

This is another trade off need to make.
I will choose O(1) on my fork, since it is what I need.
O(N) cause much more delay problems in my project than rerender caused by object reference change.

I also feel the use case in #1162 can be avoid by user if use state.nested.count = 0 which is more common to use.

[dai-shi]

That's good to know. So, it's what I originally assumed.

If we can force use state.nested.count = 0, we could use useSnapshot(state.nested). Feels suboptimal though.

What I'm still curious is what capability could add in vanilla.ts like replace internal function to allow you to create a third-party library. We don't add the 1161 change directly, but it would be nice to open possibilities so that the third-party library can extend it for the 1161 change.

[pilaoda]

I don't know man, the replace api is global and one way, and what I need to is not just extend.
I need to break the Liskov Substitution Principle especially in createSnapshot. I don't read getter and cache anymore, but defineProperty only.

That means, if a user require my third-party (valtio-observer I assumed), user can't expect valtio to work like before. it's replaced forever.

[dai-shi]

Yeah, I mean replace doesn't work well in this case.