Faster Collection _reset

[akre54]

As discovered in #2098, resetting Collection#models with an empty array is slow and prone to memory leaks (see SO post).

Setting this.models.length = 0 shows a manifold (up to about 50x) improvement over master: jsperf

[smagch]

Wow! It's much faster! I didn't know the performance tip. I've been thinking that length property is read-only. According to MDN documentation, it's proper way of truncating Array.

[caseywebdev]

👍 Looks good to me

[braddunbar]

Good stuff. Thanks @akre54!

[caseywebdev]

@akre54 Unfortunately your test is flawed. The results seemed a little too good to be true, so I traced exactly what was happening. You are creating models from only the most recently loaded Backbone (your branch) and adding those to the empty collections. Because of this, the attrs instanceof Model check in _prepareModel is failing (only for master, because the model was created from your-patch), forcing a new model to be created in add with a whole slew of attributes (since set no longer takes a model as an attribute object).

Here is a more fair test comparing just before your changes to afterward: http://jsperf.com/bb-faster-array-reset/2

[akre54]

Ahh damn. Good catch! I knew that big of a jump seemed usual. Moral of the story: don't use Backbone Models? 😀

I copied the Events test setup from http://jsperf.com/events-vs-events2, and didn't consider the knock-on effects from models, thanks for tracking it down. I'll try and dive a little deeper next time.

[webxl]

This appears to be breaking Backbone.Paginator (ClientPager specifically) on Chrome 25.0.1364.29 beta and Firefox 17. I'll run some more tests and post a test case

[akre54]

@caseywebdev feel free to revert. no sense in keeping broken code around.

P.S. want to look into closing #2104?

[tgriesser]

@caseywebdev - don't worry about reverting this one just yet. It's actually an issue with #2003 - which actually was a regression because it is actually preferable that the success & error follow the jquery ajax signature for the success & error functions. (edit: not sure about that actually)

[jashkenas]

Slow down a bit folks -- please don't merge stuff like this ;)

Looking at the diff, it confuses and tangles the logic of the code a little bit. And as was discovered later, the micro-benchmark was totally spurious (as micro-benchmarks often are). Unless it's in the hottest of hot loops, there's not going to be a place in Backbone where truncating and reusing an array with .length versus making a new one is ever going to make a noticeable difference.

I'll revert.

[akre54]

@jashkenas in their defense (and mine) it was a pretty hot speed boost (50x in best case). It was only due to the detective work by Casey that it was clear this improvement was in error. Definitely agreed about making code look uglier.

[jashkenas]

Certainly -- and no worries, I appreciate it.

The point I'm trying to make is just that we've been burned in Backbone many times in the past by compelling-looking JSPerf tests that don't actually have a meaningful effect on an application's performance. Even if it is a 50x speed up for a particular little piece, that little piece may not have a noticeable effect, or a meaningless one at runtime.

For example, imagine you optimized the collection.fetch() method to be 100x as fast as it is currently. Worth merging, right? Wrong. In a real app, you're never going to want to call fetch any faster than your server can respond to the remote request ... within reasonable limits, it doesn't matter in the slightest how slow it is.

[webxl]

Yep, paginator broken on v0.9.10: http://jsfiddle.net/webxl/7pSx5/

Looks like it's fixed on master now: http://jsfiddle.net/webxl/nJbHF/

Thanks!

[braddunbar]

So, the issue is caused by the fact that reset previously created a separate array instead of truncating the existing one, something that Backbone Paginator depends upon. It seems that we should add a test to prevent changing this behavior by accident in the future.