Suggestion to optimize callback function allocation when using `useSignalEffect`

[adamuso]

Hello,

I have a little suggestion for you to change useSignalEffect a bit. From my understanding useSignalEffect will usually be used like in code below:

function MyComponent() {
    const localSignal = useSignal(0);
    // I think this will be the most common usage - creating a callback inside `useSignalEffect` 
    useSignalEffect(() => {
        console.log(localSignal.value);
    });

   return <div>value: {localSignal}</div>;
}

This code has one drawback: it will allocate () => { console.log(someSignal.value) } every time MyComponent rerenders even when the callback is always the same (or maybe I am incorrect and it needs to be recreated every time, then please make me aware of this). So it would be nice if we could do something like this:

function MyComponent() {
    const localSignal = useSignal(0);
    useSignalEffect()?.with(() => {
        console.log(localSignal.value);
    });

   return <div>value: {localSignal}</div>;
}

Which with a little change to useSignalEffect can be implemented like that:

export function useSignalEffect(cb?: () => void | (() => void)) {
    const callback = useRef(cb);

    if (cb) {
        callback.current = cb;
    }

    useEffect(() => {
        return effect(() => callback.current?.())
    }, []);

    return !callback.current ? { with: (cb: () => void | (() => void)) => callback.current = cb } : null;
};

The reasoning behind it is that in useSignalEffect()?.with(...) expression we are using ?. which will invoke a function only if non null or undefined value is returned. With this in mind we can return from useSignalEffect an object with with() function (with is just example name) which will set callback.current value OR if callback.current is already set then we return null. Essentialy this achieves that the callback in MyComponent will be only allocated in first render and never again, because code after ?. when returned values is null will never be executed by JavaScript.

The change in useSignalEffect is also backwards compatible. When using useSignalEffect(() => {...}) without ?.with(...) it will function exactly the same as it used to.

I hope this idea is not confusing you, but this really could improve the performance of components which have lots of useSignalEffect usages and also rerenders quite frequently. The same problem exists in reacts useCallback which can be used like this useCallback(() => { ... }, []) and it also will always allocate callback again and there is no way around it. But with useSignalEffect there is no need to specify dependencies which makes it an ideal candidate for such optimization.

Please let me know what you think about it.

[XantreDev]

I think there are good reasons behind of this idea, but it's overcomplicates API. I actually thought about this problem and decided that its enough to just write useSignalEffectOnce. Because it is reusing the clojure - it can be JIT-ed more easily. Closure that have created on each component render will be clean with minor GC, because has short live (i suppose), so there shouldn't be big problem

[leqwasd]

Wouldn't the given solution allocate the callback anyway + 1 in useEffect + 1 in effect( () => ?

[XantreDev]

My solution creates two callbacks, but they will live in minor heap and will be deallocated with the next Minor GC. Maybe it will be good to create our own collection of standard hooks to pass params with ?. and benchmark it against standard ones

[rschristian]

it will allocate () => { console.log(someSignal.value) } every time MyComponent rerenders even when the callback is always the same

The cost of this should be negligible in all real-world scenarios. You'd have to be rerendering multiple times per second, continually, likely with a more complex closure in order to run into trouble. At which point our advice would be, in all likelihood, to not do that.

Deviation from this API is a deviation from the (p)react "feel", if you will, so I don't think it'd be received very well.

I hope this idea is not confusing you, but this really could improve the performance of components which have lots of useSignalEffect usages and also rerenders quite frequently.

Have an example with a benchmark to back this up? I don't think you're going to run into a real scenario where the cost of creating a closure causes issues, and I don't think we're really that keen on benchmark-driven development.

[andrewiggins]

To re-iterate what @rschristian said, moving away from inline closures would deviate from the common way to use (p)react hooks, so without a very compelling reason/data, I don't think we'll change our API for now.

As you've demostrated though, it's not hard to implement this in user land though, so feel free to continue with that, and feel free to continue sharing what you find with that exploration with us here :) Thanks for sharing!