Relax inlining limits for simple leaf functions.

Relax inlining limits for simple leaf functions.

A simple leaf is a function that does not call other functions and does not contain complex statements.

Store AST node count and a flag, that indicates whether the function is a simple leaf function or not, in the function shared info.

Use the stored information later in hydrogen to make inlining decision.

BUG=
TEST=

[ulan, 2011-11-23 13:59:38]

Please take a look.

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

I took a quick drive-by glance.  I have a couple of concerns about the approach:

1. This makes the implementation of our inlining decisions more complicated by
introducing yet another way to do something we're already doing.

We have a function IsInlineable that is intended to walk the AST and make a
syntactic decision about inlineability.  It is currently simple, but could be
made to draw a distinction between syntactic constructs we never inline (like
now) and ones that we sometimes do not inline (like you're doing in the AST
constructors), and also compute a count of AST nodes.

Ideally, the parser is not involved with inlining decisions at all, and there is
no need to add more per-isolate global variables (that's a bad smell).

2. It introduces some new vocabulary that is not obvious.  What's "heavy"?  Why
is "primitive" the opposite of heavy?  We already use primitive to describe a
class of JavaScript values.  Why do they interact with the "hard" and "soft"
limits?

I suggest some unified terminology.  What you're trying to do is introduce a
separate "soft" blacklist from the "hard" blacklist we currently have as
implemented by IsInlineable.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4649
src/hydrogen.cc:4649: int ast_node_count = target_shared->ast_node_count();
Move this inside the if where it's used.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4652
src/hydrogen.cc:4652: if (ast_node_count > kMaxInlinedSize) {
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.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4657
src/hydrogen.cc:4657: if (target->shared()->SourceSize() > kMaxSourceSize) {
I wonder if we can just get rid of this now?

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4662
src/hydrogen.cc:4662: if (inlined_count_ > kMaxInlinedNodesSoft) {
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;
}

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4712
src/hydrogen.cc:4712: int count_before = AstNode::Count();
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]

Uploaded a new patch set that renames "heavy" to "non_primitive" and adds
comments. 

I tried moving the counters to IsInlineable (see
http://codereview.chromium.org/8700001/) but don't really like it because there
is no good place to call ComputeAstNodeCountAndInlineableFlags() from. This
function needs to be called after analyzing the scopes and before code
generation, because Jacob's ARM performance improvement CL is using the
"is_primitive" flag in the full-codegen.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4649
src/hydrogen.cc:4649: int ast_node_count = target_shared->ast_node_count();
On 2011/11/24 13:15:56, Kevin Millikin wrote:
> Move this inside the if where it's used.
Now I use to increment "inlined_count_".

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4652
src/hydrogen.cc:4652: if (ast_node_count > kMaxInlinedSize) {
On 2011/11/24 13:15:56, Kevin Millikin wrote:
> 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.

Done.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4657
src/hydrogen.cc:4657: if (target->shared()->SourceSize() > kMaxSourceSize) {
On 2011/11/24 13:15:56, Kevin Millikin wrote:
> I wonder if we can just get rid of this now?
Unfortunately, removing it regresses the delta blue benchmark.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4662
src/hydrogen.cc:4662: if (inlined_count_ > kMaxInlinedNodesSoft) {
On 2011/11/24 13:15:56, Kevin Millikin wrote:
> 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;
> }

Done.

http://codereview.chromium.org/8677008/diff/1/src/hydrogen.cc#newcode4712
src/hydrogen.cc:4712: int count_before = AstNode::Count();
On 2011/11/24 13:15:56, Kevin Millikin wrote:
> 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.

Done.

[Kevin Millikin (Chromium), 2011-11-28 12:11:59]

Another round of comments.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode694
src/ast.h:694: WithStatement(Isolate* isolate, Expression* expression,
Statement* statement)
I think we need a crisp characterization of the things that we allow to be
inlined after the soft limit is reached.  Why do we explicitly forbid 'with'?

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode888
src/ast.h:888: IncrementNonPrimitiveAstNodeCounter(isolate);
It's inconsistent that the base class increments the counter for
IterationStatement, and the subclasses for TryCatchStatement and TryStatement. 
I wonder why that is?  

Is it so we can relax inlining restrictions on try/finally but not try/catch?

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1258
src/ast.h:1258: IncrementNonPrimitiveAstNodeCounter(isolate);
It's not obvious why we won't inline small functions containing calls after the
soft limit is reached?  Intuitively, it seems like it could be productive to
allow them.

Some comment is needed, otherwise someone will want to flip this bit and either
(1) have to repeat all your investigations that determined it was a bad idea or
(2) will break something that assumes it.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1315
src/ast.h:1315: IncrementNonPrimitiveAstNodeCounter(isolate);
Same comment as call.  It intuitively seems like it would be productive to
inline small functions containing 'new'.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1347
src/ast.h:1347: IncrementNonPrimitiveAstNodeCounter(isolate);
Same comment as Call and CallNew.  Why?

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1709
src/ast.h:1709: IncrementNonPrimitiveAstNodeCounter(isolate);
Same question: why is a function literal non-primitive, but all other literals
are primitive?

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1814
src/ast.h:1814: IncrementNonPrimitiveAstNodeCounter(isolate);
This effectively prevents inlining exactly named function expressions after the
soft limit is reached.  Again, it's not clear *why* we want to explicitly
prohibit that compared to functions defined by function declarations and
anonymous function expressions.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc#newcode4674
src/hydrogen.cc:4674: inlined_count_ > kMaxInlinedNodesHard ||
I think 'inlined_count_ > MaxInlinedNodesHard' is trivially false, because
you've already bailed out otherwise.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc#newcode4676
src/hydrogen.cc:4676: TraceInline(target, caller, "cumulative AST node limit
reached");
This should be a distinct bailout message from the earlier one, so it's easy to
see which limit is in play from inspecting --trace-bailout.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h#newcode756
src/hydrogen.h:756: // Make sure that kMaxInlinedSize <
SharedFunctionInfo::kAstNodeCountMask.
How about:

STATIC_ASSERT(kMaxInlinedSize < SharedFunctionInfo::kAstNodeCountMask);

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h#newcode758
src/hydrogen.h:758: static const int kMaxInlinedNodesHard = 270;
I still don't think these constants are documented very well, here or in the
source.  I suggest something like:

// Hard limits.
static const int kMaxSourceSize = 600;  // Per site.
static const int kMaxInlinedSize = 196;  // Per site.
static const int kMaxInlinedNodesHard = 270;  // Cumulative.

// Soft limits.  After the soft limit is reached we will only inline small
primitives.
static const int kMaxInlinedNodesSoft = 196;  // Cumulative
static const int kMaxInlinedPrimitiveSize = 40;  // Per site.

http://codereview.chromium.org/8677008/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/objects.h#newcode5204
src/objects.h:5204: // Make sure that kAstNodeCountMask >
HGraphBuilder::kMaxInlinedSize.
STATIC_ASSERT-ing it in one place or the other is better than this comment.

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc#newcode659
src/parser.cc:659: int ast_nodes_before = isolate()->ast_node_count();
It seems overly complicated to use the global counter for all AST nodes
constructed for this isolate, when what we're interested in is nodes per
function.

I suggest saving the old value and zeroing it here, then restoring the old
value.

Further, you should do it in the FunctionState constructor (the you won't miss
any cases, like the one in Parser::ParseLazy (or do you intend to skip that
one---if so it's pretty subtle)).

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc#newcode661
src/parser.cc:661: isolate()->non_primitive_ast_node_count();
There's no need to count them is there?  If not, it's simpler to just have a
sticky flag.  Save the current value and clear it here (actually in the
FunctionState constructor) and restore its value in the FunctionState
destructor.

[ulan, 2011-11-29 14:06:52]

Thank you for the comments! I uploaded a new patch set.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h
File src/ast.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode694
src/ast.h:694: WithStatement(Isolate* isolate, Expression* expression,
Statement* statement)
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> I think we need a crisp characterization of the things that we allow to be
> inlined after the soft limit is reached.  Why do we explicitly forbid 'with'?

Done. Put the definition of "primitive" in the comment of
FunctionLiteral::is_primitive() and added a comment here.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode888
src/ast.h:888: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> It's inconsistent that the base class increments the counter for
> IterationStatement, and the subclasses for TryCatchStatement and TryStatement.

> I wonder why that is?  
> 
> Is it so we can relax inlining restrictions on try/finally but not try/catch?
Done. Moved it to the base class.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1258
src/ast.h:1258: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> It's not obvious why we won't inline small functions containing calls after
the
> soft limit is reached?  Intuitively, it seems like it could be productive to
> allow them.
> 
> Some comment is needed, otherwise someone will want to flip this bit and
either
> (1) have to repeat all your investigations that determined it was a bad idea
or
> (2) will break something that assumes it.
Added a comment saying that it is for performance. I didn't notice the
performance difference in this CL, but Jakob's CL  uses the fact that primitive
functions are leaf functions.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1315
src/ast.h:1315: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> Same comment as call.  It intuitively seems like it would be productive to
> inline small functions containing 'new'.

Done.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1347
src/ast.h:1347: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> Same comment as Call and CallNew.  Why?

Done.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1709
src/ast.h:1709: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> Same question: why is a function literal non-primitive, but all other literals
> are primitive?
We do not inline functions containing function literals and materialized
literals. Made materialized literals non-primitive as well.

http://codereview.chromium.org/8677008/diff/4001/src/ast.h#newcode1814
src/ast.h:1814: IncrementNonPrimitiveAstNodeCounter(isolate);
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> This effectively prevents inlining exactly named function expressions after
the
> soft limit is reached.  Again, it's not clear *why* we want to explicitly
> prohibit that compared to functions defined by function declarations and
> anonymous function expressions.
You're right. There is no reason to make such functions non-primitive. Removed
the line.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc#newcode4674
src/hydrogen.cc:4674: inlined_count_ > kMaxInlinedNodesHard ||
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> I think 'inlined_count_ > MaxInlinedNodesHard' is trivially false, because
> you've already bailed out otherwise.

Done.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.cc#newcode4676
src/hydrogen.cc:4676: TraceInline(target, caller, "cumulative AST node limit
reached");
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> This should be a distinct bailout message from the earlier one, so it's easy
to
> see which limit is in play from inspecting --trace-bailout.

Done.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h#newcode756
src/hydrogen.h:756: // Make sure that kMaxInlinedSize <
SharedFunctionInfo::kAstNodeCountMask.
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> How about:
> 
> STATIC_ASSERT(kMaxInlinedSize < SharedFunctionInfo::kAstNodeCountMask);

Done.

http://codereview.chromium.org/8677008/diff/4001/src/hydrogen.h#newcode758
src/hydrogen.h:758: static const int kMaxInlinedNodesHard = 270;
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> I still don't think these constants are documented very well, here or in the
> source.  I suggest something like:
> 
> // Hard limits.
> static const int kMaxSourceSize = 600;  // Per site.
> static const int kMaxInlinedSize = 196;  // Per site.
> static const int kMaxInlinedNodesHard = 270;  // Cumulative.
> 
> // Soft limits.  After the soft limit is reached we will only inline small
> primitives.
> static const int kMaxInlinedNodesSoft = 196;  // Cumulative
> static const int kMaxInlinedPrimitiveSize = 40;  // Per site.

Done.

http://codereview.chromium.org/8677008/diff/4001/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/8677008/diff/4001/src/objects.h#newcode5204
src/objects.h:5204: // Make sure that kAstNodeCountMask >
HGraphBuilder::kMaxInlinedSize.
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> STATIC_ASSERT-ing it in one place or the other is better than this comment.

Done. Added assert to the other place.

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc#newcode659
src/parser.cc:659: int ast_nodes_before = isolate()->ast_node_count();
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> It seems overly complicated to use the global counter for all AST nodes
> constructed for this isolate, when what we're interested in is nodes per
> function.
> 
> I suggest saving the old value and zeroing it here, then restoring the old
> value.
> 
> Further, you should do it in the FunctionState constructor (the you won't miss
> any cases, like the one in Parser::ParseLazy (or do you intend to skip that
> one---if so it's pretty subtle)).

Done.

http://codereview.chromium.org/8677008/diff/4001/src/parser.cc#newcode661
src/parser.cc:661: isolate()->non_primitive_ast_node_count();
On 2011/11/28 12:11:59, Kevin Millikin wrote:
> There's no need to count them is there?  If not, it's simpler to just have a
> sticky flag.  Save the current value and clear it here (actually in the
> FunctionState constructor) and restore its value in the FunctionState
> destructor.

Done.

http://codereview.chromium.org/8677008/diff/15001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/8677008/diff/15001/src/hydrogen.cc#newcode4649
src/hydrogen.cc:4649: if (!target->shared()->is_compiled()) {
Is it OK?
It doesn't affect performance.