[generators] Create the fixed array holding the registers only once.

[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n

Committed: https://crrev.com/dd47dcb95ce55c043a8c0b20152f6db150325462
Cr-Commit-Position: refs/heads/master@{#35872}

[neis, 2016-04-27 11:34:11]

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

[commit-bot: I haz the power, 2016-04-27 11:34:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923253002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923253002/1

[neis, 2016-04-27 11:34:49]

Description was changed from

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simply the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

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

to

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simply the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

BUG=v8:4907
LOG=n
==========

[neis, 2016-04-27 11:35:03]

Description was changed from

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simply the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

BUG=v8:4907
LOG=n
==========

to

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

BUG=v8:4907
LOG=n
==========

[neis, 2016-04-27 11:36:29]

Description was changed from

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

BUG=v8:4907
LOG=n
==========

to

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n
==========

[commit-bot: I haz the power, 2016-04-27 11:58:32]

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

[commit-bot: I haz the power, 2016-04-27 11:58:33]

Dry run: This issue passed the CQ dry run.

[neis, 2016-04-27 11:59:27]

neis@chromium.org changed reviewers:
+ bmeurer@chromium.org, rmcilroy@chromium.org

[neis, 2016-04-27 11:59:34]

PTAL

[rmcilroy, 2016-04-27 12:20:04]

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:767:
StoreFixedArrayElementInt32Index(array, index, PlaceholderConstant());
How about just filling the array with the hole or even undefined instead of
adding the new placeholder oddball?

[Benedikt Meurer, 2016-04-27 17:46:02]

Looking good modulo the naming.

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:767:
StoreFixedArrayElementInt32Index(array, index, PlaceholderConstant());
On 2016/04/27 12:20:04, rmcilroy wrote:
> How about just filling the array with the hole or even undefined instead of
> adding the new placeholder oddball?

undefined is a valid JS value, so a bug in reading from a stale register file on
a generator would be difficult to track. Same for the_hole, which is not a JS
value, but it could also leak from other sources. Having a dedicated Oddball for
this makes it easier to at least tell that something is screwed with generators
in case you ever see this oddball somewhere else.

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

https://codereview.chromium.org/1923253002/diff/1/src/objects.h#newcode1013
src/objects.h:1013: V(Placeholder)
Can we call this guy EmptyRegister or StaleRegister or something like that?
Placeholder is not very specific.

[rmcilroy, 2016-04-28 08:05:49]

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:767:
StoreFixedArrayElementInt32Index(array, index, PlaceholderConstant());
On 2016/04/27 17:46:02, Benedikt Meurer wrote:
> On 2016/04/27 12:20:04, rmcilroy wrote:
> > How about just filling the array with the hole or even undefined instead of
> > adding the new placeholder oddball?
> 
> undefined is a valid JS value, so a bug in reading from a stale register file
on
> a generator would be difficult to track. Same for the_hole, which is not a JS
> value, but it could also leak from other sources. Having a dedicated Oddball
for
> this makes it easier to at least tell that something is screwed with
generators
> in case you ever see this oddball somewhere else.

Makes sense, SGTM with the rename you suggest.

[neis, 2016-04-28 08:30:06]

Renamed as requested.

[rmcilroy, 2016-04-28 11:40:58]

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:721: 
Maybe we could add if (FLAG_debug_code) which checks that the array is of the
correct size?

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:735: // No write barrier needed for
writing into freshly allocated object.
This is no longer the case, I guess we need a write barrier now?

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:750: 
ditto

https://codereview.chromium.org/1923253002/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1923253002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:24: int size =
function->shared()->bytecode_array()->register_count();
DCHECK(funtion->shared()->HasBytecodeArray()) ?

[neis, 2016-04-28 11:47:22]

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:721: 
On 2016/04/28 11:40:57, rmcilroy wrote:
> Maybe we could add if (FLAG_debug_code) which checks that the array is of the
> correct size?

Acknowledged. But why this flag rather than a DCHECK? (I'm not familiar with the
flag.)

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:735: // No write barrier needed for
writing into freshly allocated object.
On 2016/04/28 11:40:57, rmcilroy wrote:
> This is no longer the case, I guess we need a write barrier now?

Good catch :)

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:750: 
On 2016/04/28 11:40:57, rmcilroy wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1923253002/diff/20001/src/runtime/runtime-gen...
File src/runtime/runtime-generator.cc (right):

https://codereview.chromium.org/1923253002/diff/20001/src/runtime/runtime-gen...
src/runtime/runtime-generator.cc:24: int size =
function->shared()->bytecode_array()->register_count();
On 2016/04/28 11:40:57, rmcilroy wrote:
> DCHECK(funtion->shared()->HasBytecodeArray()) ?

That's implicit in bytecode_array().

[neis, 2016-04-28 12:05:25]

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/1923253002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:721: 
On 2016/04/28 11:47:21, neis wrote:
> On 2016/04/28 11:40:57, rmcilroy wrote:
> > Maybe we could add if (FLAG_debug_code) which checks that the array is of
the
> > correct size?
> 
> Acknowledged. But why this flag rather than a DCHECK? (I'm not familiar with
the
> flag.)

Nevermind, I got it.

[neis, 2016-04-28 12:50:29]

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

[commit-bot: I haz the power, 2016-04-28 12:50:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923253002/40001

[neis, 2016-04-28 12:50:47]

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

[neis, 2016-04-28 12:54:43]

Comments addressed. @hpayer: Please have a look at heap changes.

[rmcilroy, 2016-04-28 13:21:07]

LGTM, thanks.

[commit-bot: I haz the power, 2016-04-28 13:25:51]

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

[commit-bot: I haz the power, 2016-04-28 13:25:53]

Dry run: This issue passed the CQ dry run.

[neis, 2016-04-28 14:47:52]

neis@chromium.org changed reviewers:
+ ulan@chromium.org

[ulan, 2016-04-28 14:51:17]

heap lgtm

[neis, 2016-04-28 14:52:16]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-04-28 14:52:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1923253002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1923253002/40001

[commit-bot: I haz the power, 2016-04-28 14:54:45]

Description was changed from

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n
==========

to

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n
==========

[commit-bot: I haz the power, 2016-04-28 14:54:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-01 22:02:44]

Description was changed from

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n
==========

to

==========
[generators] Create the fixed array holding the registers only once.

Instead of replacing the array with an empty one after resuming, overwrite
contents with a new Oddball.

This will simplify the work to be done by the bytecode graphbuilder and
potentially allow for more optimization.

(For full-codegen generators, nothing changes.)

BUG=v8:4907
LOG=n

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

[commit-bot: I haz the power, 2016-05-01 22:02:45]

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