jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.38k forks source link

Subcollection memory leak (single model, multiple collections) #3858

Closed TalonTR closed 8 years ago

TalonTR commented 8 years ago

When adding a single Backbone.Model to multiple Backbone.Collections, each collection adds its own event listener to the added model (in _addReference). This reference prevents any collection to be garbage collected until every single model is explicitly removed from this collection.

Our application depends on creating many temporary collections (from grouping or filtering results) to re-use extended collection methods on these results, containing already existing models from base collections. We'd like to see these temporary collections garbage collected, once their reference gets discarded (via delete or leaving the scope). Instead we're aggregating references to every single temporary collection within every model, preventing them from ever getting discarded as long as the model is alive.

Solving this problem by calling .reset() on every temporary collection and thereby removing the event listeners would be a major inconvenience. Is there any BB concept enabling us to use temporary backbone collections without forcing us to cleanup by ourselves?

Here is a jsFiddle demonstrating the issue: http://jsfiddle.net/yf2t8w98/1/ Watching the debugger timeline while running the loop you'll see something like that:

bb1

jridgewell commented 8 years ago

Hi @TalonTR, you're correct this will happen and it's on purpose. Models are part of the Collection, so you can't clean up temporary models until you remove their references from the model — how else would you use the models from the collection.

huehnerhose commented 8 years ago

As far as i understand this, a model holds the collection reference primarly for event-binding. Since a collection offers you many options that are not connected to event handling, an option to create a collection which is event-unaware would be a great feature. That way you can create one-time-usage collections for sorting, bucketing and so with all the bells and whistles collection can offer, but without a steady increase in memory usage caused by non-garbage-collectable collections

jridgewell commented 8 years ago

You're looking at this from the wrong perspective. The model event binding isn't the issue here, it's the collection. The collection has a reference to all models in it (collection.models is an array of the models).

The memory buildup you're seeing is because you keep adding models to a collection, so they can't be garbage collected until you empty the collection (because of collection.models). You can either throw away the collection instance, or throw away the collection -> model references.

TalonTR commented 8 years ago

Thanks @jridgewell for your answer. Unfortunately this doesn't provide the insight i was looking for. A non empty collection.models array is no reason for a collection not to get garbage collected, if there's no reference left:

var single_collection = new Backbone.Collection();
for (var i = 0; i < 10000; i++) {
    single_collection.add( {} );
}
delete single_collection;

The last line removes the last existing reference to _singlecollection , which then gets garbage collected, along with all its models. Thats expected behavior.

Can you do me the favor and take a second look at the fiddle? What we're doing is adding the same model to many collections, which then instantly get discarded:

for (var i = 0; i < 10000; i++) {

    var tmp_collection = new Backbone.Collection();
    tmp_collection.add(single_model);

    delete tmp_collection;
}

Since _singlemodel is already a Backbone.Model, there is at no time more then one model within our heap.

After the delete command there is no reference to _tmpcollection left, leaving it to be removed at the next garbage collection cycle - even if the collections .model array is not emtpy at that point.

The memory buildup we see is because none of those _tmpcollection collections is ever really discarded, because they all add themselves to single_model.events - hence the console.log(single_model._events); at the end of the fiddle, which outputs:

Object {all: Array[10000]}

Even without any variable pointing to any tmp_collection ever created the garbage collector can't discard these collections because of those _events entries.

Thanks in advance for your patience.

akre54 commented 8 years ago

I'm not sure Backbone is the right library for your use case. Try a library like Crossfilter if you need to be doing a lot of aggregation and filtering.

TalonTR commented 8 years ago

Thanks @akre54 for the tip, but we're not really filtering massive datasets and are otherwise very happy with Backbone. The whole problem arose because we started to re-wrap our collection function results (filter, where, ...) in collections again

reWrapFilter: function(predicate, context) {
    return new this.constructor( this.filter(predicate, context) );
}  

returning the filter result as a new collection instance of the same class. This enables function chaining as well as using all Backbone collection functions and our extended collection functions:

var ids = single_collection.reWrapFilter(filterFunction).reWrapWhere({x: 2}).pluck("id");

This is a great way to work with Backbone Collections, assuming all intermediate collections get discarded if the scope is left. Instead Backbone preserves them as event references within any matching model. A simple solution would be to prevent intermediate collections from attaching event listeners like @huehnerhose proposed.

jridgewell commented 8 years ago

Ah, I understand now.

A simple solution would be to prevent intermediate collections from attaching event listeners like @huehnerhose proposed.

You can try overriding #_addReference to do nothing, but it's a "private" method and isn't guaranteed to stay the same.

TalonTR commented 8 years ago

Thats more or less exactly what we do at the moment (removing all added collection listeners in case its a re-wrapped collection) but that might break with any next Backbone update.

    _disableModelEventListenerFlag: false,
    _addReference: function(model, options) {
        this._originalAddReference.call(this, model, options);

        if (this._disableModelEventListenerFlag) {
            model.off('all', this._onModelEvent, this);
        }
    },

We were surprised to find that Backbone doesn't offer anything like Underscores .chain() method, which would be a cool feature. Unfortunately the fact that a collection registers itself in all its models prevents us from creating a good concept (or even a PR) providing a true Backbone.Collection.chain().

There were some discussions about single model - multi collection relationships in the past (e.g. #1161) and it occurs to me that Backbone isn't clear about how it handles these.