Support for inlining small methods (independent of they're called inside a loop or not)

sdk/lib/_internal/compiler/implementation/ssa/builder.dart

Index: sdk/lib/_internal/compiler/implementation/ssa/builder.dart
diff --git a/sdk/lib/_internal/compiler/implementation/ssa/builder.dart b/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
index 6ba92acfa136a5ef5cce0926dbb209887ac92d0e..180941d6dd3e7346c704bd1609ab4ca4b90b0481 100644
--- a/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
+++ b/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
@@ -929,8 +929,12 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
     localsHandler = new LocalsHandler(this);
   }
 
-  static const MAX_INLINING_DEPTH = 3;
-  static const MAX_INLINING_NODES = 46;
+  static const MAX_INLINING_DEPTH = 5;

[ngeoffray, 2013/07/16 08:04:45]

Consider moving inlining considerations into its own class.

[kustermann, 2013/07/16 13:27:50]

Moved it now to InlineWeeder.

+  // Invariant: *INSIDE_LOOP* > *OUTSIDE_LOOP*
+  static const INLINING_NODES_OUTSIDE_LOOP = 10;
+  static const INLINING_NODES_OUTSIDE_LOOP_ARG_FACTOR = 10;
+  static const INLINING_NODES_INSIDE_LOOP = 35;
+  static const INLINING_NODES_INSIDE_LOOP_ARG_FACTOR = 15;
 
   List<InliningState> inliningStack;
 
@@ -1225,7 +1229,12 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
     element = element.implementation;
     FunctionElement function = element;
 
-    bool cachedCanBeInlined = backend.canBeInlined[function];
+    var insideLoop = loopNesting > 0 || graph.calledInLoop;

[ngeoffray, 2013/07/16 08:04:45]

var -> bool

[kustermann, 2013/07/16 13:27:50]

Done.

+
+    // Bail out early if the inlining decision is in the cache and we can't
+    // inline (no need to check the hard constraints).
+    bool cachedCanBeInlined =
+        backend.inlineCache.canInline(function, insideLoop: insideLoop);
     if (cachedCanBeInlined == false) return false;
 
     bool meetsHardConstraints() {
@@ -1259,23 +1268,38 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
         return false;
       }
 
+      // Don't inline recursivly

[ngeoffray, 2013/07/16 08:04:45]

Shouldn't that be part of the heuristics?

[kustermann, 2013/07/16 13:27:50]

Why should this be part of a heuristic? We require that the inliner doesn't run
into an infinite loop (=> hard constraint).

We could of course say that we allow recursive inlining up to a certain depth or
so. But I'm not sure if this would be worth it.

+      if (inliningStack.any((entry) => entry.function == function))

[kustermann, 2013/07/15 17:40:27]

I'll add a test to make sure this guard against recursive inlining works.

[ngeoffray, 2013/07/16 08:04:45]

Nit: add braces with a newline on an if.

[kustermann, 2013/07/16 13:27:50]

I saw this style
  if (cond)
    return x;
quite a lot in the code, so I thought that's how the dart2js gods like it.

+        return false;
+
       return true;
     }
 
