Scale invocation count by the number of BBs in the flow-graph of a method.

Scale invocation count by the number of BBs in the flow-graph of a method.

BUG=
R=vegorov@google.com

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

[Anders Johnsen, 2014-08-28 08:34:07]

ajohnsen@google.com changed reviewers:
+ fschneider@chromium.org, vegorov@google.com

[Anders Johnsen, 2014-08-28 08:34:07]

Slava, Florian, please add other reviews as you see fit. If it's approved, I'll
port to the other targets.

[Vyacheslav Egorov (Google), 2014-08-28 14:26:40]

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

[Vyacheslav Egorov (Google), 2014-08-28 14:26:40]

I think this heuristic tweak makes sense to me. 

WDYT, Srdjan?

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)

[Anders Johnsen, 2014-08-28 14:44:16]

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
On 2014/08/28 14:26:40, Vyacheslav Egorov (Google) wrote:
> Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)

I'm not sure - where do you get that "magic number 100" from??? :)

[srdjan, 2014-08-28 16:49:16]

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
On 2014/08/28 14:44:16, Anders Johnsen wrote:
> On 2014/08/28 14:26:40, Vyacheslav Egorov (Google) wrote:
> > Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)
> 
> I'm not sure - where do you get that "magic number 100" from??? :)

The number is definitely wrong, it must be a typo and should be 2718.

[srdjan, 2014-08-28 17:39:10]

I also think this CL should make sense. What about OSR threshold? Would it be
also worthwhile to trigger OSR earlier for small loops/methods?

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
On 2014/08/28 16:49:16, srdjan wrote:
> On 2014/08/28 14:44:16, Anders Johnsen wrote:
> > On 2014/08/28 14:26:40, Vyacheslav Egorov (Google) wrote:
> > > Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)
> > 
> > I'm not sure - where do you get that "magic number 100" from??? :)
> 
> The number is definitely wrong, it must be a typo and should be 2718.

On a serious note: do specific number (2270) looks fragile? Do 2000 or 3000 make
a lot of difference?

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:35: "The minimum invocation count for a
function.");
Maybe rename to min_optimization_counter_threshold?

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1007: intptr_t basic_blocks =
flow_graph().preorder().length();
const

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1010: FLAG_optimization_counter_start;
const

[Vyacheslav Egorov (Google), 2014-08-28 18:06:49]

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
I would like to clarify where 2270 came from: originally Anders suggested
quadratic scaling a*x^2 + b * x + c which did not make much sense to me --- so I
suggested to use 2270 instead which is the tangent of the line which intersects
lines x = 0 and y = 30000 in the same points as Anders'es parabola (essentially
it's a linear approximation of the same quadratic scaling which actually grows
faster on this interval).



On 2014/08/28 17:39:10, srdjan wrote:
> On 2014/08/28 16:49:16, srdjan wrote:
> > On 2014/08/28 14:44:16, Anders Johnsen wrote:
> > > On 2014/08/28 14:26:40, Vyacheslav Egorov (Google) wrote:
> > > > Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)
> > > 
> > > I'm not sure - where do you get that "magic number 100" from??? :)
> > 
> > The number is definitely wrong, it must be a typo and should be 2718.
> 
> On a serious note: do specific number (2270) looks fragile? Do 2000 or 3000
make
> a lot of difference?

[Anders Johnsen, 2014-08-29 07:11:38]

Changed

2270->2000 (results appear very similar)
Refactored out the threshold.
Ported to all archs.

