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

[IMP] lazy reactive computed value #1499

Closed
wants to merge 3 commits into from
Closed

Conversation

caburj
Copy link
Contributor

@caburj caburj commented Jul 29, 2023

Problem: Prior to this commit, existing implementations of computed values
are eager. An example is the withComputedProperties in odoo's web addon, see:
1. It has an issue such that it can lead to triggering of the compute
function even if the target is still in invalid state -- e.g. removing an item
in an array. This can be solved by batching the recomputation with batched.
But once the compute is batched, the computed value can't be used by other
computed values, because it won't recompute until the next tick -- it's not part
of the same synchronous context.

Proposed solution: This commit introduces a implementation of computed value
that is pull-based -- it only recomputes when it's needed. The main logic is the
following:

  • A computed value is declared with a single-parameter function wrapped with
    computed.
  • The compute function has a cache that stores the result of the computation.
  • The cached value is invalidated when one of the dependencies of the compute
    function changes.
    • During invalidation, the cache is only marked as invalid and the
      recomputation is not made.
  • A dependent computation such as an effect may indirectly depend in the
    dependencies of a computed value2. This is an important detail because
    without this, the 'laziness' of the computed value won't trigger the dependent
    effect.
    • This indirect dependency tracking is achieved by using the introduced notion
      of multi-reactive target where a target can have multiple callbacks linked
      to it.
    • During recomputation of the computed value, both the target's reactive and
      the invalidation callback are subscribed to the dependencies of the computed
      value. But when the value is cached, only the target's reactive callback is
      subscribed.

With this implementation, we achieve a lazy computed value that plays well with
the existing reactive system.


Here's an illustration on how the computed values interact with the rendering.
Source

computed drawio


Footnotes

  1. https://github.com/odoo/odoo/blob/15d7cf12b98bf0223371b8cca91ef6f79c60ee31/addons/web/static/src/core/utils/reactive.js#L58

  2. It's possible that in the dependency graph, an effect is affected by
    invalidation. This effect will be triggered which will potentially ask for the
    value of the computed value which will trigger the recomputation.

@caburj caburj changed the title lazy reactive getter reactive computed Jul 30, 2023
@caburj caburj force-pushed the computed branch 2 times, most recently from 7b47a37 to bd0e59d Compare July 30, 2023 22:55
@caburj caburj changed the title reactive computed [IMP] lazy reactive computed value Jul 30, 2023
@caburj caburj marked this pull request as ready for review July 30, 2023 23:02
@caburj
Copy link
Contributor Author

caburj commented Jul 30, 2023

Hi @sdegueldre and @ged-odoo. What do you think about this?

@smetl
Copy link

smetl commented Jan 3, 2024

@sdegueldre Up :p This PR looks very interesting to manage big js project with complex data structure like the POS !

caburj and others added 2 commits January 3, 2024 18:23
**Problem:** Prior to this commit, existing implementations of computed values
are eager. An example is the `withComputedProperties` in odoo's web addon, see:
[^1]. It has an issue such that it can lead to triggering of the compute
function even if the target is still in invalid state -- e.g. removing an item
in an array. This can be solved by batching the recomputation with `batched`.
But once the compute is batched, the computed value can't be used by other
computed values, because it won't recompute until the next tick -- it's not part
of the same synchronous context.

**Proposed solution:** This commit introduces a implementation of computed value
that is pull-based -- it only recomputes when it's needed. The main logic is the
following:

- A computed value is declared with a single-parameter function wrapped with
  `computed`.
- The compute function has a cache that stores the result of the computation.
- The cached value is invalidated when one of the dependencies of the compute
  function changes.
  - During invalidation, the cache is only marked as invalid and the
    recomputation is not made.
- A dependent computation such as an effect may indirectly depend in the
  dependencies of a computed value[^2]. This is an important detail because
  without this, the 'laziness' of the computed value won't trigger the dependent
  effect.
  - This indirect dependency tracking is achieved by using the introduced notion
    of multi-reactive target where a target can have multiple callbacks linked
    to it.
  - During recomputation of the computed value, both the target's reactive and
    the invalidation callback are subscribed to the dependencies of the computed
    value. But when the value is cached, only the target's reactive callback is
    subscribed.

