Deferred updates, part 3: dependency tree scheduling #1795

mbest · 2015-05-15T23:14:59Z

This is the third, and final, part of supporting deferred updates in Knockout (#1728).

With multiple deferred computed observables in a dependency tree, use "dirty" events to schedule the complete set of updates in the correct order and mark each computed observable as "dirty" so it can be evaluated if needed with the latest value.

… "dirty" events to schedule the complete set of updates in the correct order and mark each computed observable as "dirty" so it can be evaluated if needed with the latest value.

mbest · 2015-05-16T17:17:39Z

Originally, in #1728, I thought that we could use the observable versioning system, which is used to efficiently to update sleeping pure computed observables, to accomplish a similar task for deferred updates. At the very least, I wanted to try it and compare it to using a dirty event. After completing #1753 (part 2), though, I took a detour with #1775 to develop a system to properly order subscriptions, especially for deferred updates. After spending a couple of weeks working on it, I realized that it wouldn't be enough to accomplish the goal for deferred updates: to update pure computed observables in a breadth-first order without any duplicate updates. Combining it with version checking might help, but the whole thing was big and not very efficient.

So I went back to trying the dirty event, understanding that it could solve both problems for deferred updates. Scheduling (or rescheduling) the update of each computed observable as it receives dirty notifications results in the correct update order. And because this marks each observable as dirty in the process, it efficiently causes them to evaluate to the latest value if accessed while an update is pending.

mbest · 2015-05-16T17:25:46Z

spec/asyncBindingBehaviors.js

+        expect(function() {
+            jasmine.Clock.tick(1);
+        }).toThrowContaining('Too much recursion');
+    });


This test presents a concrete example of how the recursive task check could come into play (and is the reason I added the check in the deferred updates plugin). It's interesting, though, that this example doesn't result in an infinite loop before the code changes here.

Are you regarding this as not a breaking change because the previous behavior was not well defined? I'm not sure what the previous behavior would have been, as it does seem impossible for 'x' to satisfy the requirements of both 'select' boxes simultaneously.

Right. I'd say that the previous behavior was undefined.

mbest · 2015-05-21T22:18:12Z

@SteveSanderson, I'm hoping to get your feedback here. This obviously involves some code changes within dependentObservable.js, but I hope that the logic is isolated enough to be easily digestible.

…imitIsPending)

SteveSanderson · 2015-06-02T13:06:15Z

spec/asyncBehaviors.js

+            expect(dependentComputed()).toEqual('C');
+        });
+
+        it('Should *not* delay update of dependent deferred computed observable', function () {


This subtlety - how notifications and re-evaluations differ between regular, ratelimited, and deferred computeds is very subtle indeed and is going to require careful attention in docs. Probably some big table of comparisons, with notes that provide justifications for all the differences.

I think I can guess why these differences exist, but it was still surprising to me at first.

SteveSanderson · 2015-06-02T13:21:27Z

Thanks for this! Comments and questions added.

mbest · 2015-06-02T21:40:00Z

Another point to consider before merging this is that "dirty" events will cause a computed to re-evaluate even if the underlying observable doesn't change. In my plugin, I dealt with this by having two levels of "dirty". A dirty event marks a computed as possibly dirty, and a change event marks it as actually dirty. When possibly dirty, it will be evaluated on demand, but only if actually dirty will it be evaluated during task processing. Do you think I should try to include the same logic here?

…rvables update on demand when dependent on deferred observables, maintaining similar behavior for rate-limited computeds between deferred and non-deferred usage. This commit also removes the ability to turn off deferred updates for an observable because it would result in an observable that appears to be rate-limited but actually updates synchronously.

…eWhenNodeIsRemoved option) won't respond to "dirty" events. This prevents conflicting bindings from causing recursive updates.

mbest · 2015-06-10T02:50:02Z

@SteveSanderson I've made some changes that address most of the concerns you brought up. Please check them over and give me your thoughts.

mbest · 2015-06-10T02:54:50Z

spec/asyncBindingBehaviors.js

@@ -125,4 +125,14 @@ describe("Deferred bindings", function() {
        expect(testNode.childNodes[0]).toContainHtml('<span data-bind="text: childprop">moving child</span><span data-bind="text: childprop">first child</span><span data-bind="text: childprop">second child</span>');
        expect(testNode.childNodes[0].childNodes[targetIndex]).not.toBe(itemNode);    // node was create anew so it's not the same
    });
+
+    it('Should not throw an exception for value binding on multiple select boxes', function() {
+        testNode.innerHTML = "<select data-bind=\"options: ['abc','def','ghi'], value: x\"></select><select data-bind=\"options: ['xyz','uvw'], value: x\"></select>";


Given that the non-deferred scenario doesn't throw an exception, I decided to give this some more thought. I realized that if the computed observables used for updating the binding don't respond to "dirty" events, that would stop the recursion as long as the underlying observable suppresses notifications for non-changing updates. So this will still cause a "recursion" exception if the values are objects instead of strings. @SteveSanderson, do you think this change is helpful or not?

If this brings the deferred behavior more into line with non-deferred, then I guess it's a good thing, but it's hard to think of a case where someone would be doing this and expect something useful to happen :)

I think we may need to take a closer look at how bindings interact with the deferred updates feature, as you've described in #1758. So this code may need to change some more anyway.

…lse})" - it would have the opposite effect

SteveSanderson · 2015-07-03T15:14:35Z

Thanks again for implementing this, and sorry it's taken a few weeks to complete the review.

This looks good to me - the notion of 'dirty' events propagating through any unbroken chain of deferreds to spread awareness that a change is coming, and hence that they should schedule a re-evaluation (and re-evaluate proactively if read) seems like a good way to model this.

I'm happy for this to go in (so please merge it if you're ready, or let me know and I will), but I just want to ask a couple of clarifying questions in case you want to add further tweaks:

What's the reason why we can no longer support turning off deferred? Is it that we'd have to recreate all the subscriptions to dependencies (and would it be prohibitively difficult to implement that)? I know you put in the comment that "it would result in an observable that appears to be rate-limited but actually updates synchronously" but I'm unsure why that behavior is necessary.
- I've added a check that you don't try to use .extend({ deferred : false }) as that code would actually enable deferred :)
- It seems odd that you actually can turn off deferred by turning on rateLimit (which you can do at arbitrary times). Would this lead to bugs? Why is it safe to enable rateLimit and hence disable deferred, if you can't just disable deferred on its own?
Is it true that you can't have an observable/computed that is both deferred and rate-limited at the same time?
- I hope not, because then there are so many combinations of [observable/computed/pureComputed] * [deferred on/off] * [rateLimit on/off] and I don't have any idea how the deferred+rateLimited combinations should behave
- Should we add an exception if you try to use both extenders on the same thing?

mbest · 2015-07-04T02:05:34Z

@SteveSanderson, I'm glad to get your feedback. Hopefully we can get this in soon.

What's the reason why we can no longer support turning off deferred?

One reason is that I realized that it creates a new class of observables that would need to be thoroughly tested (and the implementation probably adjusted accordingly). Another reason is that I decided that I didn't want to encourage turning off deferred updates for an individual observable when using the global deferUpdates option, mostly because I couldn't decide how such a scenario should behave.

Is it true that you can't have an observable/computed that is both deferred and rate-limited at the same time?

That's correct. Both use the internal limit function and setting one turns off the other.

Why is it safe to enable rateLimit and hence disable deferred, if you can't just disable deferred on its own?

When using deferred updates globally, it still makes sense that people will sometimes want to use rateLimit for specific observables. The rate-limited observable will behave as expected whether used with other normal or deferred observables.

Should we add an exception if you try to use both extenders on the same thing?

We need to support setting rateLimit on a deferred observable but not vice versa. We could add an exception for the latter case, although I don't think it's necessary.

brianmhunt · 2015-07-04T03:11:06Z

Is it true that you can't have an observable/computed that is both deferred and rate-limited at the same time?
That's correct. Both use the internal limit function and setting one turns off the other.

Is deferring analogous to rateLimit: 0 (or 1), much like setTimeout(fn, 0)? If not, how are they different?

mbest · 2015-07-04T08:27:53Z

@brianmhunt That's a good question. If you use deferred for a single observable, it could be equivalent to rateLimit: 0, at least on older browsers. Once you have multiple deferred observables, you'll see more benefit because all updates happen together with no delay between them. This might help:

Default - Notifications happen immediately and synchronously.
Deferred - Notifications happen asynchronously as soon as possible, hopefully before any redraws.
Rate-limit - Notifications happen after a specified period of time (minimum of 2-10 ms depending on the browser).

brianmhunt · 2015-07-04T14:44:04Z

