perf($rootScope): make postDigestQueue more efficient

[thorn0]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Performance improvement

What is the current behavior? (You can also link to an open issue here)
#14534

What is the new behavior (if this is a feature change)?

Using this implementation of queue instead of an array.

Does this PR introduce a breaking change?

postDigestQueue is available as $rootScope.$$postDigestQueue. So it can be considered a breaking change.

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)

[gkalpak]

This looks pretty good to me. I think they tiny extra bytes are totally worth it if we can get a speed boost on every $digest.

OOC, have you done any benchmarks ?

[thorn0]

There is a benchmark on the page from where I took the implementation of queue.

[jbedard]

Since postDigestQueue is only used in one location and it is emptied each and every time... why not just loop through it and then clear it once at the end?

for (var i=0, len=postDigestQueue.length; i<len; i++) {
   try {
      postDigestQueue[i]();
   } catch (e) {
       $exceptionHandler(e);
   }
}
postDigestQueue.length = 0;

That seems simpler and does only one .length = 0 instead of .slice(offset) log(n) times?

[thorn0]

Because this loop can be called recursively inside postDigestQueue[i]();.

[jbedard]

Ah, no wonder that simple solution was never used ^_^

[lgalfaso]

This change looks interesting, but the code at http://code.stephenmorley.org/javascript/queues/ is not MIT licensed, so it cannot be merged into Angular.

[thorn0]

On the other hand, it's quite difficult to implement queue in such a way that it wouldn't resemble and look like a work derived from this CC0 licensed code.

[thorn0]

@lgalfaso I rewrote it a bit. Please have a look. Now slice isn't called unless the underlying array is longer than 200, which makes it even faster because previously slice was called too often for short queues.

[gkalpak]

Similar to what @jbedard proposed, couldn't we just empty the queue when it's empty ?
I mean, we know that it is always traversed until it's emptied and previous values are already being set to null (so the memory consumption shouldn't be a concern).

Not a big deal, but it could save a few .slice() calls for really large queues.

[petebacondarwin]

I would be interested in benchmarking this against a linked list queue.

[thorn0]

@petebacondarwin Linked list seems to be a bit slower than that queue. You can get the script I used for benchmarking from here.

[thorn0]

@gkalpak Although my benchmark script doesn't show any improvement from doing this, I moved the slice call to a separate method purge, which has to be called manually.

Also I used this queue implementation for asyncQueue and applyAsyncQueue.

[dcherman]

As a play on what @jbedard suggested, what if you looped through the $$postDigestQueue but stored the variable pointing to head in a shared closure so that each call to $digest would start draining the queue at the appropriate location?

// At the top of the `$RootScopeProvider` closure
var postDigestQueuePosition = 0;

while(postDigestQueuePosition < postDigestQueue.length) {
  try {
    postDigestQueue[postDigestQueuePosition++]();
  } catch (e) {
    $exceptionHandler(e);
  }
}

postDigestQueue.length = postDigestQueuePosition = 0;

How does that compare perf wise to the queue? It's also a more minimalistic change

[thorn0]

@dcherman You're right. What you wrote makes perfect sense. I've updated the commit. Of course, it's better than the queue as it doesn't involve any calls to methods like slice. Your solution retains the references to processed items until the array is cleared, but I'm not sure whether it's important.

[jbedard]

Nice 👍

[petebacondarwin]

This looks pretty good now. Thanks @thorn0

[gkalpak]

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

[thorn0]

See above. This loop can be reentered recursively.

[gkalpak]

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

[dcherman]

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.

[thorn0]

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