Sdk: dart2js inlining: short-term improvements for constructors

[rakudrama]

These are some changes to inlining that could be beneficial within the current framework.
I have a prototype for modelling constructors which I will revisit in Q2.

• Distinguish generative constructor _factories_ from generative constructor _bodies_. It is reasonable the expect a noInline annotation to apply only to the factory.
• Model the size of generative constructor factories by
  
  
  
  • visiting initializers up inheritance chain
  
  
  • visiting field initializers
  
  
  • modelling the calls to non-trivial constructor bodies
  
  
• Fix modelling of redirecting constructors
• There is no 'called once' information for constructor bodies. A replacement heuristic is: the constructor body of a leaf type is called once if the constructor factory is not inlined.

See also https://github.com/dart-lang/sdk/issues/9757 for a broader discussion of inlining improvements.

[matanlurey]

I wanted to add a bit more data here, as we're increasingly seeing this as critical.

Effects constructors currently have on code-size is entirely non-intuitive. Consider:

import 'dart:html';

class Ecosystem {
  final version = 1.0;
  final ecology = 'Good';
  final metadata = <String, Object>{};
  final String name;

  Ecosystem(this.name); 
}

class SuperEcosystem extends Ecosystem {
  final String owner;
  SuperEcosystem(String name, this.owner) 
    : super(name);
}

void main(dynamic someInput) {
  Function externalFunc = someInput;
  externalFunc([
    Ecosystem('A'),
    Ecosystem('B'),
    Ecosystem('C'),
    SuperEcosystem('D', 'Mr. D'),
  ]);
}

... emits ...

H.interceptedTypeCheck(someInput, "$isFunction").call$1(
  H.setRuntimeTypeInfo([
    new F.Ecosystem(1, "Good", P.LinkedHashMap_LinkedHashMap$_empty(t1, t2), "A"), 
    new F.Ecosystem(1, "Good", P.LinkedHashMap_LinkedHashMap$_empty(t1, t2), "B"),
    new F.Ecosystem(1, "Good", P.LinkedHashMap_LinkedHashMap$_empty(t1, t2), "C"),
    new F.SuperEcosystem("Mr. D", 1, "Good", P.LinkedHashMap_LinkedHashMap$_empty(t1, t2), "D")], 
  [F.Ecosystem])
);

This is super punishing for following the current style guide:
https://www.dartlang.org/guides/language/effective-dart/usage#prefer-using-a-final-field-to-make-a-read-only-property

Adding @pragma('dart2js:noInline') helps quite a bit:

H.interceptedTypeCheck(someInput, "$isFunction").call$1
(H.setRuntimeTypeInfo([
  F.Ecosystem$("A"),
  F.Ecosystem$("B"),
  F.Ecosystem$("C"),
  F.SuperEcosystem$("D", "Mr. D")], [F.Ecosystem]));

... this looks a lot more like what the author has written.

[rakudrama]

I will take a look at this again. Last time I looked it was hard to get a big win, e.g. you don't add in the cost for declaring F.Ecosystem$ which was eliminated by always inlining.

65% of classes in a certain large app have a single call to the constructor, so the inlining is the right thing (e.g. for SuperEcosystem in your example). This is often true of Acx widgets that are created by injection.

In the above example I want to make changes that push the 1 and "Good" into the JavaScript constructor function, but I need to deprecate the 'full emitter' first. We already do this for null.

[matanlurey]

@rakudrama: Sounds totally reasonable, thanks for the update!

[rakudrama]

/cc @leonsenft

Something to consider is that the inlining annotations now apply to the _factory_ and not the _body_.
We made this change since if you don't inline the factory, you want to inline the body since it likely has one call site.
This change doesn't make any sense for a constructor on an abstract class (like Dart Angular's AppView), since the factory is the factory of the subclass.
I think we should change the annotation to also apply to the body of a non-leaf class.

[rakudrama]

I have comitted cf7a63afe673a38d404375e7b8186d2e6fb738ad which takes are of the most egregious examples.
It does not quite handle the Ecosystem example, but would do so if it had a few more fields or larger field initializers.
Two major apps were improved by 1% or more.

Since the initial task is complete, I'm closing this in favor of #9757.