Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

perf(Scope): limit propagation of $broadcast #5371

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 29 additions & 4 deletions src/ng/rootScope.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ function $RootScopeProvider(){
this.$$asyncQueue = [];
this.$$postDigestQueue = [];
this.$$listeners = {};
this.$$listenerCount = {};
this.$$isolateBindings = {};
}

Expand Down Expand Up @@ -192,6 +193,7 @@ function $RootScopeProvider(){
}
child['this'] = child;
child.$$listeners = {};
child.$$listenerCount = {};
child.$parent = this;
child.$$watchers = child.$$nextSibling = child.$$childHead = child.$$childTail = null;
child.$$prevSibling = this.$$childTail;
Expand Down Expand Up @@ -696,6 +698,8 @@ function $RootScopeProvider(){
this.$$destroyed = true;
if (this === $rootScope) return;

forEach(this.$$listenerCount, bind(null, decrementListenerCount, this));

if (parent.$$childHead == this) parent.$$childHead = this.$$nextSibling;
if (parent.$$childTail == this) parent.$$childTail = this.$$prevSibling;
if (this.$$prevSibling) this.$$prevSibling.$$nextSibling = this.$$nextSibling;
Expand Down Expand Up @@ -885,8 +889,18 @@ function $RootScopeProvider(){
}
namedListeners.push(listener);

var current = this;
do {
if (!current.$$listenerCount[name]) {
current.$$listenerCount[name] = 0;
}
current.$$listenerCount[name]++;
} while ((current = current.$parent));

var self = this;
return function() {
namedListeners[indexOf(namedListeners, listener)] = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to decrement the counters here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

decrementListenerCount(self, 1, name);
};
},

Expand Down Expand Up @@ -998,8 +1012,7 @@ function $RootScopeProvider(){
listeners, i, length;

//down while you can, then up and next sibling or up and next sibling until back at root
do {
current = next;
while ((current = next)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which change do-while to while? it doesn't seem to make a difference and it causes the code to go out of sync with the traversal code in digest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It struck me as redundant and weird:

do {
current = next;
...
} while ((current = next));

is identical to (or perhaps a very tiny bit slower than) the simpler:

while ((current = next)) {
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for that matter, they were already out of sync in this respect. The similar loop in $digest is:

do {
// current = next is not here.
} while ((current = next))

event.currentScope = current;
listeners = current.$$listeners[name] || [];
for (i=0, length = listeners.length; i<length; i++) {
Expand All @@ -1021,12 +1034,14 @@ function $RootScopeProvider(){
// Insanity Warning: scope depth-first traversal
// yes, this code is a bit crazy, but it works and we have tests to prove it!
// this piece should be kept in sync with the traversal in $digest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should update the comment to reflect that this version is now slightly different because of listenerCount short-circuiting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (!(next = (current.$$childHead || (current !== target && current.$$nextSibling)))) {
// (though it differs due to having the extra check for $$listenerCount)
if (!(next = ((current.$$listenerCount[name] && current.$$childHead) ||
(current !== target && current.$$nextSibling)))) {
while(current !== target && !(next = current.$$nextSibling)) {
current = current.$parent;
}
}
} while ((current = next));
}

return event;
}
Expand Down Expand Up @@ -1055,6 +1070,16 @@ function $RootScopeProvider(){
return fn;
}

function decrementListenerCount(current, count, name) {
do {
current.$$listenerCount[name] -= count;

if (current.$$listenerCount[name] === 0) {
delete current.$$listenerCount[name];
}
} while ((current = current.$parent));
}

/**
* function used as an initial value for watchers.
* because it's unique we can easily tell it apart from other values
Expand Down
59 changes: 59 additions & 0 deletions test/ng/rootScopeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,27 @@ describe('Scope', function() {
first.$apply();
expect(log).toBe('1232323');
}));

it('should decrement anscestor $$listenerCount entries', inject(function($rootScope) {
var EVENT = 'fooEvent',
spy = jasmine.createSpy('listener'),
firstSecond = first.$new();

firstSecond.$on(EVENT, spy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please register two listeners for this event so that we test that we decrement the counters by the right amount (not just by one)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

firstSecond.$on(EVENT, spy);
middle.$on(EVENT, spy);

expect($rootScope.$$listenerCount[EVENT]).toBe(3);
expect(first.$$listenerCount[EVENT]).toBe(2);

firstSecond.$destroy();

expect($rootScope.$$listenerCount[EVENT]).toBe(1);
expect(first.$$listenerCount[EVENT]).toBeUndefined();

$rootScope.$broadcast(EVENT);
expect(spy.callCount).toBe(1);
}));
});


Expand Down Expand Up @@ -1107,13 +1128,34 @@ describe('Scope', function() {
child.$emit('abc');
child.$broadcast('abc');
expect(log).toEqual('XX');
expect($rootScope.$$listenerCount['abc']).toBe(1);

log = '';
listenerRemove();
child.$emit('abc');
child.$broadcast('abc');
expect(log).toEqual('');
expect($rootScope.$$listenerCount['abc']).toBeUndefined();
}));


it('should increment ancestor $$listenerCount entries', inject(function($rootScope) {
var child1 = $rootScope.$new(),
child2 = child1.$new(),
spy = jasmine.createSpy();

$rootScope.$on('event1', spy);
expect($rootScope.$$listenerCount).toEqual({event1: 1});

child1.$on('event1', spy);
expect($rootScope.$$listenerCount).toEqual({event1: 2});
expect(child1.$$listenerCount).toEqual({event1: 1});

child2.$on('event2', spy);
expect($rootScope.$$listenerCount).toEqual({event1: 2, event2: 1});
expect(child1.$$listenerCount).toEqual({event1: 1, event2: 1});
expect(child2.$$listenerCount).toEqual({event2: 1});
}))
});


Expand Down Expand Up @@ -1358,6 +1400,23 @@ describe('Scope', function() {
$rootScope.$broadcast('fooEvent');
expect(log).toBe('');
}));


it('should not descend past scopes with a $$listerCount of 0 or undefined',
inject(function($rootScope) {
var EVENT = 'fooEvent',
spy = jasmine.createSpy('listener');

// Precondition: There should be no listeners for fooEvent.
expect($rootScope.$$listenerCount[EVENT]).toBeUndefined();

// Add a spy listener to a child scope.
$rootScope.$$childHead.$$listeners[EVENT] = [spy];

// $rootScope's count for 'fooEvent' is undefined, so spy should not be called.
$rootScope.$broadcast(EVENT);
expect(spy).not.toHaveBeenCalled();
}));


it('should return event object', function() {
Expand Down