preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.78k stars 93 forks source link

"No-op" batch triggers effects #242

Open felixschorer opened 2 years ago

felixschorer commented 2 years ago

Given a signal foo and an effect logging its value. When I run a batch, which changes the value of foo in such a way that it is essentially a no-op, then the value of foo is logged even though the value of foo is the same as before the batch.

const foo = signal(42);

effect(() => console.log(foo.value));

batch(() => {
  foo.value = 0;
  foo.value = 42;
});

This is also the case when the effect depends on a computed signal, but only if the computed value is read in between.

const foo = signal(42);
const bar = computed(() => foo.value);

effect(() => console.log(bar.value));

batch(() => {
  foo.value = 0;
  bar.value;
  foo.value = 42;
});
eddyw commented 2 years ago

As I understand by working with the internals the last weeks, batch doesn't batch updates but rather batches effects, it works sort of like this:

batch(() => {
  foo.value = 0;
  // ^ value changed, it increments the signal version number +1
  // ^ foo notifies all targets
  // ^     if target is a computed with subscribers (e.g: an effect),
  // ^          forward notification to subscribers (other effects/computed) (repeat)
  // ^     else if target is an effect
  // ^          effect is batched (added as a head of a LinkedList of effects to run)
  foo.value = 42;
  // ^ repeat same as above, version number increments +1 again
})
// ^ end of batch:
// Walk through each batched effect,
//   Check if needs to re-run by comparing the version number of its sources (signals/computed)
//   While doing this, if source is a 'computed',
//       Computed checks if need to re-run by comparing version number of its sources
//       Re-run computed and increase its version number +1 ... or not
//   Re-run effect .. or not

Basically, signals use a 'version' number rather than the actual value to know when a 'computed' or 'effect' needs to re-run. This is done because comparing numbers is cheaper. The "downside" is that if you set the same value as it was before the batch, there is no way to know if the value is the same, again because it compares version numbers which increase of every set and not the actual value.

There is this comment in the code regarding the version:

    // The version number of the source that target has last seen. We use version numbers
    // instead of storing the source value, because source values can take arbitrary amount
    // of memory, and computeds could hang on to them forever because they're lazily evaluated.
    // Use the special value -1 to mark potentially unused but recyclable nodes.
felixschorer commented 2 years ago

I think you're confusing nodes with signals. Nodes only have a version. Signals and computed signals have a version and keep a reference to the current / cached value.

Signals and computed signals could also keep a reference to the value (and version) which was valid before the batch. Then, in the commit phase of the batch, the signal could compare its current value with its previous value and reset its version to the previous version if they match. If they don't match, it can discard the previous value.

developit commented 2 years ago

As far as I am aware, there is no way to implement collapsing of batched value assignments/computations without retaining previous values, degrading performance and causing GC. It's also unnecessary for a large number of the intended use-cases for Signals.

I like to use the Preact adapter as a gauge for whether functionality is specialized to Signals usage, since the adapters are the largest and primary user of Signals. In the adapter, the effect that binds a signal to a prop has implicit access to its previous value by virtue of the fact that has to assign values to the props object for synchronization, and can simply read the value back for comparison.

felixschorer commented 2 years ago

In the adapter, the effect that binds a signal to a prop has implicit access to its previous value by virtue of the fact that has to assign values to the props object for synchronization, and can simply read the value back for comparison.

That's not always possible, e.g., when a computed signal returns a new reference every time it is executed.

const foo = signal(42);
const bar = computed(() => [foo.value]);

let previous = undefined;
effect(() => {
  // always true
  if (previous !==  bar.value) {
    console.log(bar.value);
  }
  previous = bar.value;
});

As far as I am aware, there is no way to implement collapsing of batched value assignments/computations without retaining previous values, degrading performance and causing GC.

You would only have to keep a single previous value for each signal while the batch hasn't committed yet.

class Computed<T> {
  version: number
  committedVersion: number

  value: T
  committedValue: T

  notify() {
    this.version = this.committedVersion
    this.value = this.committedValue
  }

  refresh() {
    const this.value= /* ... */
    if (this.value === this.committedValue) {
      this.version = this.committedVersion
    }
  }

  commit() {
    this.committedVersion = this.version
    this.committedValue = this.value
  }
}
eddyw commented 2 years ago

I've tried to implement it in my local fork some time ago but failed 😅 There isn't exactly a "commit" phase:

batch(() => {
  foo.value = 0;  // notifies all targets (effects are scheduled to run)
  foo.value = 42; // notifies all targets (what should it do here?)
});

Batch batches effects, so you'd need to un-schedule those effects. This is tricky because an effect can be scheduled to run by other signals too so you can't just remove them from the linked list of batched effects.

If you keep the prev. value of foo (e.g: I used foo.draft) and let the effect check for changes in sources, that'd work .. but then you need to clean up those previous/draft values at some point. A way to achieve this is creating another linked list of Signals (like batchedEffect) so you can iterate over them at the end of the batch and clean up those values. Otherwise signals will hold a reference in memory to values that should be GC-ed. Doing this does come with a perf. penalty depending on the number of signals that changed in the batch.

This could kinda work but it also doesn't take into account that computed and effect can have side-effects:

const foo = signal(42);

effect(() => {
    foo.value;
    foo.value = 42;
});
effect(() => console.log("Print or no print", foo.value));

batch(() => {
    foo.value = 123;
});
felixschorer commented 2 years ago

A way to achieve this is creating another linked list of Signals (like batchedEffect) so you can iterate over them at the end of the batch and clean up those values. Otherwise signals will hold a reference in memory to values that should be GC-ed. Doing this does come with a perf. penalty depending on the number of signals that changed in the batch.

Yes, that's what I meant. I am sorry that I was being unclear.

I think that this linked list is going to very short in most cases. You only have to schedule Signals, which have been mutated, and Computed signals, whose value property was accessed during the batch, for commit.

MicahZoltu commented 1 year ago

Batch batches effects, so you'd need to un-schedule those effects. This is tricky because an effect can be scheduled to run by other signals too so you can't just remove them from the linked list of batched effects.

Is it possible to queue up a list of the signals whose values have changed rather than queueing up a list of callbacks to run? This would require tracking the signals with updated values and the original values of those signals. Then at the end of the batch you would iterate over the collected signals and compare their current value against the previous value and only trigger their associated callbacks if the value changed during the batch.

The argument above about GC and memory pressure are legitimate, so that would still need to be weighed against the value added by this feature.