Bindings need to be able to destroy themselves. #844

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
3 participants
Owner

dcporter commented Oct 2, 2012

This solves a major, pervasive source of memory leaks in the framework.

@dcporter dcporter referenced this pull request Oct 8, 2012

Closed

Memory Leaks #848

Contributor

fairbanksg commented Oct 10, 2012

We have been working independently on some memory leak fixes. Your latest commit looks like you are about to write code almost identical to what we have, so you may want to hold off so work is not duplicated. I should be opening a pull request very soon.

Owner

dcporter commented Oct 10, 2012

Thanks Greg. I don't have any further additions to destroyObserverable planned at this time. I've covered bindings, but properties and observers are still open items.

My remaining memory leak plans for the time being include my other pull request on View#owner, and some work on SC.ScrollView. Does that overlap with anything you're working on?

Contributor

fairbanksg commented Oct 10, 2012

The view pull request does, but your code covers that better, so I'll just remove that from my pull request.

@@ -692,6 +692,8 @@ SC.mixin(/** @scope window.SC.prototype */ {
@returns {Array} array with [object, property] if found or null
*/
tupleForPropertyPath: function(path, root) {
+ // if passed nothing, return nothing.
+ if (SC.none(path)) return null;
@publickeating

publickeating Oct 29, 2012

Owner

Why is this in this pull request? Is it part of the problem or an unrelated issue that came up?

@dcporter

dcporter Oct 29, 2012

Owner

This was a related issue – destroyed bindings were sending their nulled-out property paths through this function one last time, and it was throwing an error because it wasn't properly gated. I suspect that there is a solution that involves preventing destroyed bindings from doing anything, but I couldn't track down whatever queue they were still part of, and nulling out their property paths and fixing this method to not throw up on nulls worked.

+// toObject.destroy();
+// ok(binding.isDestroyed, "destroying a view destroys its bindings.");
+//});
+
@publickeating

publickeating Oct 29, 2012

Owner

I really prefer not to bring "TODO"'s into the framework, what else needs to be done to fix this?

@dcporter

dcporter Oct 29, 2012

Owner

I forgot about this one... let me take another look.

@dcporter

dcporter Oct 29, 2012

Owner
  1. Create failing unit test.
  2. Get annoyed with trying to pass it; comment it out.
  3. Later, fix code.
  4. Forget to uncomment the unit test, which now passes.

New commit coming shortly...

dcporter and others added some commits Oct 29, 2012

Removes unused internal property of SC.Binding:_changePending. This p…
…roperty gets assigned and never read other than in some unit tests which have been upgraded to check for the real indication of when a change is pending.
Fixes small memory leak in SC.ObserverSet.
Each time that a new observer is added to an object the ObserverSet for the object will add a tracking hash for the target and the method.  As more methods are tracked for the target, they are added and as methods are no longer tracked for the target, they are removed.  However, even when no methods are tracked for the target, the tracking hash for the target still exists.  This creates a small leak of memory, because the target may have been destroyed and freed, but we are still maintaining an empty tracking hash for it.

Here's an example that would create 3,000 empty hashes in coreOb._kvo_observers_key._members:

    obs = [];
    window.coreOb = SC.Object.create({ key: '1' });
    SC.run(function() {
      for (var i = 2999; i >= 0; i--) {
        obs.push(SC.Object.create({ myKeyBinding: SC.Binding.from('coreOb.key') }));
      }
    });

    SC.run(function() {
      for (var i = obs.length - 1; i >= 0; i--) {
        obs.pop().destroy();
      }
    });
Fixes memory leak in SC.Set.
The way that SC.Set removes objects is to "untrack" the internal index of the object by shrinking its length, but it never actually removed the object at the last index.  The only way that the object could be freed is if a new object is inserted at the same internal index, thus replacing it.  If the objects were removed in the reverse order that they were added, every object would still be in the set (until they were possibly overwritten).
Extends SC.Observable:destroyObservable to also remove observers.
- Previously the object would remain registered in other object's ObserverSets and could not be freed.
- Adds tests for destroyObservable behavior.

@dcporter dcporter closed this Oct 30, 2012

@dcporter dcporter referenced this pull request May 30, 2013

Closed

Strange Binding Behavior #987

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment