Optimize performance by optionally collectiong only EA directives #14850

drpicox · 2016-06-30T09:28:17Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Performance optimization that introduces a new feature as $compileProvider configuration.

What is the current behavior? (You can also link to an open issue here): $compile collect directives from entities, attributes, classes and comments. It parses all four kind of directives, although usually only E (components) and A ("directives") are used in most of the projects.

What is the new behavior (if this is a feature change)?: It introduces a new configuration in the $compileProvider that disables the compilation of C and M directives:

$compileProvider.commentDirectivesEnabled(false);
$compileProvider.cssClassDirectivesEnabled(false);

Does this PR introduce a breaking change?
No. By default is configured true, thus it is compatible with all existing code.

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)
Docs have been added / updated (for bug fixes / features)

Other information:
It is closely related to #14848
I have reused the same benchmarks to measure the improved performance of this PR, and it is around 10-15% of performance improvement by setting restrictDirectivesTo to 'EA'.

petebacondarwin · 2016-06-30T13:05:05Z

This seems like a pretty good idea since just about every Angular app I have ever written doesn't actually use comment or CSS class based directives. We could even make this opt-out in a future version...

The API naming seems a bit clunky though... Perhaps something more like restrictDirectivesTo('EA') or similar? Open to discussion on that...

Narretz · 2016-06-30T13:50:30Z

I also like it. One of these things that are really obvious in hindsight.
I also think that we could expand the API to allow any combination of restricts (although EA is needed by probably 99.999% of apps).

drpicox · 2016-06-30T14:58:44Z

Good!
I'll try to update it later today :)
(I also like more the naming)

drpicox · 2016-06-30T21:53:23Z

I have updated it.

The function name is restrictDirectivesTo, default value is 'EACM', and the implementation is almost the same.

Because:

restrictDirectivesTo may accept any combination of EACM,
I do not want to add more conditionals than previously existing (deactivate E or A is a little bit more complex),
and it makes no sense to do not include EA

I force to have at least EA in the expression (throws an error if not).

petebacondarwin · 2016-07-01T08:17:37Z

Great and rapid work - thanks @drpicox
Sorry to be a pain but now that I realise that disabling E and A is not really viable, my suggested API naming doesn't make that much sense.

Can we have a quick chat about what might work best before you make any more changes...

Perhaps we could make it explicit, following the getter/setter style of debugInfoEnabled:

commentDirectivesEnabled(false);
cssClassDirectivesEnabled(false);

or perhaps we could disable them together...

commentAndCssClassDirectivesEnabled(false);

or some better thing??

drpicox · 2016-07-01T08:39:12Z

I have seen some people using class directives, mainly because some people work like times of backbone or jquey and they use templates from designers which do thinks like <button class="save-user-button">Save<button> . There is also a popular library to draw charts by using canvas that has class directives (it break when restrict was defaulted to EA but they corrected it to support C).

So it make sense to have separated class and comments. I have never seen a comment directive. I know why it is supposed to be useful because of the angular documentation.

I have difficult to do a chat by now, now I'm at AngularCamp listening to @toddmotto talking about .component :)

drpicox · 2016-07-01T08:40:09Z

Btw I'm follows you at Twitter, so if you follow me you can send me pms and have the chat.

gkalpak · 2016-07-01T09:35:50Z

test/ng/compileSpec.js

@@ -3128,17 +3128,129 @@ describe('$compile', function() {
      });
    });