@mbest Got it – that's very helpful (and probably worthwhile to reference in the docs!).

Deferred updates, part 3: dependency tree scheduling

mbest · 2015-07-10T21:27:49Z

This is merged now.

mbest force-pushed the deferred-updates-part-3 branch from bb4a26b to 8b14e81 Compare May 16, 2015 00:03

With multiple deferred computed observables in a dependency tree, use…

baccd6e

… "dirty" events to schedule the complete set of updates in the correct order and mark each computed observable as "dirty" so it can be evaluated if needed with the latest value.

mbest force-pushed the deferred-updates-part-3 branch from 8b14e81 to baccd6e Compare May 16, 2015 01:03

mbest added this to the 3.4.0 milestone May 16, 2015

mbest added severity: major type: feature affected: most labels May 16, 2015

mbest mentioned this pull request May 16, 2015

Include deferred updates in Knockout core #1728

Closed

mbest reviewed May 16, 2015
View reviewed changes

mbest added 2 commits May 26, 2015 23:15

Handle dirty/change events without the need of a new property (_rateL…

6d5d786

…imitIsPending)

Make sure that rate-limit supersedes deferred updates.

048179c

mbest force-pushed the deferred-updates-part-3 branch from 76c6364 to 048179c Compare May 27, 2015 21:43

SteveSanderson reviewed Jun 2, 2015
View reviewed changes

mbest added 2 commits June 3, 2015 14:15

Computed observables used for bindings (identified by having a dispos…

a0134f5

…eWhenNodeIsRemoved option) won't respond to "dirty" events. This prevents conflicting bindings from causing recursive updates.

mbest reviewed Jun 10, 2015
View reviewed changes

mbest mentioned this pull request Jun 12, 2015

Controlling which parts of the UI have their bindings deferred #1758

Open

Ensure developers don't get confused by writing ".extend({deferred:fa…

4ca0819

…lse})" - it would have the opposite effect

fix trailing space

f5a4d17

mbest added a commit that referenced this pull request Jul 10, 2015

Merge pull request #1795 from knockout/deferred-updates-part-3

6fa03bb

Deferred updates, part 3: dependency tree scheduling

mbest merged commit 6fa03bb into master Jul 10, 2015

mbest deleted the deferred-updates-part-3 branch July 10, 2015 21:27

rniemeyer mentioned this pull request Aug 30, 2015

Release version 3.4.0 #1858

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deferred updates, part 3: dependency tree scheduling #1795

Deferred updates, part 3: dependency tree scheduling #1795

mbest commented May 15, 2015

mbest commented May 16, 2015

mbest May 16, 2015

SteveSanderson Jun 2, 2015

mbest Jun 2, 2015

mbest commented May 21, 2015

SteveSanderson Jun 2, 2015

SteveSanderson commented Jun 2, 2015

mbest commented Jun 2, 2015

mbest commented Jun 10, 2015

mbest Jun 10, 2015

SteveSanderson Jul 3, 2015

mbest Jul 4, 2015

SteveSanderson commented Jul 3, 2015

mbest commented Jul 4, 2015

brianmhunt commented Jul 4, 2015

mbest commented Jul 4, 2015

brianmhunt commented Jul 4, 2015

mbest commented Jul 10, 2015

Deferred updates, part 3: dependency tree scheduling #1795

Deferred updates, part 3: dependency tree scheduling #1795

Conversation

mbest commented May 15, 2015

mbest commented May 16, 2015

mbest May 16, 2015

Choose a reason for hiding this comment

SteveSanderson Jun 2, 2015

Choose a reason for hiding this comment

mbest Jun 2, 2015

Choose a reason for hiding this comment

mbest commented May 21, 2015

SteveSanderson Jun 2, 2015

Choose a reason for hiding this comment

SteveSanderson commented Jun 2, 2015

mbest commented Jun 2, 2015

mbest commented Jun 10, 2015

mbest Jun 10, 2015

Choose a reason for hiding this comment

SteveSanderson Jul 3, 2015

Choose a reason for hiding this comment

mbest Jul 4, 2015

Choose a reason for hiding this comment

SteveSanderson commented Jul 3, 2015

mbest commented Jul 4, 2015

brianmhunt commented Jul 4, 2015

mbest commented Jul 4, 2015

brianmhunt commented Jul 4, 2015

mbest commented Jul 10, 2015