Move megamorphic lookup code to stub instead of inlining. Preformance loss < 5%, instruction size r…

Move megamorphic lookup code to stub instead of inlining. Preformance loss < 5%, instruction size reduced by 30% (when using always-megamorphic).

R=fschneider@google.com

Committed: https://code.google.com/p/dart/source/detail?r=45525

[srdjan, 2015-05-04 13:22:58]

srdjan@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-05-04 22:06:20]

LGTM with comments.

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1295: if (FLAG_use_megamorphic_stub) {
To avoid performance regression in optimized code, maybe try always doing the
inline lookup in optimized code?

if (FLAG_use_megamorphic_stub && !is_optimizing()) {
  ...
} else {
  ...
}

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1334: }
Maybe you can factor this lookup code into a common code generation helper that
the stub can reuse? It would avoid the duplication between here and
stub_code_*.cc.

[srdjan, 2015-05-05 07:15:29]

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1295: if (FLAG_use_megamorphic_stub) {
On 2015/05/04 22:06:20, Florian Schneider wrote:
> To avoid performance regression in optimized code, maybe try always doing the
> inline lookup in optimized code?
> 
> if (FLAG_use_megamorphic_stub && !is_optimizing()) {
>   ...
> } else {
>   ...
> }

There are no measureable (< 0.5% diffs) in optimized code. Only when running
with --noopt can I measure  regression.  I think the code size saving is more
significant than such a small performance loss What do you think?

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1334: }
On 2015/05/04 22:06:20, Florian Schneider wrote:
> Maybe you can factor this lookup code into a common code generation helper
that
> the stub can reuse? It would avoid the duplication between here and
> stub_code_*.cc.
> 

Will do.

[srdjan, 2015-05-05 12:13:28]

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1295: if (FLAG_use_megamorphic_stub) {
On 2015/05/05 07:15:29, srdjan wrote:
> On 2015/05/04 22:06:20, Florian Schneider wrote:
> > To avoid performance regression in optimized code, maybe try always doing
the
> > inline lookup in optimized code?
> > 
> > if (FLAG_use_megamorphic_stub && !is_optimizing()) {
> >   ...
> > } else {
> >   ...
> > }
> 
> There are no measureable (< 0.5% diffs) in optimized code. Only when running
> with --noopt can I measure  regression.  I think the code size saving is more
> significant than such a small performance loss What do you think?

Measured dart2js optimized: with out-of-line megamorphic lookup: ~4% reduction
in code size, < 1% loss in performance. Will not to add '!is_optimizing', let me
know if you disagree.

[srdjan, 2015-05-05 14:08:16]

Committed patchset #3 (id:40001) manually as r45525 (presubmit successful).

[srdjan, 2015-05-05 14:10:12]

On 2015/05/05 12:13:28, srdjan wrote:
>
https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
> File runtime/vm/flow_graph_compiler_arm.cc (right):
> 
>
https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
> runtime/vm/flow_graph_compiler_arm.cc:1295: if (FLAG_use_megamorphic_stub) {
> On 2015/05/05 07:15:29, srdjan wrote:
> > On 2015/05/04 22:06:20, Florian Schneider wrote:
> > > To avoid performance regression in optimized code, maybe try always doing
> the
> > > inline lookup in optimized code?
> > > 
> > > if (FLAG_use_megamorphic_stub && !is_optimizing()) {
> > >   ...
> > > } else {
> > >   ...
> > > }
> > 
> > There are no measureable (< 0.5% diffs) in optimized code. Only when running
> > with --noopt can I measure  regression.  I think the code size saving is
more
> > significant than such a small performance loss What do you think?
> 
> Measured dart2js optimized: with out-of-line megamorphic lookup: ~4% reduction
> in code size, < 1% loss in performance. Will not to add '!is_optimizing', let
me
> know if you disagree.

Reduction in code size measured on Instruction objects total size only.

[Florian Schneider, 2015-05-05 16:34:43]

Sounds good.

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/1125623003/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_arm.cc:1295: if (FLAG_use_megamorphic_stub) {
On 2015/05/05 12:13:28, srdjan wrote:
> On 2015/05/05 07:15:29, srdjan wrote:
> > On 2015/05/04 22:06:20, Florian Schneider wrote:
> > > To avoid performance regression in optimized code, maybe try always doing
> the
> > > inline lookup in optimized code?
> > > 
> > > if (FLAG_use_megamorphic_stub && !is_optimizing()) {
> > >   ...
> > > } else {
> > >   ...
> > > }
> > 
> > There are no measureable (< 0.5% diffs) in optimized code. Only when running
> > with --noopt can I measure  regression.  I think the code size saving is
more
> > significant than such a small performance loss What do you think?
> 
> Measured dart2js optimized: with out-of-line megamorphic lookup: ~4% reduction
> in code size, < 1% loss in performance. Will not to add '!is_optimizing', let
me
> know if you disagree.

If the difference is not noticable in optimized code, I'm totally fine with
always using the stub.