[ast] Remove heap accesses from AST numbering

[ast] Remove heap accesses from AST numbering

Moves constant element/property array building to be deferred for
igition and on-demand for the other compilers, and splits off the
object/array literal depth/flag initialisation from the array building.

BUG=v8:5832
Review-Url: https://codereview.chromium.org/2625873009
Cr-Commit-Position: refs/heads/master@{#42362}
Committed: https://chromium.googlesource.com/v8/v8/+/b5b56e920aaaf3cc155926e0483ebcdbf8301b36

[Leszek Swirski, 2017-01-12 12:59:29]

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

[commit-bot: I haz the power, 2017-01-12 12:59:36]

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

[Leszek Swirski, 2017-01-12 13:01:05]

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

[Leszek Swirski, 2017-01-12 13:01:05]

Hey Ross, here's the first pass on this, passes tests locally.

[commit-bot: I haz the power, 2017-01-12 13:43:14]

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

[commit-bot: I haz the power, 2017-01-12 13:43:15]

Dry run: Try jobs failed on following builders:
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/18966)
  v8_linux_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng_triggered/b...)

[Leszek Swirski, 2017-01-12 14:16:25]

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

[commit-bot: I haz the power, 2017-01-12 14:16:33]

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, 2017-01-12 14:43:59]

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

[commit-bot: I haz the power, 2017-01-12 14:44:00]

Dry run: This issue passed the CQ dry run.

[Leszek Swirski, 2017-01-12 15:46:45]

Description was changed from

==========
[ast] Remove heap accesses from AST numbering

Moves constant element/property array building to be deferred for
igition and on-demand for the other compilers, and splits off the
object/array literal depth/flag initialisation from the array building.
==========

to

==========
[ast] Remove heap accesses from AST numbering

Moves constant element/property array building to be deferred for
igition and on-demand for the other compilers, and splits off the
object/array literal depth/flag initialisation from the array building.

BUG=v8:5832
==========

[rmcilroy, 2017-01-13 09:53:56]

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

[rmcilroy, 2017-01-13 09:53:58]

LGTM after comments addressed. Adding Toon for ast/

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h
File src/ast/ast-numbering.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:8: #include <stdint.h>
Unecessary?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:16: class ArrayLiteral;
Ditto?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:30: // to |object_literals| and ArrayLiterals to
|array_literals|.
Remove sentence added here?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:199: }
You need to be careful to check for minus 0.0 as well. How about making a
variant of IsUint32Double in conversions.h which takes an argument to return the
uint32 value, and just use that here?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast.cc#newcode431
src/ast/ast.cc:431: if (key->IsStringLiteral()) {
Could you add a DCHECK(key->IsPropertyName());

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast.cc#newcode564
src/ast/ast.cc:564: } else if (key->ToUint32(&element_index) && element_index !=
kMaxUInt32) {
Just for my understanding, where does the element_index != kMaxUInt32
restriction come in?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast.cc#newcode600
src/ast/ast.cc:600: DCHECK(!property->is_computed_name());
nit - could you share these initial checks with InitDepthAndFlags in a helper
function?

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:193: BytecodeArrayBuilder&
CreateArrayLiteral(size_t entry, int literal_index,
nit - constant_elements_entry (and in cc file)

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:195: BytecodeArrayBuilder&
CreateObjectLiteral(size_t entry, int literal_index,
nit - constant_properties_entry (and in cc file)

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:645: }
nit - newline

[Leszek Swirski, 2017-01-13 10:53:53]

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

[commit-bot: I haz the power, 2017-01-13 10:54:00]

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

[Leszek Swirski, 2017-01-13 10:58:09]

leszeks@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Leszek Swirski, 2017-01-13 10:58:10]

Hi Michi and Toon,

Could I get your approvals here for compiler/ and ast/, respectively?

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h
File src/ast/ast-numbering.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:8: #include <stdint.h>
On 2017/01/13 09:53:57, rmcilroy wrote:
> Unecessary?

Necessary, for uintptr_t

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:16: class ArrayLiteral;
On 2017/01/13 09:53:57, rmcilroy wrote:
> Ditto?

Removed.

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-numbering.h...
src/ast/ast-numbering.h:30: // to |object_literals| and ArrayLiterals to
|array_literals|.
On 2017/01/13 09:53:57, rmcilroy wrote:
> Remove sentence added here?

Oops, leftovers. Removed.

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-value-facto...
File src/ast/ast-value-factory.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/ast/ast-value-facto...
src/ast/ast-value-factory.h:199: }
On 2017/01/13 09:53:57, rmcilroy wrote:
> You need to be careful to check for minus 0.0 as well. How about making a
> variant of IsUint32Double in conversions.h which takes an argument to return
the
> uint32 value, and just use that here?

Done, and amended the code in objects-inl.h that I stole to call teh same
function.

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:193: BytecodeArrayBuilder&
CreateArrayLiteral(size_t entry, int literal_index,
On 2017/01/13 09:53:57, rmcilroy wrote:
> nit - constant_elements_entry (and in cc file)

Done. (Also changed CreateClosure for consistency)

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:195: BytecodeArrayBuilder&
CreateObjectLiteral(size_t entry, int literal_index,
On 2017/01/13 09:53:57, rmcilroy wrote:
> nit - constant_properties_entry (and in cc file)

Done.

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2625873009/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:645: }
On 2017/01/13 09:53:57, rmcilroy wrote:
> nit - newline

Done.

[commit-bot: I haz the power, 2017-01-13 11:28:04]

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

[commit-bot: I haz the power, 2017-01-13 11:28:05]

Dry run: Try jobs failed on following builders:
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20789)
  v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)

