[interpreter] Add OSR nesting level to bytecode header.

[interpreter] Add OSR nesting level to bytecode header.

This adds a new field to the header of every BytecodeArray which stores
the current nesting level up to which loop back edges are armed as OSR
points. The intention is to arm OSR points incrementally from outermost
to innermost until one fires (similar to OSR from FullCodegen).

R=rmcilroy@chromium.org
BUG=v8:4764
Committed: https://crrev.com/b54e49ae49f162fbe3e46eaa8d9ba89ab0de2411
Cr-Commit-Position: refs/heads/master@{#38017}

[Michael Starzinger, 2016-07-21 09:47:01]

The CQ bit was checked by mstarzinger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 09:47:12]

Dry run: 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-21 10:11:46]

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

[commit-bot: I haz the power, 2016-07-21 10:11:47]

Dry run: This issue passed the CQ dry run.

[Hannes Payer (out of office), 2016-07-21 19:06:36]

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

[Hannes Payer (out of office), 2016-07-21 19:06:36]

heap lgtm

[rmcilroy, 2016-07-22 09:46:01]

LGTM with two optional suggestions.

https://codereview.chromium.org/2172583002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects-inl.h#newcode4104
src/objects-inl.h:4104: DCHECK(0 <= depth && depth <=
Code::kMaxLoopNestingMarker);
Probably not worth the churn, but maybe we could move
Code::kMaxLoopNestingMarker to AbstractCode?

https://codereview.chromium.org/2172583002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects.h#newcode4562
src/objects.h:4562: static const int kHeaderSize = kOSRNestingLevelOffset +
kIntSize;
The OSR nesting level is always less than a byte (<=6), and the interrupt budget
is set to 0x1800 by flags, so less than two bytes. Could we pack these to avoid
increasing the size of the bytecode array?

[Michael Starzinger, 2016-07-25 10:24:42]

Comments addressed.

https://codereview.chromium.org/2172583002/diff/1/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects-inl.h#newcode4104
src/objects-inl.h:4104: DCHECK(0 <= depth && depth <=
Code::kMaxLoopNestingMarker);
On 2016/07/22 09:46:01, rmcilroy wrote:
> Probably not worth the churn, but maybe we could move
> Code::kMaxLoopNestingMarker to AbstractCode?

Done.

https://codereview.chromium.org/2172583002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects.h#newcode4562
src/objects.h:4562: static const int kHeaderSize = kOSRNestingLevelOffset +
kIntSize;
On 2016/07/22 09:46:01, rmcilroy wrote:
> The OSR nesting level is always less than a byte (<=6), and the interrupt
budget
> is set to 0x1800 by flags, so less than two bytes. Could we pack these to
avoid
> increasing the size of the bytecode array? 

As discussed offline: Yes, the OSR nesting level is fully bounded by
kMaxNestingLevel (i.e. <= 6). But the interrupt budget is not necessarily
bounded by the initial value, because forward branches will end up increasing
the value temporarily. The budget computation would need to truncate it to the
correct size. The following is an example of how I can imagine the value running
out of bounds (i.e. 32 * 0x1800 + 1000 * n) ...

function f() {
  if (x == 0) {
    // very large block containing n bytes of bytecode.
  }
  f(x - 1)
}
f(1000);

[rmcilroy, 2016-07-25 11:14:47]

Still LGTM, thanks.

https://codereview.chromium.org/2172583002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects.h#newcode4562
src/objects.h:4562: static const int kHeaderSize = kOSRNestingLevelOffset +
kIntSize;
On 2016/07/25 10:24:42, Michael Starzinger wrote:
> On 2016/07/22 09:46:01, rmcilroy wrote:
> > The OSR nesting level is always less than a byte (<=6), and the interrupt
> budget
> > is set to 0x1800 by flags, so less than two bytes. Could we pack these to
> avoid
> > increasing the size of the bytecode array? 
> 
> As discussed offline: Yes, the OSR nesting level is fully bounded by
> kMaxNestingLevel (i.e. <= 6). But the interrupt budget is not necessarily
> bounded by the initial value, because forward branches will end up increasing
> the value temporarily. The budget computation would need to truncate it to the
> correct size. The following is an example of how I can imagine the value
running
> out of bounds (i.e. 32 * 0x1800 + 1000 * n) ...
> 
> function f() {
>   if (x == 0) {
>     // very large block containing n bytes of bytecode.
>   }
>   f(x - 1)
> }
> f(1000);