-    it('should observe interpolated attrs', inject(function($rootScope, $compile) {


Why was this test removed?

Ops! It shouldn't!

Narretz · 2016-07-01T09:46:41Z

I think the explicit version is the way to go. It's verbose, but you are not gonna use this more than once per app, and it also doesn't give the impression that you can disable EA

commentDirectivesEnabled(false);
cssClassDirectivesEnabled(false);

gkalpak · 2016-07-01T09:47:43Z

test/ng/compileSpec.js

+          $rootScope = _$rootScope_;
+        }));
+
+        it('should not compile class directives', function() {


I think it would be cleaner to use test-collect for all testcases. It makes the comparison easier.

gkalpak · 2016-07-01T10:23:04Z

Yeah, 👍 for the explicit versions from me too.

BTW, I would feel more confident about the tests, if we used the they helper to run each of them against all possible combinations and verfiy that the behavior is correct. Something like this:

they('should handle comment directives appropriately ($prop)', [
  {restrict: 'EA', collect: false},
  {restrict: 'EAC', collect: false},
  {restrict: 'EAM', collect: true},
  {restrict: 'EACM', collect: true}
], function(config) {
  module(function($compileProvider) {
    $compileProvider.restrictDirectivesTo(config.restrict);
  });

  inject(function() {
    var html = '<!-- directive: test-collect -->';
    element = $compile('<div>' + html + '</div>')($rootScope);
    expect(collected).toBe(config.collect);
  });
});

petebacondarwin · 2016-07-01T13:16:13Z

OK, so lets go with

commentDirectivesEnabled(false);
cssClassDirectivesEnabled(false);

@drpicox can you take a look at @gkalpak's comments about the testing?

Thanks

gkalpak · 2016-07-27T08:48:17Z

test/ng/compileSpec.js

+      expect(collected).toBe(true);
+    }));
+
+    angular.forEach([


You don't need angular..

Ops, true! Thanks!

gkalpak · 2016-07-27T08:55:06Z

Do we have any (verifiable) numbers on how performance is affected by this?
That would be nice 😄

I also think it is a good idea to mention this in the Running in Production section of the Developer Guide.

drpicox · 2016-07-27T09:12:30Z

I did my performance benchmarks using code from my customers (just right click and get source).
I also have used benchpress and numbers looked similar (although I am not sure because I am not familiar with benchpress yet).
I'll try to modify benchpress to enable/disable directives conditionally so we can verify it.
And btw, the gain of comment directives is very low, there are usually no commens, but from element classes is high.

drpicox · 2016-07-27T14:29:09Z

I have added a benchpress benchmark.

I follow more or less the same structure of the other benchmarks
I have added query params (they can be set by just clicking links), in order to configure $compileProvider in config time.
I have added a repeater in order to make more benchmark repeats easily (I think that it might be possible to be configured from benchpress, but I didn't found it in a quick glance)
I have added a chooser for templates: the idea is the compile time varies in function the of compiled template. Now I just have added two templates from http://getbootstrap.com/getting-started/#examples , I consider that they are realistic.

How to run?

$ grunt bp_build
$ grunt webserver
go to incognito mode
open http://localhost:8000/build/benchmarks/bootstrap-compile-bp
select the kind of directives that you want to enable
change if you wish repeat count
select the template
and click "Loop 25x"
(better if for each test you create a new window)

The numbers that I have obtained are:

Carousel template: ~12% of speedup
- Compile everything: mean: 206.71ms ± 24% (stddev 49.45) (min 177.41 / max 332.15)
- Compile only EA: mean: 183.14ms ± 25% (stddev 45.87) (min 154.76 / max 303.56)
Theme template: ~10% of speedup
- Compile Everything: mean: 615.42ms ± 21% (stddev 126.72) (min 531.03 / max 923.89)
- Compile only EA: mean: mean: 555.74ms ± 21% (stddev 117.11) (min 474.25 / max 793.57)

… conditionally

gkalpak · 2016-07-28T09:13:15Z

src/ng/compile.js

+   * $compileProvider.commentDirectivesEnabled(false);
+   * ```
+   *
+   * @param {boolean} false if the compiler may ignore directives on comments


You should have the argument name right after the type. Now false is interpreted as the argument type and the rest as the description.

drpicox · 2016-07-28T10:04:47Z

Thanks @gkalpak for comments, I have updated it.

…ditionally

petebacondarwin · 2016-07-31T05:35:43Z

LGTM

…ass and comment directives When the functions `cssClassDirectivesEnabled()` / `commentDirectivesEnabled()` on the `$compileProvider` are called with `false`, then the compiler won't look for directives on css classes / comment elements. This can result in a compilation speed-up of around 10%. PR (#14850)

googlebot added the cla: yes label Jun 30, 2016

drpicox force-pushed the perf-collectDirectives-EA branch 4 times, most recently from ab35cc9 to 18ef9f1 Compare June 30, 2016 10:03

Narretz changed the title ~~Optimize performance by collectiong only EA directives~~ Optimize performance by optionally collectiong only EA directives Jun 30, 2016

petebacondarwin added this to the 1.5.x milestone Jun 30, 2016

petebacondarwin added type: feature type: perf branch: 1.6.x component: $compile needs: review needs: public api labels Jun 30, 2016

drpicox force-pushed the perf-collectDirectives-EA branch from 18ef9f1 to e40345e Compare June 30, 2016 21:36

gkalpak reviewed Jul 1, 2016
View reviewed changes

gkalpak reviewed Jul 27, 2016
View reviewed changes

drpicox force-pushed the perf-collectDirectives-EA branch 3 times, most recently from 699ab98 to af12104 Compare July 27, 2016 14:10

drpicox force-pushed the perf-collectDirectives-EA branch from af12104 to dd07d9c Compare July 27, 2016 17:27

perf($compile): add flags to compile css class and comment directives…

4bde767

… conditionally

drpicox force-pushed the perf-collectDirectives-EA branch from dd07d9c to 4bde767 Compare July 27, 2016 18:08

gkalpak reviewed Jul 28, 2016
View reviewed changes

drpicox force-pushed the perf-collectDirectives-EA branch 5 times, most recently from 2fa432f to af05406 Compare July 29, 2016 07:40

few fixes about flags to compile css class and comment directives con…

66f2eaa

…ditionally

drpicox force-pushed the perf-collectDirectives-EA branch from af05406 to 66f2eaa Compare July 29, 2016 11:49

Narretz modified the milestones: 1.5.9, 1.5.x Aug 3, 2016

Narretz merged commit 4c2964d into angular:master Aug 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize performance by optionally collectiong only EA directives #14850

Optimize performance by optionally collectiong only EA directives #14850

drpicox commented Jun 30, 2016 •

edited

Loading

petebacondarwin commented Jun 30, 2016

Narretz commented Jun 30, 2016

drpicox commented Jun 30, 2016 •

edited

Loading

drpicox commented Jun 30, 2016 •

edited

Loading

petebacondarwin commented Jul 1, 2016

drpicox commented Jul 1, 2016 •

edited

Loading

drpicox commented Jul 1, 2016

gkalpak Jul 1, 2016

drpicox Jul 1, 2016

Narretz commented Jul 1, 2016

gkalpak Jul 1, 2016

gkalpak commented Jul 1, 2016

petebacondarwin commented Jul 1, 2016

gkalpak Jul 27, 2016

drpicox Jul 27, 2016

gkalpak commented Jul 27, 2016 •

edited

Loading

drpicox commented Jul 27, 2016 •

edited

Loading

drpicox commented Jul 27, 2016 •

edited

Loading

gkalpak Jul 28, 2016

drpicox commented Jul 28, 2016

petebacondarwin commented Jul 31, 2016

Optimize performance by optionally collectiong only EA directives #14850

Optimize performance by optionally collectiong only EA directives #14850

Conversation

drpicox commented Jun 30, 2016 • edited Loading

petebacondarwin commented Jun 30, 2016

Narretz commented Jun 30, 2016

drpicox commented Jun 30, 2016 • edited Loading

drpicox commented Jun 30, 2016 • edited Loading

petebacondarwin commented Jul 1, 2016

drpicox commented Jul 1, 2016 • edited Loading

drpicox commented Jul 1, 2016

gkalpak Jul 1, 2016

Choose a reason for hiding this comment

drpicox Jul 1, 2016

Choose a reason for hiding this comment

Narretz commented Jul 1, 2016

gkalpak Jul 1, 2016

Choose a reason for hiding this comment

gkalpak commented Jul 1, 2016

petebacondarwin commented Jul 1, 2016

gkalpak Jul 27, 2016

Choose a reason for hiding this comment

drpicox Jul 27, 2016

Choose a reason for hiding this comment

gkalpak commented Jul 27, 2016 • edited Loading

drpicox commented Jul 27, 2016 • edited Loading

drpicox commented Jul 27, 2016 • edited Loading

gkalpak Jul 28, 2016

Choose a reason for hiding this comment

drpicox commented Jul 28, 2016

petebacondarwin commented Jul 31, 2016

drpicox commented Jun 30, 2016 •

edited

Loading

drpicox commented Jun 30, 2016 •

edited

Loading

drpicox commented Jun 30, 2016 •

edited

Loading

drpicox commented Jul 1, 2016 •

edited

Loading

gkalpak commented Jul 27, 2016 •

edited

Loading

drpicox commented Jul 27, 2016 •

edited

Loading

drpicox commented Jul 27, 2016 •

edited

Loading