[Leszek Swirski, 2017-01-13 11:59:37]

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

[commit-bot: I haz the power, 2017-01-13 11:59:48]

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, 2017-01-13 12:34:46]

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

[commit-bot: I haz the power, 2017-01-13 12:34:47]

Dry run: Try jobs failed on following builders:
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_nodcheck_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng_tr...)

[Michael Starzinger, 2017-01-13 13:11:43]

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc#newcode441
src/compiler.cc:441: if
(!AstNumbering::Renumber(parse_info->isolate()->stack_guard()->climit(),
I think we want s/climit/real_climit/ here. IIUC then we only want the visitor
to fail if we really run out of stack and not because we are attempting to
interrupt. WDYT?

[Leszek Swirski, 2017-01-13 15:49:39]

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

[commit-bot: I haz the power, 2017-01-13 15:49:42]

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

[rmcilroy, 2017-01-13 16:00:32]

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc#newcode441
src/compiler.cc:441: if
(!AstNumbering::Renumber(parse_info->isolate()->stack_guard()->climit(),
On 2017/01/13 13:11:43, Michael Starzinger wrote:
> I think we want s/climit/real_climit/ here. IIUC then we only want the visitor
> to fail if we really run out of stack and not because we are attempting to
> interrupt. WDYT?

Good catch, +1 we want real_climit

[Leszek Swirski, 2017-01-13 16:06:51]

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

[commit-bot: I haz the power, 2017-01-13 16:06:59]

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

[Leszek Swirski, 2017-01-13 16:13:09]

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/2625873009/diff/60001/src/compiler.cc#newcode441
src/compiler.cc:441: if
(!AstNumbering::Renumber(parse_info->isolate()->stack_guard()->climit(),
On 2017/01/13 16:00:32, rmcilroy wrote:
> On 2017/01/13 13:11:43, Michael Starzinger wrote:
> > I think we want s/climit/real_climit/ here. IIUC then we only want the
visitor
> > to fail if we really run out of stack and not because we are attempting to
> > interrupt. WDYT?
> 
> Good catch, +1 we want real_climit

Done.

[commit-bot: I haz the power, 2017-01-13 16:39:16]

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

[commit-bot: I haz the power, 2017-01-13 16:39:17]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-01-16 09:08:05]

LGTM on the compiler plumbing.

https://codereview.chromium.org/2625873009/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (left):

https://codereview.chromium.org/2625873009/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1614: // If constant properties is an
empty fixed array, use our cached
nit: We could preserve this comment explaining why we special-case the
{properties_count == 0} case.

[Toon Verwaest, 2017-01-16 09:30:54]

ast lgtm

https://codereview.chromium.org/2625873009/diff/100001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2625873009/diff/100001/src/ast/ast.cc#newcode773
src/ast/ast.cc:773: DCHECK(depth() >= 1);  // Depth should be initialized.
DCHECK_LE(1, depth());

[rmcilroy, 2017-01-16 09:37:21]

LGTM, thanks.

[Leszek Swirski, 2017-01-16 10:54:48]

The CQ bit was checked by leszeks@chromium.org

[Leszek Swirski, 2017-01-16 10:54:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rmcilroy@chromium.org, verwaest@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2625873009/#ps120001
(title: "Address nits")

[commit-bot: I haz the power, 2017-01-16 10:54:59]

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, 2017-01-16 11:25:07]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484564088229050,
"parent_rev": "f40fdd1f3be26cac6f15e3849ff2acd8e86a5772", "commit_rev":
"b5b56e920aaaf3cc155926e0483ebcdbf8301b36"}

[commit-bot: I haz the power, 2017-01-16 11:25:11]

Description was changed from

==========
[ast] Remove heap accesses from AST numbering

Moves constant element/property array building to be deferred for
igition and on-demand for the other compilers, and splits off the
object/array literal depth/flag initialisation from the array building.

BUG=v8:5832
==========

to

==========
[ast] Remove heap accesses from AST numbering

Moves constant element/property array building to be deferred for
igition and on-demand for the other compilers, and splits off the
object/array literal depth/flag initialisation from the array building.

BUG=v8:5832

Review-Url: https://codereview.chromium.org/2625873009
Cr-Commit-Position: refs/heads/master@{#42362}
Committed:
https://chromium.googlesource.com/v8/v8/+/b5b56e920aaaf3cc155926e0483ebcdbf83...
==========

[commit-bot: I haz the power, 2017-01-16 11:25:12]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/b5b56e920aaaf3cc155926e0483ebcdbf83...

[Leszek Swirski, 2017-01-16 12:01:11]

Thanks all!

https://codereview.chromium.org/2625873009/diff/100001/src/ast/ast.cc
File src/ast/ast.cc (right):

https://codereview.chromium.org/2625873009/diff/100001/src/ast/ast.cc#newcode773
src/ast/ast.cc:773: DCHECK(depth() >= 1);  // Depth should be initialized.
On 2017/01/16 09:30:54, Toon Verwaest wrote:
> DCHECK_LE(1, depth());

Done.

https://codereview.chromium.org/2625873009/diff/100001/src/interpreter/byteco...
File src/interpreter/bytecode-generator.cc (left):

https://codereview.chromium.org/2625873009/diff/100001/src/interpreter/byteco...
src/interpreter/bytecode-generator.cc:1614: // If constant properties is an
empty fixed array, use our cached
On 2017/01/16 09:08:05, Michael Starzinger wrote:
> nit: We could preserve this comment explaining why we special-case the
> {properties_count == 0} case.

Done.