Debounce change events to prevent event.on/event.off storm on batch change

[anton-ryzhov]

When ractive displays backbone's collection, it monitors events on collection itself and its models.
When we do collection.fetch() it triggers set/reset, on collection and add on each model.
Ractive does huge work on add, resetting event handlers, updates parent keypathes, etc.

As result — loading and displaying 20 models took ~1500 calling of changeHandler and 1.2 seconds on my machine (according to console.time/timeEnd).

I've added underscore.debounce (as far we already depend on underscore through backbone) to decrease change event triggering times.

This will slowdown rerender after atomic update for debounceInterval (50 msec), but add huge speedup in batch changes. In my case, 20 models — 30 calls of changeHandler and 30 msec total time. 40 times faster!

Actually I've got the same result with debounceInterval=10, but I think 50 is good balance between processing in slow browser and responsiveness in fast one.

[anton-ryzhov]

Hmm, I'm experiencing some issues after this patch. I'm going to analyze it deeply.
I'll write here results after it.

[rstacruz]

You don't have to edit the src/ one now: we've removed it and kept the one in the root to make things easier to manage.

[anton-ryzhov]

I've found problem and fixed it. Works good for me.
But tests are failed because debounce delay between models update and ractive receives changes, update becomes async.

Do you think this PR useful for the project? Are you going to merge it, should we think how to fix the tests?

[rstacruz]

Now that we're using mocha, we can do async tests:

it("should work", function (done) {
  model.on('change:message', function (value) {
    expect(value).to.eql('hi');
    done();
  });
  model.set('message', 'hi');
});

Can you see if you can update the tests where it fails?

Also, would you get the same benefit if you set debounceInterval to 0? hypothetically, if all the changes are done within the same process tick, deferring the processing to the next tick should be enough.

[anton-ryzhov]

Yes, I've googled for mocha async. We have to ractive.observe for test and done(), lots of rewriting.
debounceInterval=0 — great idea, looks promising. I'll check.

[rstacruz]

i tried to update the test accordingly (but didn't commit it):

https://gist.github.com/rstacruz/6444768b890ac4f0f6a6#file-ractive-test-diff

it seems to work perfectly :)

[anton-ryzhov]

Tricky a little bit, isn't it?

Also http://underscorejs.org/#defer may be useful as far we use underscore directly

[rstacruz]

It looks like _.defer is just a convenient alias for setTimeout(fn) though (which is functionally the same as setTimeout(fn, 0)) -- it should be exactly the same as _.debounce(fn) without the second parameter.

[anton-ryzhov]

Oh no, I've said about defer as idea to replace your wait in test. _.defer more predictable. I thought it is some mochas feature until I've seen definition in the end of file.
Maybe just rename it to nextTick and forget about defer)

But inside main code they have huge difference. defer just runs all this 1500 handlers later, and debounce merges several sequential calls into one. This is the goal of this PR

[anton-ryzhov]

I've tested your idea about debounceInterval=0, works one-to-one as previous version but without any extra delay. Great, I've added to PR.

[anton-ryzhov]

+Fixed unittests

[Rich-Harris]

loading and displaying 20 models took ~1500 calling of changeHandler and 1.2 seconds on my machine

😧

Thanks for flagging this @anton-ryzhov - no question that there's a major problem here. I haven't looked into it at all, but is there any way of solving it without debouncing (a flag on theCollection of some sort?) If not then updates will have to be async, but I think it'd be preferable to avoid that if possible, as Ractive updates synchronously everywhere else and it might get confusing if you're expecting the DOM to be in a particular state - you could end up with all sorts of race conditions:

list.push({ foo: 'bar' });
ractive.set( 'listHeight', ractive.find( 'ul' ).offsetHeight );

I get a chance today I'll have a poke as well

[anton-ryzhov]

I've found this bug in one case:

1. Ractive has already rendered, wrappers created and watching for the events.
2. I'm calling collection.fetch() - async load from serverside.

Internally we have these steps:

1. Fetch done callback sets data: https://github.com/jashkenas/backbone/blob/1.1.2/backbone.js#L865
2. Collection triggers lots of add https://github.com/jashkenas/backbone/blob/1.1.2/backbone.js#L749
3. Collection triggers sync after all https://github.com/jashkenas/backbone/blob/1.1.2/backbone.js#L867

Maybe there is another reproduce scenarios in some rare cases, but this is definitely one of the most frequent.
@Rich-Harris make interesting point that we should keep synchronous workflow as much as we can. Now I do agree with him.

This lib should listen to add events for case of adding one item to exist collection. But we should filter out multiple adds after fetch. We cal rely on sync event to turn on add listening, but we haven't any event to turn it off before multiple adds will be triggered.

Looks like .fetch({reset: true}) works as required — it triggers only reset and sync events, but this is not default behavior of backbone.

Now I don't know better way than decorating/wrapping collection.set method (as described http://docs.ractivejs.org/latest/writing-adaptor-plugins) with acquire-release lock like in #21.

Any another ideas?

[anton-ryzhov]

#28 replaces this PR