[modules] Store Module metadata in per-Context EmbedderData

[modules] Store Module metadata in per-Context EmbedderData

Unifies the approaches used for storing the specifier -> module mapping
and the module -> directory mapping, using std::unordered_maps for both
and storing them per-Context.

This requires adding a method to the v8::Module API to get a hash code
for a Module, but allows slimming down the API in return: gone are
SetEmbedderData/GetEmbedderData, along with the fourth argument
to ResolveModuleCallback.

Besides a simpler API, this allows d8 to get closer to the HTML loader,
which requires each Realm to have a persistent module map (though this
capability is not yet exercised by any tests).

BUG=v8:1569
Committed: https://crrev.com/9cf8fce74cf6e7afd6aea3f3545f6bb61572f277
Cr-Commit-Position: refs/heads/master@{#40133}

[adamk, 2016-10-05 21:46:31]

adamk@chromium.org changed reviewers:
+ jochen@chromium.org, neis@chromium.org

[adamk, 2016-10-05 21:46:31]

neis, please look at everything here.
jochen, I want to make sure it's OK to leak the embedder data in d8, ptal.

[adamk, 2016-10-05 21:46:36]

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

[commit-bot: I haz the power, 2016-10-05 21:46:47]

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-05 22:21:45]

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

[commit-bot: I haz the power, 2016-10-05 22:21:46]

Dry run: This issue passed the CQ dry run.

[neis, 2016-10-06 09:44:47]

Looks mostly good, but I'd like to talk offline about an idea I have that seems
related to this.

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode582
src/d8.cc:582: // Map from Module to the directory that Module was loaded from.
Nit: keep the comments both in singular or both in plural.

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode588
src/d8.cc:588: kDebugIdIndex = Context::kDebugIdIndex,
What is this for?

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode646
src/d8.cc:646: std::make_pair(Global<Module>(isolate, module), dir_name));
Can you add checks here that these insertions succeeded?

[jochen (gone - plz use gerrit), 2016-10-06 12:21:30]

include/ lgtm

[jochen (gone - plz use gerrit), 2016-10-06 13:28:48]

I guess lsan won't be thrilled about leaking the embedder data. Why not just
pointing a weak persistent at the context and deleting the module data when it
gets GC'd?

[adamk, 2016-10-06 18:36:18]

On 2016/10/06 13:28:48, jochen wrote:
> I guess lsan won't be thrilled about leaking the embedder data. Why not just
> pointing a weak persistent at the context and deleting the module data when it
> gets GC'd?

Given that the Modules hold on to JSFunctions which hold on to the Context, will
the contexts ever be GCed as long as the EmbedderData is holding Globals to the
Context?

[jochen (gone - plz use gerrit), 2016-10-06 19:06:15]

if those globals are weak as well, yes

[adamk, 2016-10-06 20:47:01]

Handled neis's comments (interested to hear your offline idea).

Also it seems like the hash I'm exposing here isn't good, as it depends on the
Symbol name that's been given to the module which has some problems elsewhere in
the system.

As for the lifetime stuff, it's tricky. The modules themselves really shouldn't
be held weakly (at least not in the specifier -> module map). So I'm not sure
how to allow the Context to die, other than by manually clearing the maps at
shutdown. Is that worthwhile? I guess I should run an lsan build and see what it
says, but I see no reason this leak wouldn't be reported.

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc
File src/d8.cc (right):

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode582
src/d8.cc:582: // Map from Module to the directory that Module was loaded from.
On 2016/10/06 09:44:46, neis wrote:
> Nit: keep the comments both in singular or both in plural.

Thanks for keeping me honest. Went with singular.

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode588
src/d8.cc:588: kDebugIdIndex = Context::kDebugIdIndex,
On 2016/10/06 09:44:46, neis wrote:
> What is this for?

Added a comment (and removed the type name). Basically this is a fancy attempt
at making a future-proof kModuleEmbedderDataIndex = 1 :)

https://codereview.chromium.org/2393303002/diff/1/src/d8.cc#newcode646
src/d8.cc:646: std::make_pair(Global<Module>(isolate, module), dir_name));
On 2016/10/06 09:44:46, neis wrote:
> Can you add checks here that these insertions succeeded?

