[interpreter] Compute and use type info percentage

[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884
Committed: https://crrev.com/3200fafa5fcf70c21304e735502040ccf3af2662
Cr-Commit-Position: refs/heads/master@{#39677}

[rmcilroy, 2016-09-23 08:10:33]

rmcilroy@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[rmcilroy, 2016-09-23 08:10:34]

Overall looks good - I like the perf improvements! Made some comments, please
let mvstanton take a look too.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:83: switch (reason) {
Just call OptimizationReasonToString rather than another switch?

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:118: int with = 0, gen = 0, interpreter_total = 0;
nit - type_vector_ic_count

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:124: *ic_total_count = interpreter_total;
DCHECK ic_total_count is zero beforehand

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:360: auto reason =
OptimizationHeuristicIgnition(function, frame);
Don't use auto unless it improves readability
(https://google.github.io/styleguide/cppguide.html#auto) - use
OptimizationReason here.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:380: return false;
Hmm, not keen on this. Could you just pull out the FLAG_always_osr case and
inline to the functions so it's still obvious that both OSR and
optimize/baseline happens.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:384: OptimizationReason::kDoNotOptimize) ||
Could you pull this out to a separate local for clarity

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h
File src/runtime-profiler.h (right):

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h#newc...
src/runtime-profiler.h:35: // optimization attempts should be made
fullstops on comments, and newline before the comment (also, not sure if
comments are needed here given they are private functions).

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h#newc...
src/runtime-profiler.h:39: OptimizationReason
OptimizationHeuristicIgnition(JSFunction* function,
nit - ShouldOptimizeIgnition(...) and move either above Optimise or below
Baseline

https://codereview.chromium.org/2360913003/diff/1/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/2360913003/diff/1/src/type-feedback-vector.h#...
src/type-feedback-vector.h:251: int* interpreter_total, bool
code_is_interpreted);
vector_ic_count

[klaasb, 2016-09-23 08:31:39]

klaasb@google.com changed reviewers:
+ mvstanton@chromium.org

[klaasb, 2016-09-23 08:31:40]

Hi Michael, please take a look as well.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:83: switch (reason) {
On 2016/09/23 08:10:34, rmcilroy wrote:
> Just call OptimizationReasonToString rather than another switch?

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:118: int with = 0, gen = 0, interpreter_total = 0;
On 2016/09/23 08:10:34, rmcilroy wrote:
> nit - type_vector_ic_count

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:124: *ic_total_count = interpreter_total;
On 2016/09/23 08:10:34, rmcilroy wrote:
> DCHECK ic_total_count is zero beforehand

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:360: auto reason =
OptimizationHeuristicIgnition(function, frame);
On 2016/09/23 08:10:34, rmcilroy wrote:
> Don't use auto unless it improves readability
> (https://google.github.io/styleguide/cppguide.html#auto) - use
> OptimizationReason here.

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:380: return false;
On 2016/09/23 08:10:34, rmcilroy wrote:
> Hmm, not keen on this. Could you just pull out the FLAG_always_osr case and
> inline to the functions so it's still obvious that both OSR and
> optimize/baseline happens.

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.cc#new...
src/runtime-profiler.cc:384: OptimizationReason::kDoNotOptimize) ||
On 2016/09/23 08:10:34, rmcilroy wrote:
> Could you pull this out to a separate local for clarity

Done.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h
File src/runtime-profiler.h (right):

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h#newc...
src/runtime-profiler.h:35: // optimization attempts should be made
On 2016/09/23 08:10:34, rmcilroy wrote:
> fullstops on comments, and newline before the comment (also, not sure if
> comments are needed here given they are private functions).

The comment is mostly about the intended semantics for the bool return value.

https://codereview.chromium.org/2360913003/diff/1/src/runtime-profiler.h#newc...
src/runtime-profiler.h:39: OptimizationReason
OptimizationHeuristicIgnition(JSFunction* function,
On 2016/09/23 08:10:34, rmcilroy wrote:
> nit - ShouldOptimizeIgnition(...) and move either above Optimise or below
> Baseline

Done.

https://codereview.chromium.org/2360913003/diff/1/src/type-feedback-vector.h
File src/type-feedback-vector.h (right):

https://codereview.chromium.org/2360913003/diff/1/src/type-feedback-vector.h#...
src/type-feedback-vector.h:251: int* interpreter_total, bool
code_is_interpreted);
On 2016/09/23 08:10:34, rmcilroy wrote:
> vector_ic_count

Done.

[mvstanton, 2016-09-23 11:19:03]

Right on!
LGTM

[klaasb, 2016-09-23 13:04:02]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-09-23 13:04:07]

The CQ bit was unchecked by klaasb@google.com

[rmcilroy, 2016-09-23 13:04:41]

One nit, but LGTM, thanks!

https://codereview.chromium.org/2360913003/diff/20001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):

https://codereview.chromium.org/2360913003/diff/20001/src/runtime-profiler.cc...
src/runtime-profiler.cc:380: bool optimize_before_baseline =
function->IsMarkedForBaseline() &&
nit - osr_before_baselined

[klaasb, 2016-09-23 13:05:43]

Description was changed from

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
==========

to

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884
==========

[klaasb, 2016-09-23 13:09:22]

The CQ bit was checked by klaasb@google.com

[klaasb, 2016-09-23 13:09:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2360913003/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-09-23 13:09:31]

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-09-23 15:30:24]

Description was changed from

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884
==========

to

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884
==========

[commit-bot: I haz the power, 2016-09-23 15:30:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-23 15:30:38]

Description was changed from

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884
==========

to

==========
[interpreter] Compute and use type info percentage

Previously we would not have a total count of ICs when interpreting and
thus the check for sufficient type info would always succeed.
Also use the optimization checks for OSR while waiting for baseline
compilation and refactor the check.

BUG=v8:4280
BUG=chromium:634884

Committed: https://crrev.com/3200fafa5fcf70c21304e735502040ccf3af2662
Cr-Commit-Position: refs/heads/master@{#39677}
==========

[commit-bot: I haz the power, 2016-09-23 15:30:39]

Patchset 4 (id:??) landed as
https://crrev.com/3200fafa5fcf70c21304e735502040ccf3af2662
Cr-Commit-Position: refs/heads/master@{#39677}