-    bool heuristicsSayGoodToGo(FunctionExpression functionExpression,
-                                TreeElements newElements) {
-      if (loopNesting == 0 && !graph.calledInLoop) return false;
+    bool heuristicSayGoodToGo(FunctionExpression functionExpression) {
+      //if (inliningStack.length >= MAX_INLINING_DEPTH) {

[kustermann, 2013/07/15 17:40:27]

Don't know if we should limit the inlining depth here (we may want to inline
small functions always).

[ngeoffray, 2013/07/16 08:04:45]

Should we have a canBeInlined cache for methods that are inlined after
MAX_INLINING_DEPTH? This cache would be filled with small methods, smaller than
the current INLINING_NODES constants.

[kustermann, 2013/07/16 13:27:50]

I think we should not make our inlining decisions based on how deep the inlining
stack is. There are other things we should consider for the heuristics, but not
the inlining depth
 - guessing the number of call sites of a function (could be collected by the
type inference algorithm?)
 - ...

This code was probably here only to guard against running into a recursion.

+      //  return false;
+      //}
 
-      int maxDepth = (loopNesting > 0) ? MAX_INLINING_DEPTH : 1;
-      if (inliningStack.length >= maxDepth) return false;
+      if (cachedCanBeInlined == true)
+        return cachedCanBeInlined;

[ngeoffray, 2013/07/16 08:04:45]

One line, or braces.

[kustermann, 2013/07/16 13:27:50]

Done.

 
-      if (cachedCanBeInlined == null) {
-        var canBeInlined =
-            InlineWeeder.canBeInlined(functionExpression, newElements);
-        backend.canBeInlined[function] = canBeInlined;
-        return canBeInlined;
+      var numParameters = function.functionSignature.parameterCount;

[ngeoffray, 2013/07/16 08:04:45]

var -> int

[kustermann, 2013/07/16 13:27:50]

Done.

+      var maxInliningNodes;

[ngeoffray, 2013/07/16 08:04:45]

var -> int.

[kustermann, 2013/07/16 13:27:50]

Done.

+      if (insideLoop) {
+        maxInliningNodes = SsaBuilder.INLINING_NODES_INSIDE_LOOP +
+            SsaBuilder.INLINING_NODES_INSIDE_LOOP_ARG_FACTOR * numParameters;
+      } else {
+        maxInliningNodes = SsaBuilder.INLINING_NODES_OUTSIDE_LOOP +
+            SsaBuilder.INLINING_NODES_OUTSIDE_LOOP_ARG_FACTOR * numParameters;
       }
-      return cachedCanBeInlined;
+      var canBeInlined = InlineWeeder.canBeInlined(

[ngeoffray, 2013/07/16 08:04:45]

var -> bool

[kustermann, 2013/07/16 13:27:50]

Done.

+          functionExpression, maxInliningNodes);
+      if (canBeInlined) {
+        backend.inlineCache.markAsInlinable(element, insideLoop: insideLoop);
+      } else {
+        backend.inlineCache.markAsNonInlinable(element, insideLoop: insideLoop);
+      }
+      return canBeInlined;
     }
 
     void doInlining(FunctionExpression functionExpression) {
@@ -1307,13 +1331,8 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
 
     if (meetsHardConstraints()) {
       FunctionExpression functionExpression = function.parseNode(compiler);
-      TreeElements newElements =
-          compiler.enqueuer.resolution.getCachedElements(function);
-      if (newElements == null) {
-        compiler.internalError("Element not resolved: $function");
-      }
 
-      if (heuristicsSayGoodToGo(functionExpression, newElements)) {
+      if (heuristicSayGoodToGo(functionExpression)) {
         doInlining(functionExpression);
         return true;
       }
@@ -5125,25 +5144,23 @@ class StringBuilderVisitor extends Visitor {
  * finds whether it is too difficult to inline.
  */
 class InlineWeeder extends Visitor {
-  final TreeElements elements;
-
   bool seenReturn = false;
   bool tooDifficult = false;
   int nodeCount = 0;
+  int maxInliningNodes;

[ngeoffray, 2013/07/16 08:04:45]

final

[kustermann, 2013/07/16 13:27:50]

Done.

 
-  InlineWeeder(this.elements);
+  InlineWeeder(this.maxInliningNodes);
 
   static bool canBeInlined(FunctionExpression functionExpression,
-                           TreeElements elements) {
-    InlineWeeder weeder = new InlineWeeder(elements);
+                           int maxInliningNodes) {
+    InlineWeeder weeder = new InlineWeeder(maxInliningNodes);
     weeder.visit(functionExpression.initializers);
     weeder.visit(functionExpression.body);
-    if (weeder.tooDifficult) return false;
-    return true;
+    return !weeder.tooDifficult;
   }
 
   bool registerNode() {
-    if (nodeCount++ > SsaBuilder.MAX_INLINING_NODES) {
+    if (nodeCount++ > maxInliningNodes) {
       tooDifficult = true;
       return false;
     } else {