Added flag --inline_smi_string_hashcode and --inline_smi_string_hashcode_ratio to decide when/if sh…

Added flag --inline_smi_string_hashcode and --inline_smi_string_hashcode_ratio to decide when/if should Smi/OnebyteString hashcode be inlined before calling megamorphic. Improve code as suggested by zra.

Added flag --display_sorted_ic_data which displays ICData as sorted by count, unary ICData (e.g., in disassembly).

BUG=
R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/4ea8b39157b9c22fd11084a5ebd1744541fd72e9

[srdjan, 2016-04-26 23:25:02]

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

[zra, 2016-04-27 16:28:17]

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:165: if (!FLAG_inline_smi_string_hashcode)
return false;
Please add curly braces. Maybe also merge with the if below that also returns
false.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:168: if (ic_data.IsNull() ||
(ic_data.NumberOfChecks() == 0)) return true;
ditto.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:169: // Check if OneByteString is hot enough.
Sorry if I'm confused. So Smi hashcode will only be inlined if OneByteString is
hot?

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:174: const intptr_t ratio =
ic_data_sorted.GetCountAt(0) * 100 / total_count;
Maybe add parens around the multiply.

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1309: __ movl(EAX, EBX);  // Move Smi
hashcode to EAX.
Smi hash code looks really easy to calculate =) Have you tried always inlining
it? Why is inlining of Smi and String coupled like this?

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:1338: __ beq(CMPRES1, ZR, &done);  // Is
Smi.
The mov can go in the branch delay slot:

__ beq(CMPRES1, ZR, &done);
__ delay_slot()->mov(V0, T0);

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc
File runtime/vm/il_printer.cc (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:198: intptr_t num_checks_to_print = -1) {
intptr_t num_checks_to_print = FlowGraphPrinter::kPrintAll or just pass
kPrintAll at the <5 call-sites.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:212: if ((num_checks_to_print < 0) ||
Then, this would be (num_checks_to_print == FlowGraphPrinter::kPrintAll)

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:482: PrintICDataHelper(f, ic_data());
Should the flag cause the others to be sorted, too?

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:983: PrintICDataHelper(f, unary_checks());
ditto

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.h
File runtime/vm/il_printer.h (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.h#new...
runtime/vm/il_printer.h:63: intptr_t num_checks_to_print = -1);
It looks like this function only previously had one call-site. Maybe instead of
using a default parameter we could have a:

static const intptr_t kPrintAll = -1;

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1964: // Smi not first!! Used for printing and
optimizations.
Could you explain a bit more what "Smi not first" means?

[srdjan, 2016-04-27 18:10:22]

Thanks, PTAL

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:165: if (!FLAG_inline_smi_string_hashcode)
return false;
On 2016/04/27 16:28:17, zra wrote:
> Please add curly braces. Maybe also merge with the if below that also returns
> false.

Done.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:168: if (ic_data.IsNull() ||
(ic_data.NumberOfChecks() == 0)) return true;
On 2016/04/27 16:28:17, zra wrote:
> ditto.

Done.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:169: // Check if OneByteString is hot enough.
On 2016/04/27 16:28:17, zra wrote:
> Sorry if I'm confused. So Smi hashcode will only be inlined if OneByteString
is
> hot?

Yes, because Smi test must be done anyway.  Added comment TODO.

Also added 'lost' check that the hottest class is OnebyteString,

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler.cc:174: const intptr_t ratio =
ic_data_sorted.GetCountAt(0) * 100 / total_count;
On 2016/04/27 16:28:17, zra wrote:
> Maybe add parens around the multiply.

Done.

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_ia32.cc:1309: __ movl(EAX, EBX);  // Move Smi
hashcode to EAX.
On 2016/04/27 16:28:17, zra wrote:
> Smi hash code looks really easy to calculate =) Have you tried always inlining
> it? Why is inlining of Smi and String coupled like this?

In order to get the cid we must check for Smi anyway. As you see. the hashcode
is almost free, so inlining it anyway. 
Added a TODO (elsewhere), to check for Smi hotness and emit Smi check only.
Maybe I'll have time to implement it in a next CL. Adding the check always would
increase code size unnecessarily.

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

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/flow_graph_compi...
runtime/vm/flow_graph_compiler_mips.cc:1338: __ beq(CMPRES1, ZR, &done);  // Is
Smi.
On 2016/04/27 16:28:17, zra wrote:
> The mov can go in the branch delay slot:
> 
> __ beq(CMPRES1, ZR, &done);
> __ delay_slot()->mov(V0, T0);

Done.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc
File runtime/vm/il_printer.cc (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:198: intptr_t num_checks_to_print = -1) {
On 2016/04/27 16:28:17, zra wrote:
> intptr_t num_checks_to_print = FlowGraphPrinter::kPrintAll or just pass
> kPrintAll at the <5 call-sites.

Done.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:212: if ((num_checks_to_print < 0) ||
On 2016/04/27 16:28:17, zra wrote:
> Then, this would be (num_checks_to_print == FlowGraphPrinter::kPrintAll)

Done.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.cc#ne...
runtime/vm/il_printer.cc:482: PrintICDataHelper(f, ic_data());
On 2016/04/27 16:28:17, zra wrote:
> Should the flag cause the others to be sorted, too?

Done. Originally planned it only for megamorphic calls (InstanceCallInstr),
adding it to others as it could be useful.

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.h
File runtime/vm/il_printer.h (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/il_printer.h#new...
runtime/vm/il_printer.h:63: intptr_t num_checks_to_print = -1);
On 2016/04/27 16:28:17, zra wrote:
> It looks like this function only previously had one call-site. Maybe instead
of
> using a default parameter we could have a:
> 
> static const intptr_t kPrintAll = -1;

Using kPrintAll, but leaving it as default as I often use PrintICData as a
temporary debugging call and do not want to pass kPrintAll or -1 (from lldb).

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1920103004/diff/1/runtime/vm/object.h#newcode...
runtime/vm/object.h:1964: // Smi not first!! Used for printing and
optimizations.
On 2016/04/27 16:28:17, zra wrote:
> Could you explain a bit more what "Smi not first" means? 

Added comment that the convention for ICData used in code generation is that Smi
check is first.

[zra, 2016-04-27 18:19:57]

lgtm

[srdjan, 2016-04-27 20:28:06]

Description was changed from

==========
Added flag --inline_smi_string_hashcode and --inline_smi_string_hashcode_ratio
to decide when/if should Smi/OnebyteString hashcode be inlined before calling
megamorphic. Improve code as suggested by zra.

Added flag --display_sorted_ic_data which displays ICData as sorted by count,
unary ICData (e.g., in disassembly).

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

to

==========
Added flag --inline_smi_string_hashcode and --inline_smi_string_hashcode_ratio
to decide when/if should Smi/OnebyteString hashcode be inlined before calling
megamorphic. Improve code as suggested by zra.

Added flag --display_sorted_ic_data which displays ICData as sorted by count,
unary ICData (e.g., in disassembly).

BUG=
R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4ea8b39157b9c22fd11084a5ebd1744541fd72e9
==========

[srdjan, 2016-04-27 20:28:08]

Committed patchset #2 (id:20001) manually as
4ea8b39157b9c22fd11084a5ebd1744541fd72e9 (presubmit successful).