Fix listenTo memory leak #3455

jridgewell · 2015-01-22T21:53:06Z

Fixes #3453, and competes with #3049.

I think this aligns more closely with what's currently implemented. 😄

It also implements listenApi, which is eventsApi translated to the listenTo / stopListening method signatures.

megawac · 2015-01-22T21:59:38Z

test/events.js

@@ -79,7 +79,7 @@
  test("listenTo and stopListening with event maps", 4, function() {
    var a = _.extend({}, Backbone.Events);
    var b = _.extend({}, Backbone.Events);
-    var cb = function(){ ok(true); };
+    var cb = function(){ console.log('called'); ok(true); };


Fixes jashkenas#3453

http://jsperf.com/eventsapi-refactor

If you trigger with an object, it's prop value is prepended onto the listener's arguments.

jridgewell · 2015-01-26T20:41:22Z

As a side: I have a completed eventsApi refactor that cleans up the duplication in listenApi. I haven't included it in this PR yet, since it might be polarizing. If the only issue with this PR is the duplication, I'd be glad to put it in as well.

jashkenas · 2015-01-26T20:52:38Z

Cleaning up the duplication would be lovely.

jridgewell · 2015-01-26T21:07:44Z

Ok, it's included now.

To be clear, I think the trigger with object behavior is silly. But I needed tests to ensure that my [`eventsApi` refactor] (https://github.com/jridgewell/backbone/tree/eventsApi-refactor) kept the current functionality.

akre54 · 2015-01-27T03:43:23Z

backbone.js

+  // Maps the normalized event callbacks into onceWrappers.
+  var onceMap = function(name, callback, offer) {
+    return eventsApi(function(map, name, callback, offer) {
+      if (callback) map[name] = onceWrap(name, callback, offer);


Any reason not to inline onceWrap here?

Nope. I'll fix it.

jridgewell · 2015-01-27T05:45:13Z

Ping @akre54: I commented, and added a triggerSentinel object to prevent further confusion. Let me know you have any other suggestions.

akre54 · 2015-01-27T15:35:20Z

backbone.js

-      if (events) triggerEvents(events, args);
-      if (allEvents) triggerEvents(allEvents, arguments);
+
+      // Use `eventsApi` to normalize `name` into the proper event names.


Mind clarifying this? I'm not sure what this is getting at.

@jridgewell the comments are still lacking. Can you do a pass to make sure the logic is clear? I'm finding it hard to follow what is happening and why.

I haven't revised the comments yet, was trying to solve the off problem first.

Is there a particular spot you're not able to follow?

This line in particular isn't clear what it does, but the bits with triggerSentinel and the numerous internal functions mostly.

It almost seems like we should drop the Events = {on: ..., off: ..., listenTo: ...} object, and just colocate the different events methods (Events.on = ...; onApi = ...; Events.off = ...;.) To follow this logic, I'm having to jump back and forth through the source to even understand where the different pieces are going.

There's so much extra overhead and indirection going on here, it really detracts from the readability of the Events code.

I agree the source could be better arranged, I'll work on that tonight along with the comments.

There's so much extra overhead and indirection going on here

I actually thought it was kind of easier to follow without the old eventsApi behavior, but then I had to account for off cleaning up any listeners. It makes it easier in my mind to think of all the *Api functions as special purpose reduce iteratees, with eventsApi being _.reduce.

akre54 · 2015-01-27T15:36:11Z

I think a little more of the "why" and the "how" and less "what" in the comments might be helpful.

Maybe a sentence or two at the top briefly describing how the many abstracted functions work. Right now I'm on the fence that this consolidation is worth the added indirection. It definitely makes it harder to follow the logic.

Can you also make a jsperf showing the extra functions aren't too horrible and squash to a single commit and I'll merge? Thanks for your hard work on this. (And no stress about getting it done today... enjoy the snowday!)

jridgewell · 2015-01-27T15:42:05Z

Here's a simple jsperf: http://jsperf.com/eventsapi-refactor (It was hidden in 50adef3's commit message). Safari seems to be the only one taking a hit, but it's still very fast.

akre54 · 2015-01-27T15:45:17Z

how about vs master? That's showing all the changes with the extra
callbacks added in.

On Tue Jan 27 2015 at 10:42:24 AM Justin Ridgewell notifications@github.com
wrote:

Here's a simple jsperf: http://jsperf.com/eventsapi-refactor (It was
hidden in 50adef3
50adef3c03841038a706427591cfeccc1ad280d8's
commit message). Safari seems to be the only one taking a hit, but it's
still very fast.

—
Reply to this email directly or view it on GitHub
#3455 (comment).

jridgewell · 2015-01-27T15:54:48Z

Here's the simple case against master, and another using space-separated events.

I did the original tests against the listenTo-memory-leak-immutable-on-events branch because that's the one with the minimum code to fix the memory leak. Comparing against master seemed apples-to-oranges.

jridgewell · 2015-01-27T16:14:13Z

Actually, the performance hit seems to be coming from #3463, which is also merged into eventsApi-refactor. The simple case against those two have the exact same speeds.

melnikov-s · 2015-01-27T16:28:50Z

Nice, but does not seem to compete with #3049 as it does not address the following:

var a = _.extend({}, Backbone.Events);
var b = _.extend({}, Backbone.Events);
var c = _.extend({}, Backbone.Events);

a.listenTo(c, 'x', fn);
b.listenTo(c, 'x', fn);
c.off('x');
c = null; //c can't be garbage collected until 'stopListening()' is called on a and b

Here are the relevant tests

 test("off removes object from listeners", 6, function() {                                                                                                                                                                                    
    var a = _.extend({}, Backbone.Events);
    var b = _.extend({}, Backbone.Events);
    var c = _.extend({}, Backbone.Events);
    var d = _.extend({}, Backbone.Events);
    var fn = function() {};    

    b.listenTo(a, 'all', fn);  
    c.listenTo(a, 'all', fn);  
    c.listenTo(d, 'x', fn);    

    equal(_.size(b._listeningTo), 1);
    equal(_.size(c._listeningTo), 2);
    equal(_.size(a._listeners), 2); 
    a.off();                   
    equal(_.size(b._listeningTo), 0);
    equal(_.size(c._listeningTo), 1);
    equal(_.size(a._listeners), 0); 
  }); 

  test("off w/event name removes object from listeners", 8, function() {
    var a = _.extend({}, Backbone.Events);
    var b = _.extend({}, Backbone.Events);
    var c = _.extend({}, Backbone.Events);
    var fn = function() {};

    b.listenTo(a, 'x', fn);
    c.listenTo(a, 'x y', fn);

    equal(_.size(b._listeningTo), 1);
    equal(_.size(c._listeningTo), 1);
    equal(_.size(a._listeners), 2); 
    a.off('x');
    equal(_.size(b._listeningTo), 0);
    equal(_.size(c._listeningTo), 1);
    equal(_.size(a._listeners), 1); 
    a.off('y');
    equal(_.size(c._listeningTo), 0);
    equal(_.size(a._listeners), 0); 
  });

jashkenas#3455 (comment)

jridgewell · 2015-01-29T16:11:28Z

I've addressed @smelnikov's #off memory leak, but the code isn't as pretty as I would have liked.

This is to prevent an infinite off -> stopListening -> off -> stopListening.

megawac · 2015-01-29T17:36:00Z

backbone.js

+        var ids = context != null ? [context._listenId] : _.keys(listeners);
+        for (var i = 0, length = ids.length; i < length; i++) {
+          var listener = listeners[ids[i]];
+          if (!listener) break;


Why is this necessary? I would expect a continue if anything

When you listenTo, context is forced to be this. If we're offing, we need to consider two circumstances:

No context

We'll loop through all our listeners, since any could be affected.

This conditional will never be true, so we'll never break.

Context

Only loop through the listeners of the corresponding _listenId, since only that listener will have the same context

If we have listeners, but not that _listenId, then break.

ids is guaranteed to only equal [context._listenId], so there's no use continueing.

If we have that _listenId, all is good.

jridgewell · 2015-01-30T04:26:39Z

@akre54 Commented and rearranged.

jashkenas · 2015-02-03T19:16:13Z

I think let's maybe merge this for now -- and then go over it and tweak as need be. (But hopefully need not ;)

Fix listenTo memory leak

We should eventually pull in their changes but for a hope in actually getting this fix in production I opted to do this instead for now.

megawac reviewed Jan 22, 2015
View reviewed changes

jridgewell force-pushed the listenTo-memory-leak branch from 944787f to f122de6 Compare January 22, 2015 22:04

jridgewell mentioned this pull request Jan 23, 2015

Add View Info Panel Maluen/Backbone-Debugger#33

Open

jridgewell force-pushed the listenTo-memory-leak branch from da8e79b to eb28f9a Compare January 23, 2015 19:17

jridgewell mentioned this pull request Jan 24, 2015

Trigger with object #3460

Closed

jridgewell added 7 commits January 24, 2015 14:44

Do not alter the "on" events array

c48cf5f

Fix listenTo memory leak

85fb8c3

Fixes jashkenas#3453

Delete events hash if there are none.

0c200ca

Simplify #stopListening

1cb600e

Use faster listeningTo iteration

d7b7a9c

Refactor the eventsApi

50adef3

http://jsperf.com/eventsapi-refactor

Trigger with object behaves dumbly

9abb4ab

If you trigger with an object, it's prop value is prepended onto the listener's arguments.

Test trigger arguments behavior

59c3704

To be clear, I think the trigger with object behavior is silly. But I needed tests to ensure that my [`eventsApi` refactor] (https://github.com/jridgewell/backbone/tree/eventsApi-refactor) kept the current functionality.

jridgewell force-pushed the listenTo-memory-leak branch from 6ad0063 to 59c3704 Compare January 26, 2015 21:55

akre54 reviewed Jan 27, 2015
View reviewed changes

jridgewell added 2 commits January 26, 2015 23:28

Inline onceWrapper

c73a3c3

Comment eventsApi refactor

6796ca0

akre54 reviewed Jan 27, 2015
View reviewed changes

jridgewell added 3 commits January 27, 2015 17:21

Break instead of continuing

e01869d

Address listenTo and off memory leak

778d447

jashkenas#3455 (comment)

off cleans up listenTo

b49f17c

jridgewell force-pushed the listenTo-memory-leak branch from ad3e065 to c7309a3 Compare January 29, 2015 16:06

jridgewell force-pushed the listenTo-memory-leak branch from c7309a3 to f28fca1 Compare January 29, 2015 16:21

Create internal use stopListening

826b110

This is to prevent an infinite off -> stopListening -> off -> stopListening.

jridgewell force-pushed the listenTo-memory-leak branch from f28fca1 to 826b110 Compare January 29, 2015 16:28

megawac reviewed Jan 29, 2015
View reviewed changes

jridgewell added 2 commits January 29, 2015 23:20

Rearrange and comment Events

4018a56

Undo concat #on tests

3aac74d

jridgewell mentioned this pull request Feb 3, 2015

Fix #3448. Ensure listenToOnce splits event names. #3449

Merged

jashkenas added a commit that referenced this pull request Feb 3, 2015

Merge pull request #3455 from jridgewell/listenTo-memory-leak

713c288

Fix listenTo memory leak

jashkenas merged commit 713c288 into jashkenas:master Feb 3, 2015

jashkenas added bug fixed labels Feb 3, 2015

jridgewell mentioned this pull request Feb 5, 2015

Implement Events as a linked list #3476

Closed

jameshartig pushed a commit to mc0/bedrock that referenced this pull request Feb 12, 2015

Much smaller/managable fix for jashkenas/backbone#3455

efdb54e

We should eventually pull in their changes but for a hope in actually getting this fix in production I opted to do this instead for now.

jameshartig pushed a commit to mc0/bedrock that referenced this pull request Feb 12, 2015

Much smaller/managable fix for jashkenas/backbone#3455

5f625d6

We should eventually pull in their changes but for a hope in actually getting this fix in production I opted to do this instead for now.

jridgewell mentioned this pull request Feb 20, 2015

prevent memory leaks with 'stopListening' and 'off' #3049

Closed

jridgewell mentioned this pull request May 2, 2015

Refine eventsApi #3594

Merged

This was referenced May 18, 2015

listenTo change in behavior, 1.2.0? #3611

Closed

Use the public on method when listening #3615

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix listenTo memory leak #3455

Fix listenTo memory leak #3455

jridgewell commented Jan 22, 2015

megawac Jan 22, 2015

jridgewell Jan 22, 2015

jridgewell commented Jan 26, 2015

jashkenas commented Jan 26, 2015

jridgewell commented Jan 26, 2015

akre54 Jan 27, 2015

jridgewell Jan 27, 2015

jridgewell commented Jan 27, 2015

akre54 Jan 27, 2015

akre54 Jan 29, 2015

jridgewell Jan 29, 2015

akre54 Jan 29, 2015

jridgewell Jan 29, 2015

akre54 commented Jan 27, 2015

jridgewell commented Jan 27, 2015

akre54 commented Jan 27, 2015

jridgewell commented Jan 27, 2015

jridgewell commented Jan 27, 2015

melnikov-s commented Jan 27, 2015

jridgewell commented Jan 29, 2015

megawac Jan 29, 2015

jridgewell Jan 29, 2015

jridgewell commented Jan 30, 2015

jashkenas commented Feb 3, 2015

Fix listenTo memory leak #3455

Fix listenTo memory leak #3455

Conversation

jridgewell commented Jan 22, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jan 26, 2015

jashkenas commented Jan 26, 2015

jridgewell commented Jan 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jan 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akre54 commented Jan 27, 2015

jridgewell commented Jan 27, 2015

akre54 commented Jan 27, 2015

jridgewell commented Jan 27, 2015

jridgewell commented Jan 27, 2015

melnikov-s commented Jan 27, 2015

jridgewell commented Jan 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell commented Jan 30, 2015

jashkenas commented Feb 3, 2015