Devirtualize AstNode and subclasses, except for visiting-related methods.

Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather than double dispatch. With additional packing in a third CL there's roughly 8 byte reduction for almost all AstNode types, and for some even more.

BUG=

Committed: https://crrev.com/157008add027193b9b4fc21b55ff0bec7d0901c5
Cr-Commit-Position: refs/heads/master@{#37770}

[Toon Verwaest, 2016-07-13 16:38:39]

verwaest@chromium.org changed reviewers:
+ ishell@chromium.org, mstarzinger@chromium.org

[Toon Verwaest, 2016-07-13 16:38:40]

PTAL

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL I switch over that tag rather than double dispatch. With additional packing
in a third CL there's roughly 8 byte reduction for almost all AstNode types, and
for some even more.

[Toon Verwaest, 2016-07-13 16:42:33]

Description was changed from

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

BUG=
==========

to

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather
than double dispatch. With additional packing in a third CL there's roughly 8
byte reduction for almost all AstNode types, and for some even more.

BUG=
==========

[Michael Starzinger, 2016-07-14 08:38:51]

https://codereview.chromium.org/2126233002/diff/40001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2126233002/diff/40001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2975: PrepareFrameState(node,
stmt->StackCheckId());
I think if we were to devirtualize IterationStatement::StackCheckId() but leave
it as a member on IterationStatement and use a dispatch on the AST type in the
implementation, then this visitation method would not need to be templatized.
WDYT?

[Michael Starzinger, 2016-07-14 08:47:01]

https://codereview.chromium.org/2126233002/diff/40001/src/compiler/ast-graph-...
File src/compiler/ast-graph-builder.cc (right):

https://codereview.chromium.org/2126233002/diff/40001/src/compiler/ast-graph-...
src/compiler/ast-graph-builder.cc:2975: PrepareFrameState(node,
stmt->StackCheckId());
On 2016/07/14 08:38:51, Michael Starzinger wrote:
> I think if we were to devirtualize IterationStatement::StackCheckId() but
leave
> it as a member on IterationStatement and use a dispatch on the AST type in the
> implementation, then this visitation method would not need to be templatized.
> WDYT?

Alternatively Jaro suggested to just pass in the "stack_check_id" as an argument
to this function. That would also work for me and even sounds a bit cleaner.

[Toon Verwaest, 2016-07-14 09:01:17]

Addressed comment

[Michael Starzinger, 2016-07-14 09:02:57]

LGTM. Thanks.

[Igor Sheludko, 2016-07-14 14:02:33]

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode84
src/ast/ast.cc:84: case k##Node:             \
How about adding something like 
  STATIC_ASSERT(&Expression::initialization != &Node::initialization);
here and in similar places below to avoid accidental infinite recursion.

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode170
src/ast/ast.cc:170: IsContinueStatement() || IsBreakStatement() ||
IsReturnStatement() ||
How about following same pattern for all JumpStatements:
  IsContinueStatement() && AsContinueStatement()->IsJump()?
otherwise JumpStatements::IsJump() is going to be unused and could eventually
went out of sync with this method.

Maybe it is even better to rewrite this whole method the same way you rewrote
IsMonomorphic() dispatcher (with switch and casts).

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode861
src/ast/ast.cc:861: CALL_NODE_LIST(GENERATE_CASE)
GENERATE_CASE(ObjectLiteralProperty) ?

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.h
File src/ast/ast.h (left):

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.h#oldcode1384
src/ast/ast.h:1384: bool IsMonomorphic() { return !receiver_type_.is_null(); }
Why is it disappeared?

[Toon Verwaest, 2016-07-14 14:19:39]

Addressed comments.

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode84
src/ast/ast.cc:84: case k##Node:             \
On 2016/07/14 14:02:33, Igor Sheludko wrote:
> How about adding something like 
>   STATIC_ASSERT(&Expression::initialization != &Node::initialization);
> here and in similar places below to avoid accidental infinite recursion.

