perf($compile): only use document fragments and copy data when necessary (replaceWith) #9365

jbedard · 2014-10-01T09:20:37Z

This does a few things, all within the internal replaceWith...

only creates the document fragment if needed and clarifies why it's there
only copies data if the node hasData to avoid creating empty data to copy (a jqLite version of hasData was added)
uses the static data methods to avoid creating jqLite/jQuery objects
cleans the data of all replaced elements the same way (this will no longer trigger $destroy on nodes 1+, like node 0, which seems correct to me)
uses jqLite/jQuery/Array.splice to do the array modifications

tbosch · 2014-10-09T21:02:16Z

src/ng/compile.js

+      // If multiple elements are being replaced they will be placed into a temporary fragment
+      // so they can still be traversed via .nextSibling or .parent.children while detached.
+      // This also detaches the elements (index 1+) if they have a parent.
+      if (1 < removeCount) {


Easier to read: removeCount > 1

I think putting the (expected to be) smaller value on the left is easier to read, but I think I'm normally the odd one out on that one. Then expressions such as 1 < a && a < 5 read nicely left to right, and you don't have to remember what > means!

But I'll change it when/if this is closer to being merged, along with any other feedback...

+1 for what @tbosch said. we don't use this style elsewhere in the code base, so we should be consistent.

IgorMinar · 2014-10-31T15:27:19Z

Do we have a benchmark that shows the impact of this change?

jbedard · 2014-11-01T09:29:30Z

I don't think any of the benchmarks cover this right now. replaceWith is mainly used during compilation or during linking of a templateUrl directive (if linked before the templateUrl promise resolves).

Just doing a quick profile such as:
while (loop a lot) $compile( $("<div replace-directive></div>") )
replaceWith is about 30% faster (~6% => 4% of the $compile call).

Or for the templateUrl case something such as:
var l = $compile( $("<div replace-url-directive></div>") ); while (loop a lot) l($rootScope)
replaceWith goes from ~30% => 25% of the link call. In this case GC also seems to go down from ~12% => 7%.

On another note... it looks like both of those cases can be 2x faster when replacing a single node (99.9% of cases?) by using a simple = instead of .splice for the 2 array entry substitutions, but that's adding code... I've added this in a second commit (and used indexOf instead of a loop+break).

IgorMinar · 2014-11-12T01:15:07Z

src/jqLite.js

@@ -169,6 +169,13 @@ function jqLiteAcceptsData(node) {
  return nodeType === NODE_TYPE_ELEMENT || !nodeType || nodeType === NODE_TYPE_DOCUMENT;
 }

+function jqLiteHasData(node) {


should we document this in angular.element docs?

I would vote no for now since the other static methods (jqLiteData, jqLiteRemoveData) have not been documented. But we easily could, and we can also add the non-static version of hasData...

IgorMinar · 2014-11-12T01:15:21Z

src/ng/compile.js

-      fragment.appendChild(firstElementToRemove);
+      // If multiple elements are being replaced they will be placed into a temporary fragment
+      // so they can still be traversed via .nextSibling or .parent.children while detached.
+      // This also detaches the elements (index 1+) if they have a parent.


"index 1+"? what does that mean?

If the elementsToRemove have a parent node this what removes them, except the firstElementToRemove which was removed using replaceWith above. I'll try to rephrase that...

IgorMinar · 2014-11-12T01:16:51Z

otherwise this looks pretty good to me.

@lgalfaso or @caitp can you take a quick look too?

IgorMinar · 2014-11-12T01:17:15Z

it would be great if you could add a benchpress benchmark for this scenario so that we don't regress the perf of this in the future.

caitp · 2014-11-12T01:55:22Z

patch looks okay to me, but I agree we should see how this impacts benchmarks / applications. I'm not sure that needs to block landing

jbedard · 2014-11-12T04:57:06Z

Updated the commits.

For the benchmarks I'm thinking we'll need a new benchpress app to allow measuring compile, link before templateUrl is resolved, etc. Any objections to a new bp app? Or other ideas?

jbedard · 2014-11-13T06:50:32Z

I've added a new benchpress app to measure specific directive/compile/link features. This is quite different from the other benchpress apps so please take a look and let me know what you think. Basically instead of adding N test cases it has N features that can be turned on/off to test different combos of those features (without having to write N! test cases).

The benchmarks seem to follow what I said in my previous comment:

compiling a directive with template + replace:true is ~20% faster
linking a directive with templateUrl + replace:true is ~15% faster

jbedard · 2015-01-26T01:44:50Z

So is this worth it? I think the code is simpler and explains some TODOs which is worth it on its own. The new benchmark app I'm not so sure about though, has anyone looked at it...?

jbedard · 2015-06-06T20:01:35Z

These changes (excluding the benchmarks) have been broken up into smaller PRs, closing this...

mary-poppins added cla: yes and removed cla: no labels Oct 1, 2014

jeffbcross force-pushed the master branch from cad9560 to f294244 Compare October 2, 2014 22:09

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

jeffbcross force-pushed the master branch 3 times, most recently from 8292c21 to 7b9fddf Compare October 9, 2014 20:43

tbosch self-assigned this Oct 9, 2014

btford added type: perf component: $compile labels Oct 9, 2014

tbosch added this to the Backlog milestone Oct 9, 2014

tbosch reviewed Oct 9, 2014
View reviewed changes

tbosch assigned jeffbcross and unassigned tbosch Oct 9, 2014

jeffbcross added the post-1.3.0 label Oct 9, 2014

jeffbcross removed their assignment Oct 9, 2014

jeffbcross force-pushed the master branch from e8dc429 to e83fab9 Compare October 10, 2014 17:38

tbosch modified the milestones: Backlog, 1.3.1 Oct 15, 2014

petebacondarwin force-pushed the master branch from 4dd5a20 to 998c61c Compare October 19, 2014 10:58

jbedard force-pushed the compile-replacewith-perf branch from e3a9ae7 to 0ca44b3 Compare November 1, 2014 09:25

IgorMinar reviewed Nov 12, 2014
View reviewed changes

jbedard force-pushed the compile-replacewith-perf branch from 0ca44b3 to cda5241 Compare November 12, 2014 04:29

jbedard force-pushed the compile-replacewith-perf branch 3 times, most recently from 2b98ed5 to ee4adc1 Compare November 13, 2014 06:43

jbedard added 3 commits January 25, 2015 17:38

perf($compile): only use document fragments and copy data when necessary

c0fbf83

perf($compile): improving single-node case of replaceWith

6dfc188

perf(benchmark): add compile/link benchmarks

2eed4cf

jbedard force-pushed the compile-replacewith-perf branch from ee4adc1 to 2eed4cf Compare January 26, 2015 01:40

jbedard mentioned this pull request Mar 5, 2015

refactor($compile): use indexof/splice instead of manually manipulating jquery object #11248

Closed

This was referenced Jun 6, 2015

perf($compile): only use document fragments when necessary #12041

Closed

perf($compile): avoid jquery data calls when there is no data #12042

Closed

jbedard closed this Jun 6, 2015

perf($compile): only use document fragments and copy data when necessary (replaceWith) #9365

perf($compile): only use document fragments and copy data when necessary (replaceWith) #9365

Conversation

jbedard commented Oct 1, 2014

Uh oh!

tbosch Oct 9, 2014

Choose a reason for hiding this comment

Uh oh!

jbedard Oct 10, 2014

Choose a reason for hiding this comment

Uh oh!

IgorMinar Nov 12, 2014

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Oct 31, 2014

Uh oh!

jbedard commented Nov 1, 2014

Uh oh!

IgorMinar Nov 12, 2014

Choose a reason for hiding this comment

Uh oh!

jbedard Nov 12, 2014

Choose a reason for hiding this comment

Uh oh!

IgorMinar Nov 12, 2014

Choose a reason for hiding this comment

Uh oh!

jbedard Nov 12, 2014

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Nov 12, 2014

Uh oh!

IgorMinar commented Nov 12, 2014

Uh oh!

caitp commented Nov 12, 2014

Uh oh!

jbedard commented Nov 12, 2014

Uh oh!

jbedard commented Nov 13, 2014

Uh oh!

jbedard commented Jan 26, 2015

Uh oh!

jbedard commented Jun 6, 2015

Uh oh!

Uh oh!