PTAL.

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:32: DEFINE_FLAG(int,
optimization_counter_scale, 2270,
On 2014/08/28 18:06:49, Vyacheslav Egorov (Google) wrote:
> I would like to clarify where 2270 came from: originally Anders suggested
> quadratic scaling a*x^2 + b * x + c which did not make much sense to me --- so
I
> suggested to use 2270 instead which is the tangent of the line which
intersects
> lines x = 0 and y = 30000 in the same points as Anders'es parabola
(essentially
> it's a linear approximation of the same quadratic scaling which actually grows
> faster on this interval).
> 
> 
> 
> On 2014/08/28 17:39:10, srdjan wrote:
> > On 2014/08/28 16:49:16, srdjan wrote:
> > > On 2014/08/28 14:44:16, Anders Johnsen wrote:
> > > > On 2014/08/28 14:26:40, Vyacheslav Egorov (Google) wrote:
> > > > > Maybe use 100 * Math.pow(Math.PI, Math.E) here? :)
> > > > 
> > > > I'm not sure - where do you get that "magic number 100" from??? :)
> > > 
> > > The number is definitely wrong, it must be a typo and should be 2718.
> > 
> > On a serious note: do specific number (2270) looks fragile? Do 2000 or 3000
> make
> > a lot of difference? 
> 

Yes, the quadratic scale came from Karl and I experimenting, but as Slava found,
the curve was very close to a line for our use-case range. I've found 2000 to
work just as well.

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:35: "The minimum invocation count for a
function.");
On 2014/08/28 17:39:10, srdjan wrote:
> Maybe rename to min_optimization_counter_threshold?

Done.

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1007: intptr_t basic_blocks =
flow_graph().preorder().length();
On 2014/08/28 17:39:10, srdjan wrote:
> const

Done.

https://codereview.chromium.org/501553005/diff/60001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1010: FLAG_optimization_counter_start;
On 2014/08/28 17:39:10, srdjan wrote:
> const

Cannot, I may reassign below.

[kasperl, 2014-08-29 10:52:26]

kasperl@google.com changed reviewers:
+ kasperl@google.com

[kasperl, 2014-08-29 10:52:26]

Clearly you meant 2269.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:38: DEFINE_FLAG(int, optimization_counter_scale,
2000,
2269

[Vyacheslav Egorov (Google), 2014-08-29 11:17:19]

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:40: DEFINE_FLAG(int,
min_optimization_counter_threshold, 5000,
Just move these flags to the file where they are used from, intead of defining
them here and declaring them there.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:1434: 
+ empty line

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:986: } else {
else -> if (!is_optimizing()) move comment above it.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm64.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm64.cc:983: } else {
ditto

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:999: __ cmpl(FieldAddress(function_reg,
Function::usage_counter_offset()),
Move comparison out of the if() [it is duplicated now]

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1001: } else {
else -> if (!is_optimizing()), leave only incl inside the if

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_mips.cc:1011: if (is_optimizing()) {
ditto like on arm.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_x64.cc:1008: if (is_optimizing()) {
ditto like on ia32

[Anders Johnsen, 2014-08-29 11:56:28]

PTAL

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/code_generato...
runtime/vm/code_generator.cc:40: DEFINE_FLAG(int,
min_optimization_counter_threshold, 5000,
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> Just move these flags to the file where they are used from, intead of defining
> them here and declaring them there.

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler.cc:1434: 
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> + empty line

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm.cc:986: } else {
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> else -> if (!is_optimizing()) move comment above it.

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_arm64.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_arm64.cc:983: } else {
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> ditto

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:999: __ cmpl(FieldAddress(function_reg,
Function::usage_counter_offset()),
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> Move comparison out of the if() [it is duplicated now]

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_ia32.cc:1001: } else {
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> else -> if (!is_optimizing()), leave only incl inside the if

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_mips.cc:1011: if (is_optimizing()) {
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> ditto like on arm.

Done.

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/501553005/diff/80001/runtime/vm/flow_graph_co...
runtime/vm/flow_graph_compiler_x64.cc:1008: if (is_optimizing()) {
On 2014/08/29 11:17:18, Vyacheslav Egorov (Google) wrote:
> ditto like on ia32

Done.

[Vyacheslav Egorov (Google), 2014-08-29 12:16:59]

lgtm

[Anders Johnsen, 2014-08-29 12:51:18]

Committed patchset #6 (id:100001) manually as 39698 (presubmit successful).