[modules] Maintain array of cells for imports and local exports.

[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569
Committed: https://crrev.com/21463f73e97deefe89177ce6b65a81e898c2acc6
Cr-Commit-Position: refs/heads/master@{#40808}

[adamk, 2016-11-02 18:41:58]

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

[adamk, 2016-11-02 18:41:59]

This looks reasonable overall, I think most comments I would have made already
have "XXX" comments next to them.

[neis, 2016-11-03 12:53:05]

Description was changed from

==========
WIP: [modules] Maintain array of cells for imports and local exports.

Amongst other things, this replaces LoadImport and LoadExport with a single
LoadVariable taking a variable index as argument (rather than a name).

COMMIT=false
BUG=v8:1569
==========

to

==========
[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569
==========

[neis, 2016-11-03 12:53:40]

Ready for review now.

[adamk, 2016-11-03 18:30:45]

https://codereview.chromium.org/2465283004/diff/20001/src/ast/modules.h
File src/ast/modules.h (right):

https://codereview.chromium.org/2465283004/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:115: static CellIndexKind cell_index_kind(int cell_index);
The underscore naming for a static method looks a little funny to me, maybe call
this GetCellIndexKind?

https://codereview.chromium.org/2465283004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2465283004/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19844: void Module::CreateExport(Handle<Module> module, int
index,
Curious why you sometimes say "index" and sometimes "cell_index". Is there a
distinction?

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h#newcode8179
src/objects.h:8179: // The export table.
This comment could be further fleshed out too (e.g., what are the keys and
values?)

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h#newcode8182
src/objects.h:8182: DECL_ACCESSORS(regular_exports, FixedArray)
Comments for these? Especially since "exports" and "regular_exports" next to
each other is confusing without a comment.

[neis, 2016-11-04 10:14:39]

https://codereview.chromium.org/2465283004/diff/20001/src/ast/modules.h
File src/ast/modules.h (right):

https://codereview.chromium.org/2465283004/diff/20001/src/ast/modules.h#newco...
src/ast/modules.h:115: static CellIndexKind cell_index_kind(int cell_index);
On 2016/11/03 18:30:45, adamk wrote:
> The underscore naming for a static method looks a little funny to me, maybe
call
> this GetCellIndexKind?

Done.

https://codereview.chromium.org/2465283004/diff/20001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/2465283004/diff/20001/src/objects.cc#newcode1...
src/objects.cc:19844: void Module::CreateExport(Handle<Module> module, int
index,
On 2016/11/03 18:30:45, adamk wrote:
> Curious why you sometimes say "index" and sometimes "cell_index". Is there a
> distinction?

Not here at least. Changed these to cell_index.

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h#newcode8179
src/objects.h:8179: // The export table.
On 2016/11/03 18:30:45, adamk wrote:
> This comment could be further fleshed out too (e.g., what are the keys and
> values?)

Done.

https://codereview.chromium.org/2465283004/diff/20001/src/objects.h#newcode8182
src/objects.h:8182: DECL_ACCESSORS(regular_exports, FixedArray)
On 2016/11/03 18:30:45, adamk wrote:
> Comments for these? Especially since "exports" and "regular_exports" next to
> each other is confusing without a comment.

Done.

[adamk, 2016-11-04 17:15:35]

lgtm

[neis, 2016-11-07 09:48:02]

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

[neis, 2016-11-07 09:48:13]

On 2016/11/04 17:15:35, adamk wrote:
> lgtm

+jgruber for debug/

[neis, 2016-11-07 09:49:02]

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

[neis, 2016-11-07 09:49:02]

+rmcilroy for interpreter

[jgruber, 2016-11-07 10:47:26]

lgtm for debug/

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

interpreter/ lgtm

[neis, 2016-11-07 14:55:43]

The CQ bit was checked by neis@chromium.org

[commit-bot: I haz the power, 2016-11-07 14:55:50]

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-07 14:59:03]

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

[commit-bot: I haz the power, 2016-11-07 14:59:04]

Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15743)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15708)

[neis, 2016-11-07 15:29:57]

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

[commit-bot: I haz the power, 2016-11-07 15:30:05]

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-11-07 15:34:04]

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

[commit-bot: I haz the power, 2016-11-07 15:34:05]

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

[neis, 2016-11-07 15:36:58]

Patchset #5 (id:80001) has been deleted

[neis, 2016-11-07 15:55:45]

The CQ bit was checked by neis@chromium.org

[neis, 2016-11-07 15:55:46]

The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org,
rmcilroy@chromium.org, jgruber@chromium.org
Link to the patchset: https://codereview.chromium.org/2465283004/#ps100001
(title: "Rename parameter also in header file.")

[commit-bot: I haz the power, 2016-11-07 15:55:54]

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-07 16:23:27]

Description was changed from

==========
[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569
==========

to

==========
[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569
==========

[commit-bot: I haz the power, 2016-11-07 16:23:28]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-11-17 22:24:55]

Description was changed from

==========
[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569
==========

to

==========
[modules] Maintain array of cells for imports and local exports.

This makes use of the newly introduced cell indices to speed up variable
accesses. Imports and local exports are now directly stored in (separate)
arrays. In the future, we may merge the two arrays into a single one, or
even into the module context.

This CL also replaces the LoadImport and LoadExport runtime functions with
a single LoadVariable taking a variable index as argument (rather than a
name).

BUG=v8:1569

Committed: https://crrev.com/21463f73e97deefe89177ce6b65a81e898c2acc6
Cr-Commit-Position: refs/heads/master@{#40808}
==========

[commit-bot: I haz the power, 2016-11-17 22:24:57]

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