Yeah this is an issue I hadn't thought of.  We could also potentially merge it
with parameter_count (since we seem to have a limit of 2^16 arguments in V8) but
it might not be worthwhile. Could you add a TODO(rmcilroy) in case we decide we
want to trim the size of bytecode arrays.

[Michael Starzinger, 2016-07-25 11:21:10]

The CQ bit was checked by mstarzinger@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-25 11:21:19]

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

[Michael Starzinger, 2016-07-25 11:21:28]

Thanks. Added TODO. Landing.

https://codereview.chromium.org/2172583002/diff/1/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2172583002/diff/1/src/objects.h#newcode4562
src/objects.h:4562: static const int kHeaderSize = kOSRNestingLevelOffset +
kIntSize;
On 2016/07/25 11:14:47, rmcilroy wrote:
> On 2016/07/25 10:24:42, Michael Starzinger wrote:
> > On 2016/07/22 09:46:01, rmcilroy wrote:
> > > The OSR nesting level is always less than a byte (<=6), and the interrupt
> > budget
> > > is set to 0x1800 by flags, so less than two bytes. Could we pack these to
> > avoid
> > > increasing the size of the bytecode array? 
> > 
> > As discussed offline: Yes, the OSR nesting level is fully bounded by
> > kMaxNestingLevel (i.e. <= 6). But the interrupt budget is not necessarily
> > bounded by the initial value, because forward branches will end up
increasing
> > the value temporarily. The budget computation would need to truncate it to
the
> > correct size. The following is an example of how I can imagine the value
> running
> > out of bounds (i.e. 32 * 0x1800 + 1000 * n) ...
> > 
> > function f() {
> >   if (x == 0) {
> >     // very large block containing n bytes of bytecode.
> >   }
> >   f(x - 1)
> > }
> > f(1000);
> 
> Yeah this is an issue I hadn't thought of.  We could also potentially merge it
> with parameter_count (since we seem to have a limit of 2^16 arguments in V8)
but
> it might not be worthwhile. Could you add a TODO(rmcilroy) in case we decide
we
> want to trim the size of bytecode arrays.

Done.

[commit-bot: I haz the power, 2016-07-25 11:49:09]

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

[commit-bot: I haz the power, 2016-07-25 11:49:10]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2016-07-25 12:17:17]

The CQ bit was checked by mstarzinger@chromium.org

[Michael Starzinger, 2016-07-25 12:17:17]

The patchset sent to the CQ was uploaded after l-g-t-m from hpayer@chromium.org,
rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2172583002/#ps40001
(title: "Add TODO.")

[commit-bot: I haz the power, 2016-07-25 12:17:26]

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-25 12:19:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-25 12:22:49]

Description was changed from

==========
[interpreter] Add OSR nesting level to bytecode header.

This adds a new field to the header of every BytecodeArray which stores
the current nesting level up to which loop back edges are armed as OSR
points. The intention is to arm OSR points incrementally from outermost
to innermost until one fires (similar to OSR from FullCodegen).

R=rmcilroy@chromium.org
BUG=v8:4764
==========

to

==========
[interpreter] Add OSR nesting level to bytecode header.

This adds a new field to the header of every BytecodeArray which stores
the current nesting level up to which loop back edges are armed as OSR
points. The intention is to arm OSR points incrementally from outermost
to innermost until one fires (similar to OSR from FullCodegen).

R=rmcilroy@chromium.org
BUG=v8:4764

Committed: https://crrev.com/b54e49ae49f162fbe3e46eaa8d9ba89ab0de2411
Cr-Commit-Position: refs/heads/master@{#38017}
==========

[commit-bot: I haz the power, 2016-07-25 12:22:50]

Patchset 3 (id:??) landed as
https://crrev.com/b54e49ae49f162fbe3e46eaa8d9ba89ab0de2411
Cr-Commit-Position: refs/heads/master@{#38017}