Thread decls-list through Declaration

Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one additional pointer per entry, rather than an average of 2 per entry for larger-than-4 element lists. For temp zone parsed functions it additionally makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a list.

BUG=v8:5209
Committed: https://crrev.com/5a18685e08f2d2efc8c9ad44afc5adec8e42a4c8
Cr-Commit-Position: refs/heads/master@{#40703}

[Toon Verwaest, 2016-10-31 14:22:06]

Description was changed from

==========
Thread decls-list through Declaration

BUG=v8:5209
==========

to

==========
Thread decls-list through Declaration using a ThreadedList

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

[Toon Verwaest, 2016-10-31 14:23:01]

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

[Toon Verwaest, 2016-10-31 14:23:02]

ptal; I'll add more owners if you think this goes in the right direction

[Toon Verwaest, 2016-10-31 14:27:17]

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

[commit-bot: I haz the power, 2016-10-31 14:27:27]

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-10-31 14:36:43]

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

[commit-bot: I haz the power, 2016-10-31 14:36:44]

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

[Toon Verwaest, 2016-10-31 14:47:04]

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

[commit-bot: I haz the power, 2016-10-31 14:47:15]

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

[Toon Verwaest, 2016-10-31 15:45:51]

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

[commit-bot: I haz the power, 2016-10-31 15:45:58]

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

[Toon Verwaest, 2016-10-31 15:48:19]

verwaest@chromium.org changed reviewers:
+ adamk@chromium.org, ahaas@chromium.org

[Toon Verwaest, 2016-10-31 15:48:20]

ptal
adamk: ptal ast/parsing
ahaas: ptal asmjs

[commit-bot: I haz the power, 2016-10-31 16:16:06]

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

[commit-bot: I haz the power, 2016-10-31 16:16:07]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-31 18:33:08]

Haven't looked at the code changes yet, but one thing that would help me
evaluate this change is more info in the CL description: why this change? what
is the impact (on memory usage, presumably)?

[Toon Verwaest, 2016-10-31 19:34:21]

Description was changed from

==========
Thread decls-list through Declaration using a ThreadedList

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

to

==========
Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one
additional pointer per entry, rather than an average of 2 per entry for
larger-than-4 element lists. For temp zone parsed functions it additionally
makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

[Toon Verwaest, 2016-10-31 19:34:47]

Added more info.

[adamk, 2016-10-31 20:34:23]

Overall seems like a reasonable win, a few questions below but happy to lgtm for
my directories.

https://codereview.chromium.org/2457393003/diff/160001/src/ast/ast.h
File src/ast/ast.h (right):

https://codereview.chromium.org/2457393003/diff/160001/src/ast/ast.h#newcode516
src/ast/ast.h:516: Declaration** next() { return &next_; }
Should this be called anywhere other than inside ThreadedList<Declaration>?
Perhaps this should be private and that class should be a friend?

Alternatively add a comment noting that this is just to implement List.

https://codereview.chromium.org/2457393003/diff/160001/src/utils.h
File src/utils.h (right):

https://codereview.chromium.org/2457393003/diff/160001/src/utils.h#newcode1610
src/utils.h:1610: tail_ = v->next();
Should Add() be DCHECKing that *v->next() is null here? Or maybe in the Snapshot
case that would fail?

[ahaas, 2016-11-02 07:59:57]

On 2016/10/31 at 20:34:23, adamk wrote:
> Overall seems like a reasonable win, a few questions below but happy to lgtm
for my directories.
> 
> https://codereview.chromium.org/2457393003/diff/160001/src/ast/ast.h
> File src/ast/ast.h (right):
> 
>
https://codereview.chromium.org/2457393003/diff/160001/src/ast/ast.h#newcode516
> src/ast/ast.h:516: Declaration** next() { return &next_; }
> Should this be called anywhere other than inside ThreadedList<Declaration>?
Perhaps this should be private and that class should be a friend?
> 
> Alternatively add a comment noting that this is just to implement List.
> 
> https://codereview.chromium.org/2457393003/diff/160001/src/utils.h
> File src/utils.h (right):
> 
> https://codereview.chromium.org/2457393003/diff/160001/src/utils.h#newcode1610
> src/utils.h:1610: tail_ = v->next();
> Should Add() be DCHECKing that *v->next() is null here? Or maybe in the
Snapshot case that would fail?

wasm lgtm

[ahaas, 2016-11-02 08:02:18]

https://codereview.chromium.org/2457393003/diff/160001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2457393003/diff/160001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:650: Declaration::List* decls = scope->declarations();
Nit: Why do you store scope->declarations() in a local variable here, but below
in line 1159 you don't?

[Michael Starzinger, 2016-11-02 10:47:36]

LGTM on the back-ends.

[Toon Verwaest, 2016-11-02 13:37:15]

Patchset #11 (id:200001) has been deleted

[Toon Verwaest, 2016-11-02 13:39:41]

Addressed comments.

https://codereview.chromium.org/2457393003/diff/160001/src/asmjs/asm-typer.cc
File src/asmjs/asm-typer.cc (right):

https://codereview.chromium.org/2457393003/diff/160001/src/asmjs/asm-typer.cc...
src/asmjs/asm-typer.cc:650: Declaration::List* decls = scope->declarations();
On 2016/11/02 08:02:18, ahaas wrote:
> Nit: Why do you store scope->declarations() in a local variable here, but
below
> in line 1159 you don't?

Because it's used twice here but not below :) Or also: I didn't change it?

[Toon Verwaest, 2016-11-02 13:41:36]

The CQ bit was checked by verwaest@chromium.org

[Toon Verwaest, 2016-11-02 13:41:37]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org,
adamk@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2457393003/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-11-02 13:41:43]

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-11-02 14:08:38]

Description was changed from

==========
Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one
additional pointer per entry, rather than an average of 2 per entry for
larger-than-4 element lists. For temp zone parsed functions it additionally
makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

to

==========
Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one
additional pointer per entry, rather than an average of 2 per entry for
larger-than-4 element lists. For temp zone parsed functions it additionally
makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

[commit-bot: I haz the power, 2016-11-02 14:08:39]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-11-17 22:19:31]

Description was changed from

==========
Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one
additional pointer per entry, rather than an average of 2 per entry for
larger-than-4 element lists. For temp zone parsed functions it additionally
makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209
==========

to

==========
Thread decls-list through Declaration using a ThreadedList

This reduces per-scope overhead from minimally 6 words to 2 words, with one
additional pointer per entry, rather than an average of 2 per entry for
larger-than-4 element lists. For temp zone parsed functions it additionally
makes the declaration-list actually freeable.

This introduces ThreadedList to implement the details of dealing with such a
list.

BUG=v8:5209

Committed: https://crrev.com/5a18685e08f2d2efc8c9ad44afc5adec8e42a4c8
Cr-Commit-Position: refs/heads/master@{#40703}
==========

[commit-bot: I haz the power, 2016-11-17 22:19:33]

Patchset 11 (id:??) landed as
https://crrev.com/5a18685e08f2d2efc8c9ad44afc5adec8e42a4c8
Cr-Commit-Position: refs/heads/master@{#40703}