Relax inlining limits for simple leaf functions.

src/hydrogen.cc

 // Copyright 2011 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 4628 matching lines @@
   CallKind call_kind = (expr->expression()->AsProperty() == NULL)
       ? CALL_AS_FUNCTION
       : CALL_AS_METHOD;
 
   // Precondition: call is monomorphic and we have found a target with the
   // appropriate arity.
   Handle<JSFunction> caller = info()->closure();
   Handle<JSFunction> target = expr->target();
   Handle<SharedFunctionInfo> target_shared(target->shared());
 
-  // Do a quick check on source code length to avoid parsing large
-  // inlining candidates.
-  if (FLAG_limit_inlining && target->shared()->SourceSize() > kMaxSourceSize) {
-    TraceInline(target, caller, "target text too big");
-    return false;
+  int ast_node_count = target_shared->ast_node_count();

[Kevin Millikin (Chromium), 2011/11/24 13:15:56]

Move this inside the if where it's used.

[ulan, 2011/11/25 09:11:15]

Now I use to increment "inlined_count_".

+
+  if (FLAG_limit_inlining) {
+    if (ast_node_count > kMaxInlinedSize) {

[Kevin Millikin (Chromium), 2011/11/24 13:15:56]

All of these need some comment about what we're trying to achieve, please. 
Either here in the code, or at the constant declaration.

Otherwise, it's impossible to figure out how to tune it.

E.g.:

1. Never inline big functions (bigger than kMaxInlinedSize AST nodes or
kMaxSourceSize source text characters).

2. After reaching kMaxInlinedNodesSoft of successfully inlined AST nodes, only
inline small 'primitive' functions (smaller than kMaxInlinedPrimitiveSize,
primitive according to a syntactic criterion).

3. After reaching kMaxInlinedNodesHard of successfully inlined AST nodes, do not
inline anything.

[ulan, 2011/11/25 09:11:15]

Done.

+      TraceInline(target, caller, "target AST is too large");
+      return false;
+    }
+
+    if (target->shared()->SourceSize() > kMaxSourceSize) {

[Kevin Millikin (Chromium), 2011/11/24 13:15:56]

I wonder if we can just get rid of this now?

[ulan, 2011/11/25 09:11:15]

Unfortunately, removing it regresses the delta blue benchmark.

+      TraceInline(target, caller, "target text too big");
+      return false;
+    }
+
+    if (inlined_count_ > kMaxInlinedNodesSoft) {

[Kevin Millikin (Chromium), 2011/11/24 13:15:56]

OK, but the logic is hard to follow because it doesn't match the way the
conditions would be described (above).  Try rewriting it into something like:

// Do not inline after the hard limit is reached.
if (inlined_count_ > kMaxInlinedNodesHard) ... return false;

// Only inline small primitives after the soft limit is reached.
if (inlined_count_ > kMaxInlinedNodesSoft &&
    !(target_shared_is_primitive() && ast_node_count <=
kMaxInlinedPrimitiveSize)) {
  ... return false;
}

[ulan, 2011/11/25 09:11:15]

Done.

+      if (ast_node_count > kMaxInlinedPrimitiveSize ||
+          inlined_count_ > kMaxInlinedNodesHard ||
+          !target_shared->is_primitive()) {
+        TraceInline(target, caller, "cumulative AST node limit reached");
+        return false;
+      }
+    }
   }
 
   // Target must be inlineable.
   if (!target->IsInlineable()) {
     TraceInline(target, caller, "target not inlineable");
     return false;
   }
 
 #if !defined(V8_TARGET_ARCH_IA32)
   // Target must be able to use caller's context.
@@ skipping 22 matching lines @@
   // Don't inline recursive functions.
   for (FunctionState* state = function_state();
        state != NULL;
        state = state->outer()) {
     if (state->compilation_info()->closure()->shared() == *target_shared) {
       TraceInline(target, caller, "target is recursive");
       return false;
     }
   }
 
-  // We don't want to add more than a certain number of nodes from inlining.
-  if (FLAG_limit_inlining && inlined_count_ > kMaxInlinedNodes) {
-    TraceInline(target, caller, "cumulative AST node limit reached");
-    return false;
-  }
-
   int count_before = AstNode::Count();

[Kevin Millikin (Chromium), 2011/11/24 13:15:56]

Please remove this, because it's only used to compute nodes_added which is now
unused.  Please remove nodes_added.  Please remove AstNode::Count(), since these
are the only callers.

[ulan, 2011/11/25 09:11:15]

Done.

 
   // Parse and allocate variables.
   CompilationInfo target_info(target);
   if (!ParserApi::Parse(&target_info) ||
       !Scope::Analyze(&target_info)) {
     if (target_info.isolate()->has_pending_exception()) {
       // Parse or scope error, never optimize this function.
       SetStackOverflow();
       target_shared->DisableOptimization(*target);
     }
     TraceInline(target, caller, "parse failure");
     return false;
   }
 
   if (target_info.scope()->num_heap_slots() > 0) {
     TraceInline(target, caller, "target has context-allocated variables");
     return false;
   }
   FunctionLiteral* function = target_info.function();
 
   // Count the number of AST nodes added by inlining this call.
   int nodes_added = AstNode::Count() - count_before;
-  if (FLAG_limit_inlining && nodes_added > kMaxInlinedSize) {
-    TraceInline(target, caller, "target AST is too large");
-    return false;
-  }
 
   // Don't inline functions that uses the arguments object or that
   // have a mismatching number of parameters.
   int arity = expr->arguments()->length();
   if (function->scope()->arguments() != NULL ||
       arity != target_shared->formal_parameter_count()) {
     TraceInline(target, caller, "target requires special argument handling");
     return false;
   }
 
@@ skipping 2413 matching lines @@
     }
   }
 
 #ifdef DEBUG
   if (graph_ != NULL) graph_->Verify(false);  // No full verify.
   if (allocator_ != NULL) allocator_->Verify();
 #endif
 }
 
 } }  // namespace v8::internal