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.

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

CollectionView.reset performance #283

Closed
zakjan opened this issue Oct 12, 2012 · 13 comments
Closed

CollectionView.reset performance #283

zakjan opened this issue Oct 12, 2012 · 13 comments
Assignees

Comments

@zakjan
Copy link

zakjan commented Oct 12, 2012

I have collection with 100+ models, which are updating every few seconds, and they has to be resorted. In this case, CollectionView.reset has fairly poor performance in older browsers like IE8. Browser eats a lot of CPU time and can even freeze.

Please see this article for possible solution using native DocumentFragment for rendering view.
Can Marionette be updated to use it?

@mxriverlynn
Copy link
Member

+1 to the idea. I'll get some perf tests set up in Marionette for this, and see what kind of improvements we can get.

@jkirkwood
Copy link

I recently modified CollectionView to use document fragments in one of my projects. I haven't quantified the performance gains, but the improvement is definitely noticeable.

Basically I just modified the showCollection and renderItemView as follows:

showCollection: function(){
  var that = this;
  var ItemView = this.getItemView();
  this._frag = document.createDocumentFragment();
  this.collection.each(function(item, index){
    that.addItemView(item, ItemView, index);
  });
  var itemView = {};
  itemView.el = this._frag;
  this.appendHtml(this, itemView, 0);
  delete this._frag;
}
renderItemView: function(view, index) {
  view.render();
  if (this._frag) {
    this._frag.appendChild(view.el);
  } else {
    this.appendHtml(this, view, index);
  }    
}

@ghost ghost assigned mxriverlynn Oct 31, 2012
@mxriverlynn
Copy link
Member

FYI - i started digging in to this tonight. While the change may appear to be simple on the surface, there are a lot of nuances that throwing it for a loop. It's going to take a major re-engineering effort for the CollectionView to support this and not break any of the existing behaviors - even though it will break many of the existing API calls.

I'll continue to dig in to this and see what it takes to get it done. I still need to set up performance tests for this, though, to see if it's worth it or not.

@mxriverlynn
Copy link
Member

After some more effort, I have a working version of this in the docFrag branch now.

Things I've broken are generally limited to the appendHtml function on CollectionView and CompositeView, and the showEmptyView and showCollectionView on the same.

All of the specs are still passing, though, after updating a few of them for the breaking changes.

If anyone that is interested in this has time, please grab a build from the docFrag branch and let me know if it's working for you: https://github.com/marionettejs/backbone.marionette/tree/docFrag

If you happen to be overriding appendHtml for your composite / collection views, note the signature change:

appendHtml: function($el, itemView, index){
  $el.append(itemView.$el);
}

where $el is the jQuery element that you should append your itemView.$el in to. This is different from the previous signature which passed the collectionView or compositeView itself in as the first parameter.


Note that I still need to put together performance specs for this, to see if this really is giving us an improvement.

@mxriverlynn
Copy link
Member

FYI - the tests I put together for this are showing the document fragment version being slower, at this point. Either I'm doing something wrong w/ my document fragments, or this isn't worth it.

I'm still digging in to this, and I think I'm going to change the perf test tool I'm using... we'll see how this turns out.

@molily
Copy link
Contributor

molily commented Nov 11, 2012

The main performance improvement should come from performing the append operations off-DOM (regardless whether a DOMFragment or a normal element is used) instead of touching the DOM for every item view.

If a benchmark doesn’t show an improvement, the browser may probably optimize the rendering. If some piece of code is changing the DOM every few milliseconds, like adding and removing nodes in a tight loop, the browser presumably doesn’t layout and paint the elements with more than 60 fps.

@molily
Copy link
Contributor

molily commented Nov 11, 2012

As a side note, there are several ways to optimize collection view performance. Optimization already starts in the collection itself.

In Chaplin we don’t remove all and create new item views on reset, but reuse existing views if the model cid is the same (this could also be the model id or another criterion).

In addition, collections can be updated instead of reset()ted so existing models are updated (firing change events), new models are created (add) and old models are removed (remove).

This improved the performance a lot because most of the time when a collection is updated during application runtime, just a few models are added to the top or bottom, some model attributes change or the model order changes. In general I try to avoid classic collection resets with fresh models to allow for these optimizations.

@minglecm
Copy link
Contributor

@derickbailey what was the outcome of this or was it given up on?

@diasjorge
Copy link

Any updates on this issue? We're facing a similar problem when having views that support infinite scroll. As the user paginates more and more it gets slower.

@swamyg
Copy link

swamyg commented Aug 9, 2013

The performance of showCollection is very painfully slow in IE8. I tried implementing @jkirkwood's docFrag snippet but it doesn't seem to improve the performance. Plus the code is a bit changed since his implementation.

@derickbailey Can you please let us know if you have a lead on how to deal with this problem?

@bobbyrenwick
Copy link

👍 What's the state of this?

@samccone
Copy link
Member

samccone commented Jan 8, 2014

ah this is in the current release @bobbyrenwick !

@samccone samccone closed this as completed Jan 8, 2014
@zakjan
Copy link
Author

zakjan commented Jan 10, 2014

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants