[wasm] reuse the first compiled module

[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316
Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Committed: https://crrev.com/b4dc310aab9b8aae362bcc226a9b0bb160ed2450
Cr-Original-Commit-Position: refs/heads/master@{#39153}
Cr-Commit-Position: refs/heads/master@{#39361}

[Mircea Trofin, 2016-09-01 22:38:38]

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

[commit-bot: I haz the power, 2016-09-01 22:38:49]

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-09-01 22:46:22]

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

[commit-bot: I haz the power, 2016-09-01 22:46:22]

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

[Mircea Trofin, 2016-09-02 04:16:36]

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

[commit-bot: I haz the power, 2016-09-02 04:16:42]

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-09-02 04:21:55]

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

[commit-bot: I haz the power, 2016-09-02 04:21:56]

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/13565)

[Mircea Trofin, 2016-09-02 04:25:31]

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

[commit-bot: I haz the power, 2016-09-02 04:25:32]

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

[Mircea Trofin, 2016-09-02 04:36:25]

Description was changed from

==========
[wasm] reuse the first compiled module

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

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

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

[commit-bot: I haz the power, 2016-09-02 04:37:50]

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

[commit-bot: I haz the power, 2016-09-02 04:37:51]

Dry run: Try jobs failed on following builders:
  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_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[Mircea Trofin, 2016-09-02 04:38:43]

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

[commit-bot: I haz the power, 2016-09-02 04:38:52]

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

[Mircea Trofin, 2016-09-02 04:41:10]

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

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

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316
==========

[Mircea Trofin, 2016-09-02 04:41:39]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org

[Mircea Trofin, 2016-09-02 04:43:35]

Oddly, we need to disable ignition for the test counting remaining 
instances after GC. Having ignition "on" does not run the finalizer when
we force gc.

[commit-bot: I haz the power, 2016-09-02 04:54:41]

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

[commit-bot: I haz the power, 2016-09-02 04:54:42]

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

[Mircea Trofin, 2016-09-02 06:23:22]

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

[commit-bot: I haz the power, 2016-09-02 06:23:28]

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

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

[commit-bot: I haz the power, 2016-09-02 06:49:21]

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/11961)
  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...)

[Mircea Trofin, 2016-09-02 15:41:55]

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

[commit-bot: I haz the power, 2016-09-02 15:42: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-09-02 16:01:04]

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

[commit-bot: I haz the power, 2016-09-02 16:01:05]

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_linux64_avx2_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)

[Mircea Trofin, 2016-09-02 16:52:04]

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

[commit-bot: I haz the power, 2016-09-02 16:52:14]

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

[bradnelson, 2016-09-02 16:57:00]

https://codereview.chromium.org/2305903002/diff/100001/src/snapshot/code-seri...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2305903002/diff/100001/src/snapshot/code-seri...
src/snapshot/code-serializer.cc:102: return
SerializeGeneric(*isolate()->factory()->undefined_value(),
So the links contain undefined when serialized?

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (left):

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:456: DCHECK(code->deoptimization_data() == nullptr ||
Why?

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:162: kWasmCompiledModule,
Optional:
Realize the default is zero, but I've sometimes seen the pattern where an
explicit =0 is a signal that other code depends on this enum being tightly
packed.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1207: if (!weak_module_obj->cleared()) {
Why?

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1211: DCHECK_NULL(GetPrevInstance(current_template));
Separate out the remove from chain operation?
Or comment that's what's going on?

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1322: FlushAssemblyCache(isolate, compiled_functions);
Because the code has never run? Probably not.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1600: // replaced with the instance's actual imports in
SetupImports.
reflow comment

https://codereview.chromium.org/2305903002/diff/100001/test/mjsunit/wasm/comp...
File test/mjsunit/wasm/compiled-module-management.js (right):

https://codereview.chromium.org/2305903002/diff/100001/test/mjsunit/wasm/comp...
test/mjsunit/wasm/compiled-module-management.js:5: // Flags: --no-ignition
--no-ignition-staging --expose-wasm --expose-gc --allow-natives-syntax
You can split these into two Flags: to stay under 80.

[commit-bot: I haz the power, 2016-09-02 17:15:40]

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

[commit-bot: I haz the power, 2016-09-02 17:15:41]

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/12009)
  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...)

[Mircea Trofin, 2016-09-02 17:19:03]

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

[commit-bot: I haz the power, 2016-09-02 17:19:09]

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-09-02 17:40:43]

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

