Controller functions stringified twice when directives are compiled, even in strict DI mode

[robertknight]

Whilst investigating poor performance when instantiating a component, I was surprised to see Function.prototype.toString() showing up in the profile taking a significant amount of time (about 2ms per instance of a component in a list of potentially hundreds of items) despite the app using strict DI.

It appears that when instantiating a component, Angular calls Function.prototype.toString on every controller twice. The first time is in setupControllers due to what looks like a typo in controller == '@' and the second is to determine whether a function is an ES2015 class (!!), which seems silly considering that the proportion of users using native classes in production apps will be tiny.

In testing, it appears that stringifying a large function is somewhat more expensive in Firefox than Chrome/Node/IE.

Steps to reproduce:

1. Open this Plunker: http://plnkr.co/edit/RbdcKbymucZWvOF4V6Co
2. Open dev tools and refresh, observe 2 traces from a monkey-patched Function.prototype.toString invoked on AppController

[Narretz]

and the second is to determine whether a function is an ES2015 class (!!), which seems silly considering that the proportion of users using native classes in production apps will be tiny.

We want to support this, why do you consider this silly? This is going to be used more and more.

The first time is in setupControllers due to what looks like a typo in controller == '@'

This is to support an old API that ngController uses. I think it's undocumented. Maybe we don't need it anymore.

[Narretz]

Or use controller === '@' ....

[Narretz]

It's also worth a shot to raise the issue of toString being slow in Firefox itself.

[robertknight]

We want to support this, why do you consider this silly? This is going to be used more and more.

Apologies, perhaps I should rephrase, I think it is very unfortunate to be paying a performance penalty for something that very few users are using.

It's also worth a shot to raise the issue of toString being slow in Firefox itself.

I can do that, though I don't know whether it is reasonable to expect that stringifying a function will always be very fast. It looks like Firefox might be disposing of the original source in some contexts, possibly as an optimization? - https://dxr.mozilla.org/mozilla-central/source/js/src/jsfun.cpp#936

[Narretz]

Can you test if you see an actual improvement when you change == to === here

angular.js/src/ng/compile.js

Line 2488 in 82a4545

if (controller == '@') {

?

I've tested in a benchmark, but the results vary too strongly, so in this articial case the difference seems negligble.

[robertknight]

Yes, that saves about 30% of the cost of setupControllers in Firefox according to Firefox dev tools' profiler, that's about 5-15ms (0.5-1%) off startup time in our app - there does seem to be quite a lot of variance though.

[gkalpak]

If anything else === '@' saves as a "stringification", so it can't be a bad thing 😃

Now regarding native Class detection, unfortunately we:

1. Need to differentiate between ES2015 Classes and good old constructor functions.
   WHY ? If we don't, we can't assign bindings before calling the constructor (for constructor functions), which will be a massive breaking change.
2. As much as we'd love to have a more robust and performant way of detecting Classes, we don't.
   WHY ? Because we don't 😃

So, as long as (1) and (2) hold, there is not much we can do. Maybe browsers will provide a solution for (2) in the future.

Just m2c...

[thorn0]

We may try to cache the results of the class detection check by adding a property to checked functions.

[gkalpak]

Sounds like a good idea.
@lgalfaso, wdyt ?

[robertknight]

Caching the class detection check seems like a sensible idea. I'm OK with this issue being closed following the fix in bbd3db1 .

In terms of overall performance in our app, this is a relatively minor issue compared to the cost of compiling templates and watches in the $digest loop - so issues like #13915 are potentially much bigger wins now. I'd be quite happy even if such optimizations came with additional restrictions on the features that can be used in directives.

[lgalfaso]

Caching if a controller is a native class sounds like a good idea.

PR welcome :)

[gkalpak]

Closing this since the first stringification (due to == '@') has been eliminated (with #14271) and the second is unavoidable but has been optimized through caching (with #14322).

Thx everyone 👍