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

Optimizing getter access #763

Open
qraynaud opened this issue Sep 8, 2019 · 1 comment
Open

Optimizing getter access #763

qraynaud opened this issue Sep 8, 2019 · 1 comment

Comments

@qraynaud
Copy link

qraynaud commented Sep 8, 2019

The problem I was facing

I've been using aurelia for a project that does a lot of calculations using big.js on a single page. Those tend to be pretty slow. The page revolves around an object that has a lot of getters relying on each other to compute all the intermediary numbers (that are all printed on the page along the final results).

The issue I got into pretty quickly with that approach are:

  • first, when a value that is depended upon by a chain of getters, all of them are triggered, causing the basic getters to be called sometimes hundreds of time when they will always give the same result
  • some calculations were so expensive I had to move them into a Web Worker, thus I had some calculations that became intrinsically asynchronous, and getters returning promises don't play well with aurelia (at least now)

How I solved it

I came around one major idea, using @computedFrom is basically saying to aurelia "this getter is deterministic and given the value of the dependencies I'm listing, I will always produce the same result". Sure, this might not always be true, but at least I could assume that it is a good practice that should be encouraged.

And I came up with a bit of code that is pretty unobtrusive that I wanted to share to get your thoughts on it. But before giving the code base, I'd like to show the end result:

A basic example could look like:

@cache
class SomeDataClass {
  // ...

  @computedFrom('attr1')
  get attr2() {
    return this.attr1 + 42;
  }

  @computedFrom('attr2')
  @cache.async
  get attr3() {
    // this should be a real async task, but this will do for the exemple
    return Promise.resolve(this.attr2);
  }
}

export class MyView {
  static inject = [BindingEngine]
  constructor(bindingEngine) {
    this.bindingEngine = bindingEngine;
  }
  attached() {
    this.someData = new SomeDataClass();
    this.disposer = this.someData.observe(this.bindingEngine);
  }
  detached() {
    // we can also omit this if we want to keep the cache going on for
    // the whole app lifecycle or call it somewhere else to fine tune the lifecycle
    this.disposer.dispose();
  }
}

export class IsResolvingValueConverter {
  toView(value) {
    return value instanceof Promise;
  }
}

And the view could look like:

<template>
  <p><input type="text" value.bind="attr1 & toNumber"/>
  <p>${someData.attr2}</p>
  <template if.bind="someData.attr3 | isResolving">
    <p>Loading attr3...</p>
  </template>
  <template else>
   <p>${someData.attr3}</p>
  </template>
</template>

What the decorator does is encapsulating the getters that make use of computedFrom (I detect them using their dependencies property) to have it check if the value was already calculated, if not call the original getter and add its return value to cache, otherwise return the value from cache. I had to populate the cache into a $cache object that I put into a sub property of the object because, like that, I can add $cache.<attrName> to the getter list of dependencies to ensure that changing the value in the cache causes a new view rendering on the aurelia side.

When calling the observe method, I put in place my own property observers that bust the cache of all dependents properties when a value is changed, thus causing aurelia to access each related getters again, causing them to make the calculations once, and returning after their cache values.

When using the cache.async decorator, I put the promise in the cache, as it would have done without, but I add a very simple code to replace the promise in the cache by its resolved value if the cache still contains the original promise (thus preventing race conditions).

I also added a cache.disable decorator to disable the caching mechanism, in case of a non-deterministic getter.

You will find the (pretty suboptimized and still dirty but already so much more efficient than the default behavior in my use case) code base for those decorators in this file so it does not pollute the issue.

Why this is not an optimal solution

There are numerous reasons this is not optimal:

  • first, since I can't get my hands on an instance of BindingEngine easily, I have to require users to provide it to the object using the observe method. It also makes hierarchy of cached objects hard to initialize. I came up with a bit of code to tell the cache decorator that those exist so it automatically initializes them, but still it's far from perfect
  • I had to add a "dirty" $cache property on all observed instances of the class, which I find distasteful, if only because this cache already duplicates the value yet another time since it's already in the various currentValue properties of the various getter's observers. This is necessary so that aurelia picks up the change and access the getter to reflect it in the view. At least I did not find a better way to do it yet.
  • this approach makes it very hard to know when to clean up and what to clean up

What I believe would be awesome

I think that it would make sense for computedFrom to default to encapsulate the getters to make them return the currentValue in the observer except if the observer is marked as dirty.

I think, I might be wrong, it would be easy in the framework code base to mark each dependents observers as dirty when a value change and to notify the view it should refresh. By inverting the chain of responsibility this way, we can minimize code execution and make things quicker at the price of minor systematic checks.

It would also require to have some decorator to deactivate this behavior if needed (for non-deterministic getters for example).

The drawbacks

Since this might be a breaking change (for non-deterministic getters at least), it also needs to be carefully weighted. Maybe an opt-in behavior first and change it to default after a major update? I've no ready answer for that but if you deem the idea worth investigating, I'm sure your experience with aurelia up to now will provide enough insights as to how to handle this.

@qraynaud
Copy link
Author

qraynaud commented Sep 8, 2019

Afterthought

I want to add that the fact I was able to implement such a complex bit of code in merely 250 lines of code (including closing brackets ^^) is amazing. You really did a wonderful job and I just can't wait for vNext! Keep up with us, I Iove you all guys!

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

No branches or pull requests

1 participant