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