Potential bug discovered during benchmark stress tests

[transitive-bullshit]

Bug Description

I created a comparison of the most popular TS reactivity libs, including Valtio.

During my deep-dive, I wanted to benchmark all of them and created a fork of @milomg's excellent js-reactivity-benchmark, which includes dozens of benchmarks and stress tests.

The problem is that Valtio fails to complete some of the dynamic benchmark tests in the suite.

I've been unable to track down the source of the issue, whether it may be due to a rare bug in Valtio or possibly due to a bug in the Valtio adapter used by the benchmark. Note that this was a bit of an awkward adapter to write given that Valtio doesn't automatically track dependencies like many other reactive libs (#149).

How to reproduce the issue:

1. Checkout the minimal repro branch of https://github.com/transitive-bullshit/js-reactivity-benchmark and switch to the feature/minimal-issues-repro-valtio branch
2. Run pnpm install
3. Run pnpm test to make sure everything's working locally
4. Run pnpm bench to reproduce the issue

The valtio benchmark tests work fine for the first test case but then hangs on the second (slightly larger) test case. Note that these test cases are creating a rectangular grid of shallow signals, where each layer of the grid contains computed properties depending on some number of the previous row's signals as a stress test.

CPU remains at 100% for the hung test with RAM usage flat around 75MB which seems reasonable.

I've paired down this branch as much as possible to try and make it easier to debug.

Note that in the full benchmark branch, all of the other reactivity libs are able to pass these benchmark tests, except for MobX which runs into a similar issue (mobxjs/mobx#3926).

Versions

• latest [email protected]
• node v22.4.1
• macos sonoma 14.5

Reproduction Link

https://github.com/transitive-bullshit/js-reactivity-benchmark/tree/feature/minimal-issues-repro-valtio

[dai-shi]

Tried it, but hard to tell.
The adapter seems tricky, especially with computed. I'm not sure how watchGet hack works either.
I think it may require more work to add a reactive layer on top of valtio.
Can you investigate how 100% CPU is caused by? sync: true is causing an infinite loop?

[dai-shi]

Does this make fast enough, or is it still slow?

diff --git a/src/frameworks/valtio.ts b/src/frameworks/valtio.ts
index 94d355f..1eed8ef 100644
--- a/src/frameworks/valtio.ts
+++ b/src/frameworks/valtio.ts
@@ -14,7 +14,7 @@ export const valtioFramework: ReactiveFramework = {
     return {
       write: (v) => (s.value = v),
       read: () => {
-        return s.value;
+        return initialValue; // s.value;
         // const get = watchGet.at(-1);
         // if (get) {
         //   return get(s).value;

[dai-shi]

Can you also try this please?

diff --git a/src/frameworks/valtio.ts b/src/frameworks/valtio.ts
index 94d355f..a9f40d7 100644
--- a/src/frameworks/valtio.ts
+++ b/src/frameworks/valtio.ts
@@ -10,7 +10,8 @@ import { watch } from "valtio/utils";
 export const valtioFramework: ReactiveFramework = {
   name: "Valtio",
   signal: (initialValue) => {
-    const s = proxy({ value: initialValue });
+    // const s = proxy({ value: initialValue });
+    const s = new Proxy({ value: initialValue }, {});
     return {
       write: (v) => (s.value = v),
       read: () => {
@@ -25,14 +26,14 @@ export const valtioFramework: ReactiveFramework = {
     };
   },
   computed: (fn) => {
-    const c = proxy({
-      get value() {
-        return fn();
-      },
-    });
+    // const c = proxy({
+    //   get value() {
+    //     return fn();
+    //   },
+    // });
     return {
       read: () => {
-        return c.value;
+        return fn();
         // const get = watchGet.at(-1);
         // if (get) {
         //   return get(c).value;

[transitive-bullshit]

Making both of these changes still results in the same slow functionality, meaning that Valtio is now not used at all by the adapter: https://github.com/transitive-bullshit/js-reactivity-benchmark/blob/feature/minimal-issues-repro-valtio/src/frameworks/valtio.ts

I experienced a similar issue with mobx (here), and we tracked it down to the computed properties not being cached. The more layers you add to the network, the depth of evaluating computed properties grows something like O(n^2) which greatly slows things down if n is >= 10 or so.

So if that interpretation is correct, one aspect of this stress test is really just getting at how efficient the computed caching implementation is. No caching of unchanged portions of the reactive graph will result in this behavior. Apparently mobx doesn't cache computed properties if they're not read within a batch or effect, so the fix for mobx was relatively straightforward, though I have my doubts as to whether that really should be the intended contract since it's a subtle DX footgun.

I'm guessing this is out-of-scope for Valtio, but perhaps we could find a way to update watch to work as expected for tracking this type of reactivity as #149 originally intended? Then, if there is reactive tracking via watch, another feature to consider would be supporting caching for computed proxy getters, which I believe would solve this benchmark issue. These are likely separate, dependent features and I'm guessing the latter would require changes to core which is likely to be out of scope?

Also, as a related question, I feel like Valtio's proxy and immutable snapshot would be perfect as a core to build a standalone reactive signals library on top of, but maybe I'm just pattern matching too much here, and I'm sure you want to keep the Valtio project core itself minimal and true to its goals. The only reason I think it's worth discussing is that "Valtio makes proxy-state simple for React and Vanilla" seems to align pretty well with this sort of direction. 👀 😄 I'd be curious if you have any ideas on how Valtio could be extended in this directly or if you've seen any other projects which build off of Valtio/ImmutableJS/Immer to add this type of Signals-like reactivity based off of deep, Proxy-based tracking?

And as always, thanks for your help & amazing work on the proxy OSS ecosystem! 🙏

[dai-shi]

As I understand there are three discussion points:

1. proper watch implementation
2. computed cache
3. possible core extension

I'll respond with different comments below so that they can be discussed separately.

[dai-shi]

1: #949 (comment)
2: #949 (comment)
3: #949 (comment)

[dai-shi]

proper watch implementation

I'm guessing this is out-of-scope for Valtio, but perhaps we could find a way to update watch to work as expected for tracking this type of reactivity as #149 originally intended?

I'm very happy to change watch as long as we don't need to touch the Valtio core.

To be honest, I don't think I fully understand "tracking this type of reactivity". watch is originally developed to cover #149, but how is it limited? It would be nice if you can help me understand the spec. Ah, is it just about avoiding get()?

[dai-shi]

Found this:

valtio/docs/api/utils/watch.mdx

Lines 79 to 85 in 2587214

	## No usage tracking
	`watch` currently does not implement the usage tracking of `useSnapshot`, so the callback will rerun anytime a watched proxy (or child proxy) has any fields mutated, regardless of the fields accessed within the `callback`'s code.
	And the return value of `get` is just the proxy itself, not a snapshot.
	This is because `watch` is a vanilla primitive built on top of `subscribe`. It is potentially possible for `watch`, or a new `watch`-like method, to implement usage tracking in the future, see [this discussion](https://github.com/pmndrs/valtio/discussions/640) if interested.

and #640.

[dai-shi]

computed cache

another feature to consider would be supporting caching for computed proxy getters, which I believe would solve this benchmark issue.

I'm actually interested in it. Computeds are one of most requested features from the start. We had derive implementation and usage with proxy-memoize. It's moved to https://github.com/valtiojs/derive-valtio. We are not actively working on this direction as it's pretty complicated, especially for glitch-free. But, I still like the idea of doing such things outside the core.

So, in Valtio, our recommended pattern for computeds is now using object getters. This it not actually a computed. But, it covers lots of Valtio use cases with React. It's because the caching works with snapshot.

So, would you be able to try something like this?

const p = proxy({
  count: 0,
  get double() {
    // some heavy computation here
    return this.count * 2;
  },
});

const s1 = snapshot(p);
const s2 = snapshot(p);
s1 === s2 // is true as snapshots are cached

Is it possible to use this behavior in your benchmark?

[dai-shi]

possible core extension

I'm sure you want to keep the Valtio project core itself minimal and true to its goals.

That is very true.

The only reason I think it's worth discussing is that "Valtio makes proxy-state simple for React and Vanilla" seems to align pretty well with this sort of direction. 👀 😄

Having that said, I'm actually open to suggestions.
It's because of this: https://x.com/dai_shi/status/1824599433662828682

I'd be curious if you have any ideas on how Valtio could be extended in this directly or if you've seen any other projects which build off of Valtio/ImmutableJS/Immer to add this type of Signals-like reactivity based off of deep, Proxy-based tracking?

Can you help me understand the goal here? Is it like tracking based on state.some.thing access?

#931 exposes some internals, and we can experiment various things outside the core.
(I also consider supporting "batch" if we go this direction.)