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.

Sign up for GitHub

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

Deferred updates, part 3: dependency tree scheduling #1795

Merged
merged 7 commits into from
Jul 10, 2015
Merged

Deferred updates, part 3: dependency tree scheduling #1795

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
baccd6e
With multiple deferred computed observables in a dependency tree, use…
mbest May 15, 2015
6d5d786
Handle dirty/change events without the need of a new property (_rateL…
mbest May 27, 2015
048179c
Make sure that rate-limit supersedes deferred updates.
mbest May 27, 2015
72d5c43
Revert most changes in 6d5d7868d426276f7. This makes rate-limited com…
mbest Jun 4, 2015
a0134f5
Computed observables used for bindings (identified by having a dispos…
mbest Jun 4, 2015
4ca0819
Ensure developers don't get confused by writing ".extend({deferred:fa…
SteveSanderson Jul 3, 2015
f5a4d17
fix trailing space
mbest Jul 7, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 142 additions & 28 deletions spec/asyncBehaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,15 +648,6 @@ describe('Deferred', function() {
expect(notifySpy.argsForCall).toEqual([ ['B'] ]);
});

it('Set to false initially, should maintain synchronous notification', function() {
var observable = ko.observable().extend({deferred:false});
var notifySpy = jasmine.createSpy('notifySpy');
observable.subscribe(notifySpy);

observable('A');
expect(notifySpy.argsForCall).toEqual([ ['A'] ]);
});

it('Should suppress notification when value is changed/reverted', function() {
var observable = ko.observable('original').extend({deferred:true});
var notifySpy = jasmine.createSpy('notifySpy');
Expand All @@ -671,25 +662,6 @@ describe('Deferred', function() {
expect(observable()).toEqual('original');
});

it('Set to false, should turn off deferred notification if already turned on', function() {
var observable = ko.observable().extend({deferred:true});
var notifySpy = jasmine.createSpy('notifySpy');
observable.subscribe(notifySpy);

// First, notifications are deferred
observable('A');
expect(notifySpy).not.toHaveBeenCalled();
jasmine.Clock.tick(1);
expect(notifySpy.argsForCall).toEqual([ ['A'] ]);

notifySpy.reset();
observable.extend({deferred:false});

// Now, they are synchronous
observable('B');
expect(notifySpy.argsForCall).toEqual([ ['B'] ]);
});

it('Is default behavior when "ko.options.deferUpdates" is "true"', function() {
this.restoreAfter(ko.options, 'deferUpdates');
ko.options.deferUpdates = true;
Expand Down Expand Up @@ -808,6 +780,86 @@ describe('Deferred', function() {
expect(notifySpy.argsForCall).toEqual([ ['B'] ]);
});

it('Should delay update of dependent computed observable', function() {
var data = ko.observable('A'),
deferredComputed = ko.computed(data).extend({deferred:true}),
dependentComputed = ko.computed(deferredComputed);

expect(dependentComputed()).toEqual('A');

data('B');
expect(deferredComputed()).toEqual('B');
expect(dependentComputed()).toEqual('A');

data('C');
expect(dependentComputed()).toEqual('A');

jasmine.Clock.tick(1);
expect(dependentComputed()).toEqual('C');
});

it('Should delay update of dependent pure computed observable', function() {
var data = ko.observable('A'),
deferredComputed = ko.computed(data).extend({deferred:true}),
dependentComputed = ko.pureComputed(deferredComputed);

expect(dependentComputed()).toEqual('A');

data('B');
expect(deferredComputed()).toEqual('B');
expect(dependentComputed()).toEqual('A');

data('C');
expect(dependentComputed()).toEqual('A');

jasmine.Clock.tick(1);
expect(dependentComputed()).toEqual('C');
});

it('Should *not* delay update of dependent deferred computed observable', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

var data = ko.observable('A'),
timesEvaluated = 0,
computed1 = ko.computed(function () { return data() + 'X'; }).extend({deferred:true}),
computed2 = ko.computed(function () { timesEvaluated++; return computed1() + 'Y'; }).extend({deferred:true}),
notifySpy = jasmine.createSpy('notifySpy'),
subscription = computed2.subscribe(notifySpy);

expect(computed2()).toEqual('AXY');
expect(timesEvaluated).toEqual(1);

data('B');
expect(computed2()).toEqual('BXY');
expect(timesEvaluated).toEqual(2);
expect(notifySpy).not.toHaveBeenCalled();

jasmine.Clock.tick(1);
expect(computed2()).toEqual('BXY');
expect(timesEvaluated).toEqual(2); // Verify that the computed wasn't evaluated again unnecessarily
expect(notifySpy.argsForCall).toEqual([ ['BXY'] ]);
});

it('Should *not* delay update of dependent rate-limited computed observable', function() {
var data = ko.observable('A'),
deferredComputed = ko.computed(data).extend({deferred:true}),
dependentComputed = ko.computed(deferredComputed).extend({rateLimit: 500});
notifySpy = jasmine.createSpy('notifySpy'),
subscription = dependentComputed.subscribe(notifySpy);

expect(dependentComputed()).toEqual('A');

data('B');
expect(deferredComputed()).toEqual('B');
expect(dependentComputed()).toEqual('B');

data('C');
expect(dependentComputed()).toEqual('C');
expect(notifySpy).not.toHaveBeenCalled();

jasmine.Clock.tick(500);
expect(dependentComputed()).toEqual('C');
expect(notifySpy.argsForCall).toEqual([ ['C'] ]);
});

it('Is default behavior when "ko.options.deferUpdates" is "true"', function() {
this.restoreAfter(ko.options, 'deferUpdates');
ko.options.deferUpdates = true;
Expand All @@ -824,5 +876,67 @@ describe('Deferred', function() {
jasmine.Clock.tick(1);
expect(notifySpy.argsForCall).toEqual([ ['B'] ]);
});

it('Is superseded by rate-limit', function() {
this.restoreAfter(ko.options, 'deferUpdates');
ko.options.deferUpdates = true;

var data = ko.observable('A'),
deferredComputed = ko.computed(data),
dependentComputed = ko.computed(function() { return 'R' + deferredComputed(); }).extend({rateLimit: 500}),
notifySpy = jasmine.createSpy('notifySpy'),
subscription1 = deferredComputed.subscribe(notifySpy),
subscription2 = dependentComputed.subscribe(notifySpy);

expect(dependentComputed()).toEqual('RA');

data('B');
expect(deferredComputed()).toEqual('B');
expect(dependentComputed()).toEqual('RB');
expect(notifySpy).not.toHaveBeenCalled(); // no notifications yet

jasmine.Clock.tick(1);
expect(notifySpy.argsForCall).toEqual([ ['B'] ]); // only the deferred computed notifies initially

jasmine.Clock.tick(499);
expect(notifySpy.argsForCall).toEqual([ ['B'], [ 'RB' ] ]); // the rate-limited computed notifies after the specified timeout
});

it('Should minimize evaluation at the end of a complex graph', function() {
this.restoreAfter(ko.options, 'deferUpdates');
ko.options.deferUpdates = true;

var a = ko.observable('a'),
b = ko.pureComputed(function b() {
return 'b' + a();
}),
c = ko.pureComputed(function c() {
return 'c' + a();
}),
d = ko.pureComputed(function d() {
return 'd(' + b() + ',' + c() + ')';
}),
e = ko.pureComputed(function e() {
return 'e' + a();
}),
f = ko.pureComputed(function f() {
return 'f' + a();
}),
g = ko.pureComputed(function g() {
return 'g(' + e() + ',' + f() + ')';
}),
h = ko.pureComputed(function h() {
return 'h(' + c() + ',' + g() + ',' + d() + ')';
}),
i = ko.pureComputed(function i() {
return 'i(' + a() + ',' + h() + ',' + b() + ',' + f() + ')';
}).extend({notify:"always"}), // ensure we get a notification for each evaluation
notifySpy = jasmine.createSpy('callback'),
subscription = i.subscribe(notifySpy);

a('x');
jasmine.Clock.tick(1);
expect(notifySpy.argsForCall).toEqual([['i(x,h(cx,g(ex,fx),d(bx,cx)),bx,fx)']]); // only one evaluation and notification
});
});
});
10 changes: 10 additions & 0 deletions spec/asyncBindingBehaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>";
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :)

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 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.

var observable = ko.observable();
expect(function() {
ko.applyBindings({ x: observable }, testNode);
jasmine.Clock.tick(1);
}).not.toThrow();
expect(observable()).not.toBeUndefined(); // The spec doesn't specify which of the two possible values is actually set
});
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

});
9 changes: 9 additions & 0 deletions spec/defaultBindings/valueBehaviors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,15 @@ describe('Binding: Value', function() {
expect(dropdown.selectedIndex).toEqual(2);
});

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>";
var observable = ko.observable();
expect(function() {
ko.applyBindings({ x: observable }, testNode);
}).not.toThrow();
expect(observable()).not.toBeUndefined(); // The spec doesn't specify which of the two possible values is actually set
});

describe('Using valueAllowUnset option', function () {
it('Should display the caption when the model value changes to undefined, null, or \"\" when using \'options\' binding', function() {
var observable = ko.observable('B');
Expand Down
1 change: 0 additions & 1 deletion src/subscribables/dependencyDetection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,5 @@ ko.computedContext = ko.dependencyDetection = (function () {
ko.exportSymbol('computedContext', ko.computedContext);
ko.exportSymbol('computedContext.getDependenciesCount', ko.computedContext.getDependenciesCount);
ko.exportSymbol('computedContext.isInitial', ko.computedContext.isInitial);
ko.exportSymbol('computedContext.isSleeping', ko.computedContext.isSleeping);

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 noticed that we were exporting a non-existent property, so I took it out.

ko.exportSymbol('ignoreDependencies', ko.ignoreDependencies = ko.dependencyDetection.ignore);
46 changes: 38 additions & 8 deletions src/subscribables/dependentObservable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,45 @@ ko.computed = ko.dependentObservable = function (evaluatorFunctionOrOptions, eva
isSleeping = false;
}

function subscribeToDependency(target) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function will be quite a lot more expensive than the usual 'subscribe' if we go into the 'deferred' code path, both in memory and CPU use, as now it constructs several extra function instances with their own closures. This once for each dependency, every time any 'deferred' computed is re-evaluated.

It might be OK, but is there any possibility of dropping these dynamically created closures in favour of static function instances and variables that get mutated?

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 had originally written this with static functions but then realized I needed to access target in one of them. I'll take another look at it, though, to see if there's another way. Also this will only run once for each dependency because when a computed is re-evaluated, it saves and reuses previous subscriptions.

if (target._deferUpdates && !disposeWhenNodeIsRemoved) {
var dirtySub = target.subscribe(markDirty, null, 'dirty'),
changeSub = target.subscribe(respondToChange);
return {
_target: target,
dispose: function () {
dirtySub.dispose();
changeSub.dispose();
}
};
} else {
return target.subscribe(evaluatePossiblyAsync);
}
}

function markDirty() {
// Process "dirty" events if we can handle delayed notifications
if (dependentObservable._evalDelayed && !_isBeingEvaluated) {
dependentObservable._evalDelayed();
}
}

function respondToChange() {
// Ignore "change" events if we've already scheduled a delayed notification
if (!dependentObservable._notificationIsPending) {
evaluatePossiblyAsync();
}
}

function evaluatePossiblyAsync() {
var throttleEvaluationTimeout = dependentObservable['throttleEvaluation'];
if (throttleEvaluationTimeout && throttleEvaluationTimeout >= 0) {
clearTimeout(evaluationTimeoutInstance);
evaluationTimeoutInstance = ko.utils.setTimeout(function () {
evaluateImmediate(true /*notifyChange*/);
}, throttleEvaluationTimeout);
} else if (dependentObservable._evalRateLimited) {
dependentObservable._evalRateLimited();
} else if (dependentObservable._evalDelayed) {
dependentObservable._evalDelayed();
} else {
evaluateImmediate(true /*notifyChange*/);
}
Expand Down Expand Up @@ -115,7 +145,7 @@ ko.computed = ko.dependentObservable = function (evaluatorFunctionOrOptions, eva
--disposalCount;
} else if (!dependencyTracking[id]) {
// Brand new subscription - add it
addDependencyTracking(id, subscribable, isSleeping ? { _target: subscribable } : subscribable.subscribe(evaluatePossiblyAsync));
addDependencyTracking(id, subscribable, isSleeping ? { _target: subscribable } : subscribeToDependency(subscribable));
}
}
},
Expand Down Expand Up @@ -230,14 +260,14 @@ ko.computed = ko.dependentObservable = function (evaluatorFunctionOrOptions, eva
var originalLimit = dependentObservable.limit;
dependentObservable.limit = function(limitFunction) {
originalLimit.call(dependentObservable, limitFunction);
dependentObservable._evalRateLimited = function() {
dependentObservable._rateLimitedBeforeChange(_latestValue);
dependentObservable._evalDelayed = function() {
dependentObservable._limitBeforeChange(_latestValue);

_needsEvaluation = true; // Mark as dirty

// Pass the observable to the rate-limit code, which will access it when
// Pass the observable to the "limit" code, which will access it when
// it's time to do the notification.
dependentObservable._rateLimitedChange(dependentObservable);
dependentObservable._limitChange(dependentObservable);
}
};

Expand All @@ -262,7 +292,7 @@ ko.computed = ko.dependentObservable = function (evaluatorFunctionOrOptions, eva
// Next, subscribe to each one
ko.utils.arrayForEach(dependeciesOrder, function(id, order) {
var dependency = dependencyTracking[id],
subscription = dependency._target.subscribe(evaluatePossiblyAsync);
subscription = subscribeToDependency(dependency._target);
subscription._order = order;
subscription._version = dependency._version;
dependencyTracking[id] = subscription;
Expand Down
19 changes: 8 additions & 11 deletions src/subscribables/extenders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,24 @@ ko.extenders = {
method = options['method'];
}

// rateLimit supersedes deferred updates
target._deferUpdates = false;

limitFunction = method == 'notifyWhenChangesStop' ? debounce : throttle;
target.limit(function(callback) {
return limitFunction(callback, timeout);
});
},

'deferred': function(target, value) {
// Calling with a true value sets up and enables deferred updates.
// A false value turns off deferred updates if it was previously enabled, but won't unnecessarily set a limit function.
target._deferUpdates = value;
if (value) {
'deferred': function(target, options) {
if (!target._deferUpdates) {
target._deferUpdates = true;
target.limit(function (callback) {
var handle;
return function () {
ko.tasks.cancel(handle);
if (target._deferUpdates) {
handle = ko.tasks.schedule(callback);
} else {
handle = 0;
callback();
}
handle = ko.tasks.schedule(callback);
target['notifySubscribers'](undefined, 'dirty');
};
});
}
Expand Down
Loading