[polyfill] Unwatch is very slow

[dy]

Trying to make preact signals flavored adapter based on signal-polyfill, found one perf bottleneck.

I am using basic effect implementation (tbh watcher part looks confusing to me atm, mb I am just heavy):

let pending = false;

let watcher = new Watcher(() => {
  if (!pending) {
    pending = true;
    queueMicrotask(() => {
      pending = false;
      for (const signal of watcher.getPending()) {
        signal.get();
        watcher.watch(signal);
      }
    });
  }
});

export function effect(cb) {
  let destructor;
  let c = new Signal.Computed(() => { typeof destructor === 'function' && destructor(); destructor = cb(); });
  watcher.watch(c);
  c.get();
  return () => { typeof destructor === 'function' && destructor(); watcher.unwatch(c) };
}

After measuring performance in js-framework-benchmark, I found unwatch method to be very slow. Is there any perspective of optimization?

I attempted making simple batch for disposing effects, but there seems to be implicit bug causing index.js:407 Uncaught TypeError: Cannot read properties of undefined (reading 'wrapper') and subsequent TypeError: Cannot read properties of undefined (reading 'liveConsumerNode'). Test case is a bit tricky to reproduce.

It makes me wondering - if implementing effect on user side is so fragile and complicated, why not including it in proposal? I am sure there can be more optimal way than creating a Computed. Signals like uslignal, preact/signals or @webreflection/signal didn't have problem with having a class for effects.

[NullVoxPopuli]

I think you're running in to the same thing i did, the fix is this: https://github.com/NullVoxPopuli/signal-utils/blob/ef3d29f2dd2ff943714c5f7c1bfd0c89af529ad7/src/subtle/microtask-effect.ts#L24

for (const signal of watcher.getPending()) {
    signal.get();
}

// No args
watcher.watch();

Having args during flush causes slowness and an out of memory error. We should probably throw an error or something for this situation

[dy]

@NullVoxPopuli thanks, it works, but I wish I could understand what't going on here and why. Now it looks like some magic code to me, so it makes me wondering if that's only me or if not then why's that magic is left up to users to implement, not part of the standard.

Do you possibly know an efficient way to organize unwatch? I did this:

// destructor batches signals to unwatch
let toUnwatch = new Set
function unwatch(signal) {
  if (!toUnwatch.size) {
    queueMicrotask(flushUnwatch);
  }
  toUnwatch.add(signal)
}

const flushUnwatch = () => {
  let u = [...toUnwatch]
  toUnwatch.clear()
  watcher.unwatch(...u)
}

and it seems to produce abovementioned errors.

Feeling really tempted to use ulive for the meantime, until the signal proposal settles down.

[littledan]

Thanks for filing this issue. Note that we're considering changing the getPending/rewatch patterns as described in #77 ; we should keep performance in mind during that change (and I hope that it will also make it easier to get the usage right). Great idea to use other signal implementations; the one in this repo is unstable and not suitable for production, in addition to whatever performance issues exist.

[littledan]

Another problem is #157 ! Bug fix PRs are welcome for all of this.