[commit-bot: I haz the power, 2016-09-02 17:40:44]

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

[Mircea Trofin, 2016-09-02 20:42:48]

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

[commit-bot: I haz the power, 2016-09-02 20:42:56]

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

[Mircea Trofin, 2016-09-02 20:55:30]

https://codereview.chromium.org/2305903002/diff/100001/src/snapshot/code-seri...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2305903002/diff/100001/src/snapshot/code-seri...
src/snapshot/code-serializer.cc:102: return
SerializeGeneric(*isolate()->factory()->undefined_value(),
On 2016/09/02 16:56:59, bradnelson wrote:
> So the links contain undefined when serialized?

Yes. The module obtained by deserializing this is its own separate
JSObject instance, with its own wasm module instances chain

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (left):

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:456: DCHECK(code->deoptimization_data() == nullptr ||
On 2016/09/02 16:57:00, bradnelson wrote:
> Why?

Because now the deopt will be valid, but belonging to the code object we
used as prototype for this new one.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.cc
File src/wasm/wasm-module.cc (right):

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:162: kWasmCompiledModule,
On 2016/09/02 16:56:59, bradnelson wrote:
> Optional:
> Realize the default is zero, but I've sometimes seen the pattern where an
> explicit =0 is a signal that other code depends on this enum being tightly
> packed.

Done.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1207: if (!weak_module_obj->cleared()) {
On 2016/09/02 16:57:00, bradnelson wrote:
> Why?

It may be cleared when this happens:
var m = new WebAssembly.Module(...);
var i1 = new WebAssembly.Instance(m);
var i2 = new WebAssembly.Instance(m);
m = nullptr;
gc();

In this case, we don't care anymore to maintain the links, because no new
instances may be created.

I added a comment.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1211: DCHECK_NULL(GetPrevInstance(current_template));
On 2016/09/02 16:57:00, bradnelson wrote:
> Separate out the remove from chain operation?
> Or comment that's what's going on?

There nothing else than the delete - we're called here when an instance is
dying.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1322: FlushAssemblyCache(isolate, compiled_functions);
On 2016/09/02 16:56:59, bradnelson wrote:
> Because the code has never run? Probably not.

Acknowledged.

https://codereview.chromium.org/2305903002/diff/100001/src/wasm/wasm-module.c...
src/wasm/wasm-module.cc:1600: // replaced with the instance's actual imports in
SetupImports.
On 2016/09/02 16:57:00, bradnelson wrote:
> reflow comment

Done.

https://codereview.chromium.org/2305903002/diff/100001/test/mjsunit/wasm/comp...
File test/mjsunit/wasm/compiled-module-management.js (right):

https://codereview.chromium.org/2305903002/diff/100001/test/mjsunit/wasm/comp...
test/mjsunit/wasm/compiled-module-management.js:5: // Flags: --no-ignition
--no-ignition-staging --expose-wasm --expose-gc --allow-natives-syntax
On 2016/09/02 16:57:00, bradnelson wrote:
> You can split these into two Flags: to stay under 80.

Done.

[commit-bot: I haz the power, 2016-09-02 21:34:42]

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

[commit-bot: I haz the power, 2016-09-02 21:34:43]

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/12020)
  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...)

[Mircea Trofin, 2016-09-02 22:39:47]

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

[commit-bot: I haz the power, 2016-09-02 22:39:58]

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-09-02 23:26:11]

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

[commit-bot: I haz the power, 2016-09-02 23:26:12]

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/12024)
  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...)

[Mircea Trofin, 2016-09-02 23:52:33]

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

