Lazy render list, 10 at a time

[zmbc]

Need to test on Raingardens flavor! I have a feeling there might be other bottlenecks on that flavor, but this at least eliminates one of them.
#451

[goldpbear]

@zmbc @Lukeswart -- I played around with this branch on the raingardens flavor--the scroll-to-bottom feature works well.

I'm starting to think there are at least two culprits behind the raingarden flavor's slowness, though. One is definitely rendering: as models are added to the list view's collection, a PlaceListItemView is instantiated for each one. This happens whether or not we call renderList() lazily (as we do in this PR). I think the current implementation of lazy rendering only lazily renders the itemViewContainer (the list itself), but the individual list items are all still created on page load--one for each raingarden--and so all the raingarden images are still being queued up and fetched.

But another issue (and one that I think has a bigger impact on performance, at least based on some rough testing) is that the list view sorts all places every time the sync event is fired, which I think happens every time a page of place models comes back from the server. You can see the effect that sorting has by commenting out the call to sort() in the onSync() method in the list view, letting the raingardens flavor load completely in the map view, then clicking the list view button. On my laptop, there is a delay of several seconds before the list view is shown as it sorts the places collection--amounting to probably tens of thousands of calculations.

So I think we'll need to both lazily render and lazily sort for performance to improve. One possibly easy way to do this is maintain two collections in the list view: the existing collection (which accumulates references to all place models on the map as they are retrieved from the server), and another "working" collection, to which we could feed batches of 10 or so models as the user scrolls to the bottom of the list view. We would render and sort the working collection only.

I dunno--does that sounds reasonable? Maybe it's kind of sloppy to have multiple collections for the same models lying around.

[goldpbear]

I guess one problem with the above approach is that sorting would no longer act on the full collection. So if you wanted to see, say, the raingarden with the most likes in the whole set, you wouldn't be able to.

There might be other things we can do to speed up sorting. I found this discussion:
jashkenas/backbone#3242

It suggests passing a {sort: false} option to collection.add(). If you pass that option in the list view's addModel method, like so: this.collection.add(model, {sort:false});, you get a performance boost. On my laptop, the map loaded almost 10 seconds faster.

It seems that collections with a comparator function defined will sort themselves after each model is added by default, unless {sort: false} is passed: http://backbonejs.org/#Collection-sort. So maybe we are actually doubling up on sorting, by also calling sort in the onSync method?

[zmbc]

Because I don't really know enough to rip out Marionette completely, I went for the more moderate approach, similar to the one you mentioned (using 2 different collections). Code reviews very much welcome. I don't think I broke anything about the list view.

As far as the two goals of this PR:

• Image loading --- Images now only load when they are visible for the first time.
• JS Performance --- The raingardens flavor now finishes loading all raingardens in about half the time, at least on my machine.

I think this PR is feature-complete (the list is now lazy-rendered), but I am sure there are more raingardens performance improvements to be made, including the sort-related one @goldpbear mentioned.

[goldpbear]

@zmbc -- this looks great! I was actually thinking that the two-collection approach might be toast because I wasn't sure how you'd manage sorting on the full collection, but I think I see how you got around that issue: passing models between the two collections as needed to facilitate sorting and searching. Nice.

There's a very noticeable performance boost on my laptop. I find that if I also pass a {sort: false} option to this.collection.add() and this.unrenderedItems.add(), as well as take out the call to this.sort() in onSync(), things get faster still.

@Lukeswart -- what do you think about this two-collection approach? I think in the past you mentioned that having models in multiple collections could be dicey, but it seems to me that if we keep everything isolated in the list view it should be ok?

[modulitos]

@zmbc Nice work! This looks great. I think we should merge this in soon. This approach using multiple collections seems reasonable.

@goldpbear Thanks for looking into the sorting aspects of this issue. After a few performance tests, it looks like it saves ~15 seconds of script time, so I think we should go for it!

The downside is that the rendered list won't stay sorted as places are updated or added in real time. But I think it's worth the tradeoff because the performance gain is very significant when we have thousands of places, and the user can always close/re-open the list view to view a re-sorted list.

Once this issue is addressed, I suspect that the remaining performance issues are due to #264. I've attached some metrics on the raingardens flavor if anyone is interested:

sorting on place add/sync:

sorting on place list render:

[goldpbear]

I'm wondering if we want to merge the models in this.collection into this.unrenderedItems (similar to what happens below on lines 257-258) here before sorting, then just sort this.unrenderedItems. If not, aren't we sorting the two collections separately, and, when we merge them on lines 257-258, then iterating through a collection that isn't completely sorted?

[goldpbear]

Oh, actually it might not matter--I think that since the default behavior of .add() is to sort models as they are added to a collection, the call to this.unrenderedItems.add(this.collection.models) on line 257 should be sorting unrenderedItems. But in that case the call to this.collection.sort() on line 235 might be unnecessary?

[goldpbear]

FYI, there seems to be an issue on this branch where certain list view items refuse to render their support views (the "like" buttons that appear at right). Some will render, and some won't. As you scroll down through the list and lazily load more items, previously un-rendered support views will suddenly appear, but the occasional newly-rendered list item will be missing its support view.

I think I found a solution to this: call reset() on the marionettePlaceListItemView region that manages the support views (see this PR: #508). But I can't say why exactly this works, or what exactly causes the problem in the first place.