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. http://codereview.chromium.org/8677008/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
