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

perf($rootScope): make postDigestQueue more efficient #14545

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
16 changes: 12 additions & 4 deletions src/ng/rootScope.js
Original file line number Diff line number Diff line change
Expand Up @@ -769,15 +769,19 @@ function $RootScopeProvider() {
dirty = false;
current = target;

while (asyncQueue.length) {
// It's safe for asyncQueuePosition to be a local variable here because this loop can't
// be reentered recursively. Calling $digest from a function passed to $applyAsync would
// lead to a '$digest already in progress' error.
for (var asyncQueuePosition = 0; asyncQueuePosition < asyncQueue.length; asyncQueuePosition++) {
try {
asyncTask = asyncQueue.shift();
asyncTask = asyncQueue[asyncQueuePosition];
asyncTask.scope.$eval(asyncTask.expression, asyncTask.locals);
} catch (e) {
$exceptionHandler(e);
}
lastDirtyWatch = null;
}
asyncQueue.length = 0;

traverseScopesLoop:
do { // "traverse the scopes" loop
Expand Down Expand Up @@ -848,13 +852,15 @@ function $RootScopeProvider() {

clearPhase();

while (postDigestQueue.length) {
// postDigestQueuePosition isn't local here because this loop can be reentered recursively.
while (postDigestQueuePosition < postDigestQueue.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep postDigestQueuePosition local and use a for loop as with asyncQueue above ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above. This loop can be reentered recursively.

Copy link
Member

Choose a reason for hiding this comment

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

You are right. Which makes me wonder: Could the same happen with the asyncQueue ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No - the asyncQueue is drained during the $digest cycle where postDigestQueue is drained outside of it. As a result, you can't recurse into it since you'd have gotten a digest in progress error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added these explanations as comments in the code.

try {
postDigestQueue.shift()();
postDigestQueue[postDigestQueuePosition++]();
} catch (e) {
$exceptionHandler(e);
}
}
postDigestQueue.length = postDigestQueuePosition = 0;
},


Expand Down Expand Up @@ -1309,6 +1315,8 @@ function $RootScopeProvider() {
var postDigestQueue = $rootScope.$$postDigestQueue = [];
var applyAsyncQueue = $rootScope.$$applyAsyncQueue = [];

var postDigestQueuePosition = 0;

return $rootScope;


Expand Down
126 changes: 86 additions & 40 deletions test/ng/rootScopeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -1320,46 +1320,6 @@ describe('Scope', function() {
expect(externalWatchCount).toEqual(0);
}));

it('should run a $$postDigest call on all child scopes when a parent scope is digested', inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new(),
count = 0;

$rootScope.$$postDigest(function() {
count++;
});

parent.$$postDigest(function() {
count++;
});

child.$$postDigest(function() {
count++;
});

expect(count).toBe(0);
$rootScope.$digest();
expect(count).toBe(3);
}));

it('should run a $$postDigest call even if the child scope is isolated', inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new(true),
signature = '';

parent.$$postDigest(function() {
signature += 'A';
});

child.$$postDigest(function() {
signature += 'B';
});

expect(signature).toBe('');
$rootScope.$digest();
expect(signature).toBe('AB');
}));

it('should cause a $digest rerun', inject(function($rootScope) {
$rootScope.log = '';
$rootScope.value = 0;
Expand Down Expand Up @@ -1705,6 +1665,92 @@ describe('Scope', function() {
}));
});

describe('$$postDigest', function() {
it('should process callbacks as a queue (FIFO) when the scope is digested', inject(function($rootScope) {
var signature = '';

$rootScope.$$postDigest(function() {
signature += 'A';
$rootScope.$$postDigest(function() {
signature += 'D';
});
});

$rootScope.$$postDigest(function() {
signature += 'B';
});

$rootScope.$$postDigest(function() {
signature += 'C';
});

expect(signature).toBe('');
$rootScope.$digest();
expect(signature).toBe('ABCD');
}));

it('should support $apply calls nested in $$postDigest callbacks', inject(function($rootScope) {
var signature = '';

$rootScope.$$postDigest(function() {
signature += 'A';
});

$rootScope.$$postDigest(function() {
signature += 'B';
$rootScope.$apply();
signature += 'D';
});

$rootScope.$$postDigest(function() {
signature += 'C';
});

expect(signature).toBe('');
$rootScope.$digest();
expect(signature).toBe('ABCD');
}));

it('should run a $$postDigest call on all child scopes when a parent scope is digested', inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new(),
count = 0;

$rootScope.$$postDigest(function() {
count++;
});

parent.$$postDigest(function() {
count++;
});

child.$$postDigest(function() {
count++;
});

expect(count).toBe(0);
$rootScope.$digest();
expect(count).toBe(3);
}));

it('should run a $$postDigest call even if the child scope is isolated', inject(function($rootScope) {
var parent = $rootScope.$new(),
child = parent.$new(true),
signature = '';

parent.$$postDigest(function() {
signature += 'A';
});

child.$$postDigest(function() {
signature += 'B';
});

expect(signature).toBe('');
$rootScope.$digest();
expect(signature).toBe('AB');
}));
});

describe('events', function() {

Expand Down