[commit-bot: I haz the power, 2016-09-02 23:52:34]

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-09-03 00:20:04]

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

[commit-bot: I haz the power, 2016-09-03 00:20:05]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2016-09-03 00:28:10]

Fixed remaining gcmole issues.

[bradnelson, 2016-09-03 01:19:53]

lgtm

[Mircea Trofin, 2016-09-03 01:30:37]

The CQ bit was checked by mtrofin@chromium.org

[commit-bot: I haz the power, 2016-09-03 01:30:44]

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-09-03 01:37:33]

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

[commit-bot: I haz the power, 2016-09-03 01:37:35]

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/23237)

[Mircea Trofin, 2016-09-03 04:13:40]

mtrofin@chromium.org changed reviewers:
+ verwaest@chromium.org, vogelheim@chromium.org, yangguo@chromium.org

[Mircea Trofin, 2016-09-03 04:14:42]

Toon/Daniel/Yang - I need signoff for the serializer part. PTAL?

Thanks!

[vogelheim, 2016-09-05 08:09:52]

lgtm for src/snapshot/...

https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
src/snapshot/code-serializer.cc:101: if (SkipOver(obj)) {
I would have preferred a more expressive name, or a comment on why this is here.

You're effectively implementing special case handling for one object type (weak
cells) in one CodeSerializer subclass. But since the name only explains the
mechanism (skip over this value), there is no easy way for a reader to figure
out what the intent is, and if this is a commonly used mechanism or just an edge
case.

[Mircea Trofin, 2016-09-05 09:41:27]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-09-05 09:41:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org
Link to the patchset: https://codereview.chromium.org/2305903002/#ps220001
(title: "feedback")

[commit-bot: I haz the power, 2016-09-05 09:41:38]

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

[Mircea Trofin, 2016-09-05 09:46:37]

On 2016/09/05 08:09:52, vogelheim wrote:
> lgtm for src/snapshot/...
> 
>
https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
> File src/snapshot/code-serializer.cc (right):
> 
>
https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
> src/snapshot/code-serializer.cc:101: if (SkipOver(obj)) {
> I would have preferred a more expressive name, or a comment on why this is
here.
> 
> You're effectively implementing special case handling for one object type
(weak
> cells) in one CodeSerializer subclass. But since the name only explains the
> mechanism (skip over this value), there is no easy way for a reader to figure
> out what the intent is, and if this is a commonly used mechanism or just an
edge
> case.

Good point. I'll have to address a more involved case shortly, time at which
SkipOver either 
goes, in favor of new mechanism, or I'll rename it as per your feedback.

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

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316
==========

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316
==========

[commit-bot: I haz the power, 2016-09-05 10:08:07]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-09-05 10:08:48]

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is 
collected, we make sure that the module object still has a clone 
available, and, if the last instance is GC-ed, we also reset the compiled 
module so that it does not reference its heap, so that it (==heap) may 
be collected. 

This is achieved by linking the clones in a double-linked list and 
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When 
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316
==========

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

[commit-bot: I haz the power, 2016-09-05 10:08:49]

Patchset 12 (id:??) landed as
https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}

[Michael Achenbach, 2016-09-05 10:49:20]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/2306403002/ by machenbach@chromium.org.

The reason for reverting is: mac gc stress failures:
https://build.chromium.org/p/client.v8/builders/V8%20Mac%20GC%20Stress/builds....

[Michael Starzinger, 2016-09-07 12:56:58]

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

[Michael Starzinger, 2016-09-07 12:56:59]

https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
File test/mjsunit/wasm/compiled-module-management.js (right):

https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
test/mjsunit/wasm/compiled-module-management.js:35: gc();
The failure in Ignition is working as intended. Object lifetime is not specified
by JavaScript and is not observable via normal JavaScript code. Therefore it is
perfectly legal to hold on to hidden temporary variables and extend lifetime
until the end of the function body (which Ignition does).

If we want to stick with observing lifetime here, then I would vote for changing
the test so that "i1", "i2" and so forth are stored in a global structure and
nulled out there. The initialization (i.e. lines 23 till 31) can then life in a
separate function from the one which nulls out the instances (making them
unreachable). By splitting up initialization and mutation into two different
functions, Ignition shouldn't keep the instance alive any longer.

Note that even that is just a best effort approach. I could very well imaging
that inlining could combine those two function bodies again and (accidentally)
extend the lifetime again.

[Yang, 2016-09-10 03:55:55]

On 2016/09/07 12:56:59, Michael Starzinger wrote:
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> File test/mjsunit/wasm/compiled-module-management.js (right):
> 
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> test/mjsunit/wasm/compiled-module-management.js:35: gc();
> The failure in Ignition is working as intended. Object lifetime is not
specified
> by JavaScript and is not observable via normal JavaScript code. Therefore it
is
> perfectly legal to hold on to hidden temporary variables and extend lifetime
> until the end of the function body (which Ignition does).
> 
> If we want to stick with observing lifetime here, then I would vote for
changing
> the test so that "i1", "i2" and so forth are stored in a global structure and
> nulled out there. The initialization (i.e. lines 23 till 31) can then life in
a
> separate function from the one which nulls out the instances (making them
> unreachable). By splitting up initialization and mutation into two different
> functions, Ignition shouldn't keep the instance alive any longer.
> 
> Note that even that is just a best effort approach. I could very well imaging
> that inlining could combine those two function bodies again and (accidentally)
> extend the lifetime again.

I'd also liked to see a more expressive name than "SkipOver". I see that the
name was kept when this CL landed.

[Yang, 2016-09-10 03:58:37]

https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
File src/snapshot/code-serializer.cc (right):

https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
src/snapshot/code-serializer.cc:101: if (SkipOver(obj)) {
On 2016/09/05 08:09:52, vogelheim wrote:
> I would have preferred a more expressive name, or a comment on why this is
here.
> 
> You're effectively implementing special case handling for one object type
(weak
> cells) in one CodeSerializer subclass. But since the name only explains the
> mechanism (skip over this value), there is no easy way for a reader to figure
> out what the intent is, and if this is a commonly used mechanism or just an
edge
> case.

I wonder whether it' not cleaner to implement a separate Serialize method for
the wasm code serializer that filters weak cells before calling this Serialize
method.

[Mircea Trofin, 2016-09-10 03:59:05]

On 2016/09/10 03:55:55, Yang wrote:
> On 2016/09/07 12:56:59, Michael Starzinger wrote:
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > File test/mjsunit/wasm/compiled-module-management.js (right):
> > 
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > test/mjsunit/wasm/compiled-module-management.js:35: gc();
> > The failure in Ignition is working as intended. Object lifetime is not
> specified
> > by JavaScript and is not observable via normal JavaScript code. Therefore it
> is
> > perfectly legal to hold on to hidden temporary variables and extend lifetime
> > until the end of the function body (which Ignition does).
> > 
> > If we want to stick with observing lifetime here, then I would vote for
> changing
> > the test so that "i1", "i2" and so forth are stored in a global structure
and
> > nulled out there. The initialization (i.e. lines 23 till 31) can then life
in
> a
> > separate function from the one which nulls out the instances (making them
> > unreachable). By splitting up initialization and mutation into two different
> > functions, Ignition shouldn't keep the instance alive any longer.
> > 
> > Note that even that is just a best effort approach. I could very well
imaging
> > that inlining could combine those two function bodies again and
(accidentally)
> > extend the lifetime again.
> 
> I'd also liked to see a more expressive name than "SkipOver". I see that the
> name was kept when this CL landed.

I'll rename it when I resubmit. Would "ElideObject" be a better name?

[Mircea Trofin, 2016-09-10 04:05:16]

On 2016/09/10 03:58:37, Yang wrote:
>
https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
> File src/snapshot/code-serializer.cc (right):
> 
>
https://codereview.chromium.org/2305903002/diff/220001/src/snapshot/code-seri...
> src/snapshot/code-serializer.cc:101: if (SkipOver(obj)) {
> On 2016/09/05 08:09:52, vogelheim wrote:
> > I would have preferred a more expressive name, or a comment on why this is
> here.
> > 
> > You're effectively implementing special case handling for one object type
> (weak
> > cells) in one CodeSerializer subclass. But since the name only explains the
> > mechanism (skip over this value), there is no easy way for a reader to
figure
> > out what the intent is, and if this is a commonly used mechanism or just an
> edge
> > case.
> 
> I wonder whether it' not cleaner to implement a separate Serialize method for
> the wasm code serializer that filters weak cells before calling this Serialize
> method.

Probably. As I mentioned earlier, there is a bit more to do after this CL, such
as eliding deopt info on code objects before serializing, and re-adding after. I
could probably treat these skipoed over objects similarly. My goal for now is to
ensure the newly introduced instance management is correct. 

I'll change the api name though, for the interim.

[Mircea Trofin, 2016-09-12 22:45:22]

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

[Mircea Trofin, 2016-09-12 22:45:28]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2016-09-12 22:45:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, vogelheim@chromium.org
Link to the patchset: https://codereview.chromium.org/2305903002/#ps240001
(title: "GC before cloning")

[commit-bot: I haz the power, 2016-09-12 22:45:31]

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-09-12 23:12:30]

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

[commit-bot: I haz the power, 2016-09-12 23:12:31]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-09-12 23:13:27]

