Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: don't recompute when dependent signals come back to previous values used for last computation #197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

divdavem
Copy link

Hello,
I am a maintainer of the tansu signal library.
As I was exploring how to re-implement tansu with the signal-polyfill package (as I am hoping it will bring better interoperability with other signal libraries), I came across the following simple use case which does not match my expectations (and does not match either the behavior we implemented in tansu).
I am opening this PR to discuss it. It does not (yet) contain any fix, just the test cases. One fails and the other one succeeds:

Here is the failing use case:

let n = 0;
const s = new Signal.State(0);
const c = new Signal.Computed(() => (n++, s.get()));
c.get(); // this triggers a computation of c (so n passes from 0 to 1)
s.set(1); // this does not trigger anything because the computation of c is lazy
s.set(0); // let's come back to the previous value of s
c.get(); // if we recompute c, there is no need to call the function as the last time c was computed was with s = 0
expect(n).toBe(1); // so I am expecting n to still be 1
// but this test fails: there is an (unneeded) re-computation

Here is a small variation of the previous case, with an extra intermediate computed signal, which prevents the re-computation of c:

let n = 0;
const s = new Signal.State(0);
const extra = new Signal.Computed(() => s.get());
const c = new Signal.Computed(() => (n++, extra.get()));
c.get(); // this triggers a computation of c (so n passes from 0 to 1)
s.set(1); // this does not trigger anything because the computation of c is lazy
s.set(0); // let's come back to the previous value of s
c.get(); // if we recompute c, there is no need to call the function as the last time c was computed was with s = 0
expect(n).toBe(1); // so I am expecting n to still be 1
// this test succeeds

I believe we should not have to add intermediate computed signals to prevent unneeded re-computations.
In addition to checking the version number of dependent signals, I was expecting the algorithm to also compare the values with the previous values (using the provided equal function) before calling the re-computation function.

What do you think?

@littledan
Copy link
Member

This is an interesting comparison case. Currently, comparison is done "on the way out" rather than "on the way in", so there's no way to catch such a non-change. I don't really know how to evaluate the pros and cons of these approaches. Can you tell me more about where such a case has come up for you in practice, or what motivated you to implement tansu this way?

@shaylew
Copy link
Collaborator

shaylew commented May 13, 2024

I think the "use an extra computed signal" trick exposes basically exactly the performance tradeoff here: if you're willing to hold onto previous values values (at a memory cost) and run equality comparisons more often (at an execution time cost) you can potentially rerun computed bodies less often.

It might not be pretty/performant but I think you can actually build this behavior into a subclass of State/Computeds by attaching a WeakMap<Computed<any>, Computed<T>> to each Signal<T>, and then override get so it uses currentComputed() to look up the right intermediary computed for the current reader.

(Though this is one thing I think is cleaner with an "intercept tracked reads" primitive. You'd then be able to make a TansuComputed that created intermediaries for anything it reads, whether or not those signals were built with tansu, rather than making specific subclasses that create intermediaries when read.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants