Reduce excessive databinding updates

Reduce excessive databinding updates

This greatly lowers the cost of having a large (600+) units in the 
inspector. Past an initial rendering hit the system performs to over 2k 
units in local testing.

https://code.launchpad.net/~bcsaller/juju-gui/excessive-updates/+merge/174319

(do not edit description out of merge proposal)

[bcsaller, 2013-07-12 01:19:28]

Please take a look.

[bcsaller, 2013-07-12 01:47:06]

Please take a look.

[jeff.pihach, 2013-07-12 06:07:37]

Great work as always LGTM!

QA is good!

https://codereview.appspot.com/11198043/diff/4001/app/store/env/simulator.js
File app/store/env/simulator.js (left):

https://codereview.appspot.com/11198043/diff/4001/app/store/env/simulator.js#...
app/store/env/simulator.js:487: console.log('Simulator started');
yussssss!

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:120: this.options.interval : 250;
isn't 250ms a little excessive? Should we not match this to at a most the
interval rate of juju-core?

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:475: this._updateDOM(delta);
I don't think that this `else` clause can ever be hit because this.interval is
set in the init.

[gary.poster, 2013-07-12 11:24:54]

LGTM with comments and a test or two of the new aggregation code.  Thank you. 
Thanks also for removing some of the code that no longer has a use in the new
wireframes.

For tests, perhaps you could set the interval to 5 or so, and then  verify that
_updateDom is not called immediately and that the timeout object exists, then
wait 5 or 10 and verify that _updateDom has been called?

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:115: *          interval: ms window to restrict
multiple DOM udpates.
Trivial: might be nice to mention default. <shrug>

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:454: This introduces threshold of 250ms without trigger
before
This introduces a threshold (this.interval, defaulting to 250ms) without
triggering before

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:465: this._updateTimeout.cancel();
I guess this approach is probably fine.  I'd prefer it if this were not a
threshold but a resolution.  IOW, you never cancel the timeout, but you simply
clear it in _updateDOM, and don't do anything here if there is an interval and
an existing _updateTimeout.  This will hopefully mean that big clumps of changes
will show gradual changes as they work through the system, which would be a
better experience IMO.  WDYT?

[benjamin.saller, 2013-07-12 14:46:18]

On 2013/07/12 11:24:54, gary.poster wrote:
> LGTM with comments and a test or two of the new aggregation code.  Thank you. 
> Thanks also for removing some of the code that no longer has a use in the new
> wireframes.
> 
> For tests, perhaps you could set the interval to 5 or so, and then  verify
that
> _updateDom is not called immediately and that the timeout object exists, then
> wait 5 or 10 and verify that _updateDom has been called?

I suspect this pattern would lead to intermittent failures (wrt to CI where
times can vary a lot). I suspect the proper thing is to fire an event on the
actual render (which only testing has a use-case to call). I know this pattern
has its limitations but its the same pattern we're using in notifications where
I think the test ignore the debounce wrapper and call render directly. 
> 
> https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js
> File app/views/databinding.js (right):
> 
>
https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
> app/views/databinding.js:115: *          interval: ms window to restrict
> multiple DOM udpates.
> Trivial: might be nice to mention default. <shrug>
> 
>
https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
> app/views/databinding.js:454: This introduces threshold of 250ms without
trigger
> before
> This introduces a threshold (this.interval, defaulting to 250ms) without
> triggering before
> 
>
https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
> app/views/databinding.js:465: this._updateTimeout.cancel();
> I guess this approach is probably fine.  I'd prefer it if this were not a
> threshold but a resolution.  IOW, you never cancel the timeout, but you simply
> clear it in _updateDOM, and don't do anything here if there is an interval and
> an existing _updateTimeout.  This will hopefully mean that big clumps of
changes
> will show gradual changes as they work through the system, which would be a
> better experience IMO.  WDYT?

[benjamin.saller, 2013-07-12 14:48:11]

Thanks for the reviews. My current leaning is to keep the pattern in place as
indicated in the notes. We can revise this in the future if proves an issue (I
don't think it will though).

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js
File app/views/databinding.js (right):

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:115: *          interval: ms window to restrict
multiple DOM udpates.
On 2013/07/12 11:24:54, gary.poster wrote:
> Trivial: might be nice to mention default. <shrug>

Done.

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:120: this.options.interval : 250;
On 2013/07/12 06:07:38, jeff.pihach wrote:
> isn't 250ms a little excessive? Should we not match this to at a most the
> interval rate of juju-core?

The pattern used here is that we don't call updateDOM until this timeout has
elapsed w/o another call. Keeping this small keeps the steady state latency low.
During an activity storm it will delay updates till its over. As Gary points out
however, if this were 1sec and you got updates each second this could delay
update from ever being called. 

Keeping this low makes sense, but I could also change the pattern to traditional
rate limiting rather than a debounce.

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:465: this._updateTimeout.cancel();
On 2013/07/12 11:24:54, gary.poster wrote:
> I guess this approach is probably fine.  I'd prefer it if this were not a
> threshold but a resolution.  IOW, you never cancel the timeout, but you simply
> clear it in _updateDOM, and don't do anything here if there is an interval and
> an existing _updateTimeout.  This will hopefully mean that big clumps of
changes
> will show gradual changes as they work through the system, which would be a
> better experience IMO.  WDYT? 

I'm willing to change this but this pattern (with a low threshold as mentioned
above) has a similar effect. If the system really is mutating more  than 4 time
a second something else is up given our current model. If that model evolves to
real time then strict rate limiting makes sense. 

The other style has the advantage that we'd call it immediately and tests would
work normally. I find that compelling but the impl would still maintain a timer
and ensure that if it was called the method is triggered at each interval. This
is more code and more complexity, though if we feel we strongly need this we
could turn it into a proper Y.ratelmit(method, interval) wrapper. I don't think
we need that level of investment.

Given that I prefer this simple pattern.

https://codereview.appspot.com/11198043/diff/4001/app/views/databinding.js#ne...
app/views/databinding.js:475: this._updateDOM(delta);
On 2013/07/12 06:07:38, jeff.pihach wrote:
> I don't think that this `else` clause can ever be hit because this.interval is
> set in the init.

This is used in testing where we forced interval to zero.

[bcsaller, 2013-07-12 14:52:31]

*** Submitted:

Reduce excessive databinding updates

This greatly lowers the cost of having a large (600+) units in the 
inspector. Past an initial rendering hit the system performs to over 2k 
units in local testing.

R=jeff.pihach, gary.poster, benjamin.saller
CC=
https://codereview.appspot.com/11198043