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

perf(Angular): only create new collection in getBlockNodes if the block has changed #9899

Closed
wants to merge 1 commit into from

Conversation

jbedard
Copy link
Contributor

@jbedard jbedard commented Nov 4, 2014

Doing something such as...

var scope = $rootScope.$new();
$compile( $('<div ng-if="i"></div>').appendTo('body') )(scope);
while (loop forever) {
   scope.i = (0 === ++i%2);
   scope.digest()
}

The getBlockNodes goes from about 2.5% => 0.25%. In any real test that diff would probably round down to zero though. But for only 3 extra lines (2 being }) I thought I'd at least throw it out there...

@lgalfaso
Copy link
Contributor

lgalfaso commented Nov 7, 2014

There is something I do not fin 100% satisfactory with the patch, but I am not sure exactly what it is... Why not just

// warning untested code
var node = nodes[0];
var endNode = nodes[nodes.length - 1];
var i = 1;
do {
  node = node.nextSibling;
  if (!node) break;
  nodes[i++] = node;
} while (node !== endNode);
nodes.length = i;
return nodes;

?

@jbedard
Copy link
Contributor Author

jbedard commented Nov 7, 2014

I agree the existing patch is a little weird (something to do with the happens-once-if within a loop?).

I think by modifying the original (like your snippet) it may have broke some use cases but I'd have to double check that. I think the array entries after the new length also need to be cleared. I'll give it a shot when I have a chance.

@jbedard
Copy link
Contributor Author

jbedard commented Nov 8, 2014

Modifying the passed in array/jqlite object causes some tests to fail, I haven't really figured out why. I think that's why I originally didn't do that and left that part of the TODO in there.

@caitp
Copy link
Contributor

caitp commented Nov 11, 2014

what do we actually use getBlockNodes() for? grepping...

@caitp
Copy link
Contributor

caitp commented Nov 11, 2014

I guess it's used by some element transclusion directives, alright

@caitp caitp added this to the 1.3.x milestone Nov 11, 2014
@jbedard
Copy link
Contributor Author

jbedard commented Nov 11, 2014

I believe its main purpose is to pickup DOM updates such as nodes being added/removed between the ng-if/repeat start/end nodes. So in most cases there should be no changes.

@jbedard
Copy link
Contributor Author

jbedard commented Feb 5, 2015

I've rebased and updated this a bit to make it less awkward (imo). I'm fine just closing this if no one is interested though... (see jbedard@77f0881 for the old one that I think is more awkward).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants