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

Add caching to sleeping pure computed observables #1543

Merged
merged 5 commits into from Jan 16, 2015

Conversation

mbest
Copy link
Member

@mbest mbest commented Sep 18, 2014

This is mainly in response to #1531, although I had the idea to do this while originally working on the pure computed feature.

@mbest
Copy link
Member Author

mbest commented Sep 18, 2014

  1. Observables (normal and computed) include a version number that's updated whenever their value changes. The version number is updated immediately and is not associated with change notification, which might be delayed through rateLimit.
  2. Pure computed observables:
    • Save the version number of each observable dependency during evaluation.
    • Check whether any dependencies have changed using the version numbers before re-evaluating.
    • When awakening, subscribe to their dependencies without necessarily re-evaluating. Because some browsers re-order numeric properties, we need to keep track of the order ourselves.
    • While sleeping, perform a deep check for dependency changes.
  3. ko.subscription's target property changed to _target for better minification.

…ward supporting caching for sleeping computeds
…on number for each observable that's incremented when the observable's value is changed.
@mbest
Copy link
Member Author

mbest commented Jan 6, 2015

Since this change is all about performance, it's useful to check that there are no negative affects on performance. To check this, I set up a series of benchmark tests using a simple computed observable that just returns the value of a single observable. The following actions showed no significant change in performance:

  1. Creating a normal computed
  2. Creating a pure computed
  3. Accessing a normal (or awake) computed
  4. Updating a normal (or awake) computed

The following actions showed a significant change in performance:

  1. Accessing a sleeping computed whose dependencies haven't changed: about 3 times faster
  2. Accessing a sleeping computed whose dependencies have changed: about 40% slower (but about par with updating a normal computed)

As the computed observable itself becomes more complex, those two differences will improve as well.

@brianmhunt, @rniemeyer, @SteveSanderson, I'd like to get this merged in soon. So please let me know if you have any concerns.

@brianmhunt
Copy link
Member

Cool. This is all a bit beyond me, but I like the idea. Would every ko.subscribable will now have an associated version, which changes whenever the observable does?

@mbest
Copy link
Member Author

mbest commented Jan 6, 2015

Would every ko.subscribable will now have an associated version, which changes whenever the observable does?

Yes. Putting it in ko.subscribable allows the API and code to be shared between computed and plain observables.

@mbest mbest changed the title Add caching to sleeping pure computed observables and other minor fixes Add caching to sleeping pure computed observables Jan 6, 2015
@mbest mbest mentioned this pull request Jan 6, 2015
3 tasks
@rniemeyer
Copy link
Member

@mbest - I'll start taking a detailed look at this one and merge this week

@mbest
Copy link
Member Author

mbest commented Jan 6, 2015

Thanks, Ryan.

observable.valueHasMutated = function () { observable["notifySubscribers"](_latestValue); }
observable.valueWillMutate = function () { observable["notifySubscribers"](_latestValue, "beforeChange"); }
observable.valueHasMutated = function () {
observable.updateVersion();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think the above line should perhaps be moved inside notifySubscribers? The version number would then mean "notification number" rather than "mutation number".

The reason I suggest this is that I think there's a lot of code out in the wild that does stuff like this:

myObservable().subProperty = "New value";
myObservable.notifySubscribers();

I know that's not the recommended pattern, but people may do it (and in practice, there are sure to be plenty of cases where it is done).

If the version continues to refer to mutations rather than notifications, pure computeds won't update correctly in this case, will they?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about using the notification, but if the observable is rate-limited, that would change when a sleeping computed would see the new value. Also how would it know when a sleeping computed has changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could update the version in both cases (mutation and notification), at least for rate-limited and sleeping observables.

@SteveSanderson
Copy link
Contributor

In principle this sounds great. The way you've described the feature totally makes sense, and thanks for quantifying the perf impact.

I won't pretend to have made sense of every line in the diff (in fact, it's complex enough that I probably would have to reimplement it from scratch to understand it) but I'm fine with trusting that you know what you are doing with this :)

Some line comments added.

@mbest
Copy link
Member Author

mbest commented Jan 7, 2015

I split the code changes into two commits to help make the actual changes more clear. The first commit mainly re-organizes the code and doesn't change functionality. If you ignore white-space changes, it's a little clearer.

@mbest
Copy link
Member Author

mbest commented Jan 8, 2015

I've added a test regarding the order of subscriptions. I hope it's clear enough.

…on, except for a sleeping pure computed to support chaining of pure computeds.
@mbest
Copy link
Member Author

mbest commented Jan 8, 2015

@SteveSanderson, I thought some more about the notification/mutation issue and now agree with you that the version should be changed on change notification. This makes a sleeping computed act more like a normal computed when dependent on a rate-limited observable. I've added some tests to show this.

@rniemeyer
Copy link
Member

I think this looks good to merge. @SteveSanderson - do you feel that your concerns were addressed?

@SteveSanderson
Copy link
Contributor

@mbest's comments are comforting :) I'll check the code and merge it in the morning hopefully!

SteveSanderson added a commit that referenced this pull request Jan 16, 2015
Add caching to sleeping pure computed observables
@SteveSanderson SteveSanderson merged commit ddcfbfc into knockout:master Jan 16, 2015
@SteveSanderson
Copy link
Contributor

Thanks again @mbest! This looks like it was tricky to get right, so thanks for being so thorough with the testing. It's great that ko.pureComputed will now re-evaluate just as infrequently as a regular ko.computed while also retaining its memory-safe properties. This is cool stuff!

We have so much functionality around computeds by now that the logic in dependentObservable.js is really intricate... I think we've done a great job of keeping it robust by having such extensive test coverage, but it's absolutely at the limit of what I think is practical to maintain. I can't say I really understand it without mentally stepping through dozens of if/else cases and trying to contemplate all the scenarios. In the future, we'll have to focus on layering additional functionality on from outside (rather than expanding the core dependent observable logic) if at all possible.

Anyway, great to get this in :)

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

Successfully merging this pull request may close these issues.

None yet

4 participants