With this implementation, we achieve a lazy computed value that plays well with
the existing reactive system.

[^1]: https://github.com/odoo/odoo/blob/15d7cf12b98bf0223371b8cca91ef6f79c60ee31/addons/web/static/src/core/utils/reactive.js#L58
[^2]: It's possible that in the dependency graph, an effect is affected by
invalidation. This effect will be triggered which will potentially ask for the
value of the computed value which will trigger the recomputation.
I had a discussion with SEB and he told me about auto-sort compute. He said that
it would be better if we could implement automatic in-place sorting without
triggering the reactivity.

However, this PR can't actually handle that because this PR is just an extension
of the existing reactivity, it doesn't actually modify it. So if inplace sort
will result to multiple mutations, then that will be the number of times the
callback will be called. Even inside a `computed` that's introduced in this PR.

Nevertheless, compute that depends on sorted array is still useful, it's just
that the sorting should not be done in-place. This commit introduces an example
where a computed value that returns a sorted array is only called once (in a
batched context) even though multiple mutations are made to the array.
@sdegueldre
Copy link
Contributor

sdegueldre commented Jan 4, 2024

I'm not convinced a priori that we need this but if we do it feels like we could implement this on top of effect relatively simply?

function lazyComputed(obj, propName, compute) {
    const key = Symbol(propName);
    Object.defineProperty(obj, propName, {
        get() {
            return this[key]();
        },
        configurable: true,
    });
    effect(
        (obj) => {
            const value = [];
            obj[key] = () => {
                if (!value.length) {
                    value.push(compute(obj));
                }
                return value[0];
            };
        },
        [obj]
    );
}

There are still some shenanigans with a symbol because we don't trap defineProperty on reactive objects but otherwise this seems like a simpler implementation that doesn't require acess to owl internals or a new type of reactive object.

Edit: I've created a version of the todo list app on the playground that auto-sorts the list of tasks: playground link

Edit: slightly refined version:

function lazyComputed(obj, properties, deps = []) {
    for (const [propName, compute] of Object.entries(properties)) {
        const key = Symbol(propName);
        Object.defineProperty(obj, propName, {
            get() {
                return this[key]();
            },
            configurable: true,
        });

        effect(
            (obj, ...deps) => {
                const value = [];
                obj[key] = () => {
                    if (!value.length) {
                        value.push(Reflect.apply(compute, obj, deps));
                    }
                    return value[0];
                };
            },
            [obj, ...deps]
        );
    }
}

@caburj
Copy link
Contributor Author

caburj commented Jan 5, 2024

@sdegueldre I really tried to find a way without changing the internal but I couldn't. But man, your solution is awesome. I converted the tests to use your solution and it just works! Knowing this simple solution you provided, we can close this PR.

But before I close, you mentioned "There are still some shenanigans with a symbol because we don't trap defineProperty on reactive objects", when you find time, can you tell more about these shenanigans with the symbol?

@sdegueldre
Copy link
Contributor

As you can see in my implementation, I'm still forced to write and read on a symbol property on the reactive object because at the moment, the reactivity system doesn't consider defineProperty to be a write, and so there is no way to update the getter and have it notify. So we have to do this rigmarole where we write a normal function property on the symbol and call it from the getter.

If that wasn't the case the code could be shortened to just an effect that defines the getter.

@sdegueldre sdegueldre closed this Jan 8, 2024
caburj added a commit to odoo-dev/odoo that referenced this pull request Jul 26, 2024
**Problem:**

A customer experiences a noticeable delay (1 to 2 sec) when adding product in
the cart because of lack of caching computations. It gets worse the more
orderlines are added in the cart.

**Solution:**

This commit introduces an implementation of lazy reactive computed value that is
pull-based -- it only recomputes when it's needed. Check the following PR in
odoo/owl for its origin: odoo/owl#1499

This can be used for smart-caching selected getters. In this PR, we made a
getter for the `get_all_prices` method and instead of normal getter access, we
get the result of the getter using the `get` method introduced in the wrapper
class.

This means that in one call stack, calling `get('allPrices')` will only run once
and it will keep returning the cached value until a dependency in its
dependency (calculation) graph changed.

With this patch, adding an orderline is now 300ms -- approximate 5 times faster
than without caching.
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.

3 participants