Description was changed from

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Cr-Commit-Position: refs/heads/master@{#39153}
==========

to

==========
[wasm] reuse the first compiled module.

This change avoids needing to keep around an unused compiled
module. Instead, the result of compiling the wasm bytes is
given to the first instance. The module object and that instance object
point to the same compiled module. Instances are, then, cloned from
the compiled module the module object points to. When an instance is
collected, we make sure that the module object still has a clone
available, and, if the last instance is GC-ed, we also reset the compiled
module so that it does not reference its heap, so that it (==heap) may
be collected.

This is achieved by linking the clones in a double-linked list and
registering a finalizer for each. When we create an instance, we tie it
in the front of the list, making the module object point to it (O(1)). When
the finalizer is called, we relink the list over the dying object (O(1)). The
costliest operation is finalizing the last instance, since we need to visit
all wasm functions and reset heap references.

BUG=v8:5316

Committed: https://crrev.com/01f5af515728aebe6c5246f4f7dd6c573e8748af
Committed: https://crrev.com/b4dc310aab9b8aae362bcc226a9b0bb160ed2450
Cr-Original-Commit-Position: refs/heads/master@{#39153}
Cr-Commit-Position: refs/heads/master@{#39361}
==========

[commit-bot: I haz the power, 2016-09-12 23:13:29]

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

[Mircea Trofin, 2016-09-14 18:59:20]

On 2016/09/07 12:56:59, Michael Starzinger wrote:
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> File test/mjsunit/wasm/compiled-module-management.js (right):
> 
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> test/mjsunit/wasm/compiled-module-management.js:35: gc();
> The failure in Ignition is working as intended. Object lifetime is not
specified
> by JavaScript and is not observable via normal JavaScript code. Therefore it
is
> perfectly legal to hold on to hidden temporary variables and extend lifetime
> until the end of the function body (which Ignition does).
> 
> If we want to stick with observing lifetime here, then I would vote for
changing
> the test so that "i1", "i2" and so forth are stored in a global structure and
> nulled out there. The initialization (i.e. lines 23 till 31) can then life in
a
> separate function from the one which nulls out the instances (making them
> unreachable). By splitting up initialization and mutation into two different
> functions, Ignition shouldn't keep the instance alive any longer.
> 
> Note that even that is just a best effort approach. I could very well imaging
> that inlining could combine those two function bodies again and (accidentally)
> extend the lifetime again.

Sorry for the delayed reply, I wanted to fix up the blocker to this CL first.

Do we have a "no inline" test-only flag, perhaps a "natives" call ( a quick grep
didn't reveal 
anything). Do we want to, or is moving to the suggested alternative for this
test not
a sufficient motivation for this added functionality (==no inlining).

Thanks!

[Michael Starzinger, 2016-09-27 09:35:54]

On 2016/09/14 18:59:20, Mircea Trofin wrote:
> On 2016/09/07 12:56:59, Michael Starzinger wrote:
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > File test/mjsunit/wasm/compiled-module-management.js (right):
> > 
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > test/mjsunit/wasm/compiled-module-management.js:35: gc();
> > The failure in Ignition is working as intended. Object lifetime is not
> specified
> > by JavaScript and is not observable via normal JavaScript code. Therefore it
> is
> > perfectly legal to hold on to hidden temporary variables and extend lifetime
> > until the end of the function body (which Ignition does).
> > 
> > If we want to stick with observing lifetime here, then I would vote for
> changing
> > the test so that "i1", "i2" and so forth are stored in a global structure
and
> > nulled out there. The initialization (i.e. lines 23 till 31) can then life
in
> a
> > separate function from the one which nulls out the instances (making them
> > unreachable). By splitting up initialization and mutation into two different
> > functions, Ignition shouldn't keep the instance alive any longer.
> > 
> > Note that even that is just a best effort approach. I could very well
imaging
> > that inlining could combine those two function bodies again and
(accidentally)
> > extend the lifetime again.
> 
> Sorry for the delayed reply, I wanted to fix up the blocker to this CL first.
> 
> Do we have a "no inline" test-only flag, perhaps a "natives" call ( a quick
grep
> didn't reveal 
> anything). Do we want to, or is moving to the suggested alternative for this
> test not
> a sufficient motivation for this added functionality (==no inlining).
> 
> Thanks!

Sorry for lat reply, my Gmail at the mail. We have %NeverOptimizeFunction(f),
which will also prevent inlining of "f" into any other function.

[Mircea Trofin, 2016-10-07 21:58:10]

On 2016/09/27 09:35:54, Michael Starzinger wrote:
> On 2016/09/14 18:59:20, Mircea Trofin wrote:
> > On 2016/09/07 12:56:59, Michael Starzinger wrote:
> > >
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > > File test/mjsunit/wasm/compiled-module-management.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2305903002/diff/220001/test/mjsunit/wasm/comp...
> > > test/mjsunit/wasm/compiled-module-management.js:35: gc();
> > > The failure in Ignition is working as intended. Object lifetime is not
> > specified
> > > by JavaScript and is not observable via normal JavaScript code. Therefore
it
> > is
> > > perfectly legal to hold on to hidden temporary variables and extend
lifetime
> > > until the end of the function body (which Ignition does).
> > > 
> > > If we want to stick with observing lifetime here, then I would vote for
> > changing
> > > the test so that "i1", "i2" and so forth are stored in a global structure
> and
> > > nulled out there. The initialization (i.e. lines 23 till 31) can then life
> in
> > a
> > > separate function from the one which nulls out the instances (making them
> > > unreachable). By splitting up initialization and mutation into two
different
> > > functions, Ignition shouldn't keep the instance alive any longer.
> > > 
> > > Note that even that is just a best effort approach. I could very well
> imaging
> > > that inlining could combine those two function bodies again and
> (accidentally)
> > > extend the lifetime again.
> > 
> > Sorry for the delayed reply, I wanted to fix up the blocker to this CL
first.
> > 
> > Do we have a "no inline" test-only flag, perhaps a "natives" call ( a quick
> grep
> > didn't reveal 
> > anything). Do we want to, or is moving to the suggested alternative for this
> > test not
> > a sufficient motivation for this added functionality (==no inlining).
> > 
> > Thanks!
> 
> Sorry for lat reply, my Gmail at the mail. We have %NeverOptimizeFunction(f),
> which will also prevent inlining of "f" into any other function.

I see - I'll give that a try and update the test to not avoid ignition.