Done.

[adamk, 2016-10-06 20:47:51]

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

[commit-bot: I haz the power, 2016-10-06 20:47:56]

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-06 20:49:46]

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

[commit-bot: I haz the power, 2016-10-06 20:49:47]

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

[adamk, 2016-10-06 21:27:41]

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

[commit-bot: I haz the power, 2016-10-06 21:27:44]

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

[adamk, 2016-10-06 21:28:54]

I've added a DisposeModuleEmbedderData() helper and called it in a bunch of
places. Thoughts? Should I be creating some d8-specific RAII object to handle
this more cleanly?

[commit-bot: I haz the power, 2016-10-06 22:05:04]

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

[commit-bot: I haz the power, 2016-10-06 22:05:05]

Dry run: This issue passed the CQ dry run.

[adamk, 2016-10-07 18:18:26]

Jochen, thoughts on this approach to lifetime management?

[adamk, 2016-10-07 22:49:51]

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

[commit-bot: I haz the power, 2016-10-07 22:50:00]

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-07 23:18:48]

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

[commit-bot: I haz the power, 2016-10-07 23:18:49]

Dry run: This issue passed the CQ dry run.

[neis, 2016-10-10 12:10:41]

lgtm modulo the d8/api issues that you're discussing, which I'm not familiar
with.

[jochen (gone - plz use gerrit), 2016-10-10 12:24:29]

sorry for the late reply. 

In Blink, we also explicitly destroy some per context data when we want to be
able to GC a context, so this approach lgtm

[adamk, 2016-10-10 17:04:54]

The CQ bit was checked by adamk@chromium.org

[commit-bot: I haz the power, 2016-10-10 17:04:58]

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

[adamk, 2016-10-10 17:05:02]

The CQ bit was unchecked by adamk@chromium.org

[adamk, 2016-10-10 17:06:21]

The CQ bit was checked by adamk@chromium.org

[adamk, 2016-10-10 17:06:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
neis@chromium.org
Link to the patchset: https://codereview.chromium.org/2393303002/#ps120001
(title: "Rebased")

[commit-bot: I haz the power, 2016-10-10 17:06:30]

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-10 17:37:39]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-10 17:38:02]

Description was changed from

==========
[modules] Store Module metadata in per-Context EmbedderData

Unifies the approaches used for storing the specifier -> module mapping
and the module -> directory mapping, using std::unordered_maps for both
and storing them per-Context.

This requires adding a method to the v8::Module API to get a hash code
for a Module, but allows slimming down the API in return: gone are
SetEmbedderData/GetEmbedderData, along with the fourth argument
to ResolveModuleCallback.

Besides a simpler API, this allows d8 to get closer to the HTML loader,
which requires each Realm to have a persistent module map (though this
capability is not yet exercised by any tests).

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

to

==========
[modules] Store Module metadata in per-Context EmbedderData

Unifies the approaches used for storing the specifier -> module mapping
and the module -> directory mapping, using std::unordered_maps for both
and storing them per-Context.

This requires adding a method to the v8::Module API to get a hash code
for a Module, but allows slimming down the API in return: gone are
SetEmbedderData/GetEmbedderData, along with the fourth argument
to ResolveModuleCallback.

Besides a simpler API, this allows d8 to get closer to the HTML loader,
which requires each Realm to have a persistent module map (though this
capability is not yet exercised by any tests).

BUG=v8:1569

Committed: https://crrev.com/9cf8fce74cf6e7afd6aea3f3545f6bb61572f277
Cr-Commit-Position: refs/heads/master@{#40133}
==========

[commit-bot: I haz the power, 2016-10-10 17:38:03]

Patchset 7 (id:??) landed as
https://crrev.com/9cf8fce74cf6e7afd6aea3f3545f6bb61572f277
Cr-Commit-Position: refs/heads/master@{#40133}

[adamk, 2016-10-11 00:24:25]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/2406973003/ by adamk@chromium.org.

The reason for reverting is: Fails under LeakSanitizer on auto-roll fyi bot:

https://build.chromium.org/p/client.v8.fyi/builders/Auto-roll%20-%20release%2....