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