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

Cached computed properties seem to be invalidated too often #10433

Closed
qbolec opened this issue Feb 12, 2015 · 3 comments
Closed

Cached computed properties seem to be invalidated too often #10433

qbolec opened this issue Feb 12, 2015 · 3 comments

Comments

@qbolec
Copy link

qbolec commented Feb 12, 2015

Consider this code:

var UserController = Ember.ObjectController.extend({
  firstName: "John",
  lastName: "Doe",
  largeFirstLetter:function(){
    console.log("Recomputing");
    return this.get('firstLetter').toUpperCase();
  }.property('firstLetter'),
  firstLetter:function(){
    return this.get('fullName').substr(0,1);
  }.property('fullName'),
  fullName:function(){
    return this.get('fisrtName') + ' ' + this.get('lastName');
  }.property('firstName','lastName')
});
var userController = new UserController();

console.log(userController.get('largeFirstLetter'));
console.log(userController.get('largeFirstLetter'));
userController.set('lastName','Smith');
console.log(userController.get('largeFirstLetter'));

http://jsfiddle.net/L95npwyj/

The idea is that we have a dependency tree :


largeFirstLetter <- firstLetter <- fullName <-- firstName
                                            <-- lastName

And the operations we perform change lastName, and fullName, but not firstLetter.

The output on the console is:

Recomputing
U
U
Recomputing
U

Ember spec seems to promise to me that largeFirstLetter will be recomputed only if it's dependencies are no longer valid. But the experiment reveals that what it actually means is that "deeper" dependencies, such as lastName are also taken into account. So even though firstLetter (the only direct dependency of largeFirstLetter) does not change, the largeFirstLetter gets recomputed.

The example above is very simple, but we hit this problem in a more advanced scenario where it was causing a lot of redraws in situations where nothing actually changed.

I've "fixed" it with making firstLetter a real (not computed) property, and manually recomputing it using observe('fullName').

Am I doing something wrong?

@stefanpenner
Copy link
Member

CP's are lazy, once invalidated something needs to pull the data out of them.

imagine the following DK chain

foo -> bar -> baz

if foo becomes dirty, the values for bar + baz are also not known, as they are derived from foo. Even if foo ultimately computes to its original value the current experience is to still recompute bar + baz.

For many scenarios this actually works fine, and wont cause re-renders. Because of simple template bindings, we "diff" the previous and new value to decide if a re-render is needed.

the basic diffing code actually lives here

Where this does fall short currently is when dealing with complex objects as values. And we are aware of this.

@tomdale and @wycats are currently working on the template rendering side of this problem: tildeio/htmlbars#282. Although the goal is for server side rendering to be scooped up by the client side, it has the potential to help mitigate the issue.

On the CP side, we currently have paired changeEvent contract of willChange followed by a didChange. Although it would be a breaking change, we may be able re-imagine this as

  • willChange
  • downstream pulls
  • previous and newValue are compared
  • didChange is emitted if the newValue is different.

Ultimately, the cp side change, as I imagine it, will only work with simple values, not with arrays or objects, so in the pathalogical cases it may not help that much. Unfortunately, although it may seem simple this change is likely pretty dramatic and not without caveats.

As such I do believe the template rendering side has sufficient context to handle the complex object case much better.

Although both solutions could work together, the reactive template approach is likely be the near term solution.

@qbolec
Copy link
Author

qbolec commented Feb 12, 2015

In our scenario the problem was that largeFirstLetter was actually something somewhat difficult to compute (comparing message text against search query) and thus reevaluating it on 1k+ messages even though the query didn't change, seemed a bit slow. Probably I attributed the slowdown to re-renders while it was actually the computation itself that was so slow. In other words: consider a scenario in which re-renders are not the issue, but the re-computation of the property is costly.

Or, is it a bad pattern to have CP which are costly to compute?

@stefanpenner
Copy link
Member

this appears to be work as expected, our view layer should now be more resilient to such changes and prevent unexpected view churn.

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

2 participants