I'd add STATIC_ASSERT(k##Node != kExpression), however, only leaf-classes are in
NodeType. Which also means that it's impossible to get Expression in here ... Do
you really want me to add it? :)

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode170
src/ast/ast.cc:170: IsContinueStatement() || IsBreakStatement() ||
IsReturnStatement() ||
On 2016/07/14 14:02:33, Igor Sheludko wrote:
> How about following same pattern for all JumpStatements:
>   IsContinueStatement() && AsContinueStatement()->IsJump()?
> otherwise JumpStatements::IsJump() is going to be unused and could eventually
> went out of sync with this method.
> 
> Maybe it is even better to rewrite this whole method the same way you rewrote
> IsMonomorphic() dispatcher (with switch and casts).

Done.

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode861
src/ast/ast.cc:861: CALL_NODE_LIST(GENERATE_CASE)
On 2016/07/14 14:02:33, Igor Sheludko wrote:
> GENERATE_CASE(ObjectLiteralProperty) ?

ObjectLiteralProperty is not an AstNode. It derives directly from ZoneObject.
Hence its IsMonomorphic doesn't overlap.

[Igor Sheludko, 2016-07-14 14:49:31]

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode84
src/ast/ast.cc:84: case k##Node:             \
On 2016/07/14 14:19:39, Toon Verwaest wrote:
> On 2016/07/14 14:02:33, Igor Sheludko wrote:
> > How about adding something like 
> >   STATIC_ASSERT(&Expression::initialization != &Node::initialization);
> > here and in similar places below to avoid accidental infinite recursion.
> 
> I'd add STATIC_ASSERT(k##Node != kExpression), however, only leaf-classes are
in
> NodeType. Which also means that it's impossible to get Expression in here ...
Do
> you really want me to add it? :)

No, the idea is to ensure that when you call a foo() via static_cast<const
Node*>(this)->foo() you will not actually call the dispatcher foo() again.

https://codereview.chromium.org/2126233002/diff/120001/src/ast/ast.cc#newcode861
src/ast/ast.cc:861: CALL_NODE_LIST(GENERATE_CASE)
On 2016/07/14 14:19:39, Toon Verwaest wrote:
> On 2016/07/14 14:02:33, Igor Sheludko wrote:
> > GENERATE_CASE(ObjectLiteralProperty) ?
> 
> ObjectLiteralProperty is not an AstNode. It derives directly from ZoneObject.
> Hence its IsMonomorphic doesn't overlap.

Ah, indeed.

[Igor Sheludko, 2016-07-14 14:52:26]

lgtm

[Toon Verwaest, 2016-07-14 14:52:58]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-07-14 14:52:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2126233002/#ps160001
(title: "Added STATIC_ASSERTs")

[commit-bot: I haz the power, 2016-07-14 14:53:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 14:56:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-14 14:56:19]

Try jobs failed on following builders:
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[Toon Verwaest, 2016-07-14 14:58:59]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-07-14 14:58:59]

The patchset sent to the CQ was uploaded after l-g-t-m from ishell@chromium.org,
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2126233002/#ps180001
(title: "remove static assert again")

[commit-bot: I haz the power, 2016-07-14 14:59:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-14 15:27:02]

Description was changed from

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather
than double dispatch. With additional packing in a third CL there's roughly 8
byte reduction for almost all AstNode types, and for some even more.

BUG=
==========

to

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather
than double dispatch. With additional packing in a third CL there's roughly 8
byte reduction for almost all AstNode types, and for some even more.

BUG=
==========

[commit-bot: I haz the power, 2016-07-14 15:27:03]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-14 15:27:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-14 15:28:54]

Description was changed from

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather
than double dispatch. With additional packing in a third CL there's roughly 8
byte reduction for almost all AstNode types, and for some even more.

BUG=
==========

to

==========
Devirtualize AstNode and subclasses, except for visiting-related methods.

This is a requirement to replace AstNode vtable with a NodeType tag. In a second
CL (https://codereview.chromium.org/2142233003/) I switch over that tag rather
than double dispatch. With additional packing in a third CL there's roughly 8
byte reduction for almost all AstNode types, and for some even more.

BUG=

Committed: https://crrev.com/157008add027193b9b4fc21b55ff0bec7d0901c5
Cr-Commit-Position: refs/heads/master@{#37770}
==========

[commit-bot: I haz the power, 2016-07-14 15:28:55]

Patchset 10 (id:??) landed as
https://crrev.com/157008add027193b9b4fc21b55ff0bec7d0901c5
Cr-Commit-Position: refs/heads/master@{#37770}