[wasm] Prepare WasmCompilationUnit for lazy compilation

[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be helpful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991
Review-Url: https://codereview.chromium.org/2726553003
Cr-Commit-Position: refs/heads/master@{#43573}
Committed: https://chromium.googlesource.com/v8/v8/+/7f68cbbffa14376006226a427d1b42081b109d46

[Clemens Hammacher, 2017-02-28 14:58:54]

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

[commit-bot: I haz the power, 2017-02-28 14:58:58]

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

[Clemens Hammacher, 2017-02-28 15:00:20]

Description was changed from

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be useful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=5991
==========

to

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be useful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991
==========

[Clemens Hammacher, 2017-02-28 15:15:56]

Description was changed from

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be useful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991
==========

to

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be helpful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991
==========

[commit-bot: I haz the power, 2017-02-28 15:38:45]

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

[commit-bot: I haz the power, 2017-02-28 15:38:45]

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

[ahaas, 2017-02-28 18:33:36]

lgtm

https://codereview.chromium.org/2726553003/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2726553003/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:4073: if (func_name_.start() == nullptr) {
Shouldn't this be a !=?

[Clemens Hammacher, 2017-03-02 11:41:07]

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

[commit-bot: I haz the power, 2017-03-02 11:41:20]

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

[Clemens Hammacher, 2017-03-02 11:52:14]

https://codereview.chromium.org/2726553003/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2726553003/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:4073: if (func_name_.start() == nullptr) {
On 2017/02/28 at 18:33:35, ahaas wrote:
> Shouldn't this be a !=?

Indeed. Fixed.

[commit-bot: I haz the power, 2017-03-02 12:09:41]

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

[commit-bot: I haz the power, 2017-03-02 12:09: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/21887)
  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...)

[Clemens Hammacher, 2017-03-03 08:58:11]

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

[commit-bot: I haz the power, 2017-03-03 08:58:15]

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

[titzer, 2017-03-03 09:21:45]

lgtm with nit

https://codereview.chromium.org/2726553003/diff/40001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (left):

https://codereview.chromium.org/2726553003/diff/40001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:73: wasm::ErrorThrower* thrower_;
Ah, good that moved this out!

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h#newcode38
src/vector.h:38: SLOW_DCHECK(from <= to);
Any reason this is a SLOW_DCHECK? I wonder if it's not faster just to make this
a DCHECK, since it's single comparison. Same below.

[Clemens Hammacher, 2017-03-03 09:28:43]

clemensh@chromium.org changed reviewers:
+ jochen@chromium.org

[Clemens Hammacher, 2017-03-03 09:29:54]

Jochen, can you provide insights on the minor discussion below?

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h#newcode38
src/vector.h:38: SLOW_DCHECK(from <= to);
On 2017/03/03 at 09:21:44, titzer wrote:
> Any reason this is a SLOW_DCHECK? I wonder if it's not faster just to make
this a DCHECK, since it's single comparison. Same below.

I don't know the reason for this. Maybe just to reduce code size (assuming that
this function gets inlined a lot)? SLOW_DCHECK compiles to nothing if not
compiled with v8_enable_slow_dchecks.
Also note that the first SLOW_DCHECK is redundant with the DCHECK in the Vector
constructor.
Adding jochen@ who added the SLOW_DCHECKs more than two years ago.

[commit-bot: I haz the power, 2017-03-03 09:42:20]

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

[commit-bot: I haz the power, 2017-03-03 09:42:21]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-03-03 09:44:49]

On 2017/03/03 at 09:29:54, Clemens Hammacher wrote:
> Jochen, can you provide insights on the minor discussion below?
> 
> https://codereview.chromium.org/2726553003/diff/40001/src/vector.h
> File src/vector.h (right):
> 
> https://codereview.chromium.org/2726553003/diff/40001/src/vector.h#newcode38
> src/vector.h:38: SLOW_DCHECK(from <= to);
> On 2017/03/03 at 09:21:44, titzer wrote:
> > Any reason this is a SLOW_DCHECK? I wonder if it's not faster just to make
this a DCHECK, since it's single comparison. Same below.
> 
> I don't know the reason for this. Maybe just to reduce code size (assuming
that this function gets inlined a lot)? SLOW_DCHECK compiles to nothing if not
compiled with v8_enable_slow_dchecks.
> Also note that the first SLOW_DCHECK is redundant with the DCHECK in the
Vector constructor.
> Adding jochen@ who added the SLOW_DCHECKs more than two years ago.

Oops, Jochen is on vacation. Landing for now, we can still change those
SLOW_DCHECKs later.

[Clemens Hammacher, 2017-03-03 09:44:54]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-03-03 09:44:55]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2726553003/#ps40001
(title: "Minor fix in vector.h")

[commit-bot: I haz the power, 2017-03-03 09:45:00]

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-03-03 09:47:41]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488534294867470,
"parent_rev": "de52562d8e6fec7949cafd39bb7aa58d3269ac8e", "commit_rev":
"7f68cbbffa14376006226a427d1b42081b109d46"}

[commit-bot: I haz the power, 2017-03-03 09:47:46]

Description was changed from

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be helpful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991
==========

to

==========
[wasm] Prepare WasmCompilationUnit for lazy compilation

In lazy compilation, we only compile one function at a time, and we
might not have the wire bytes of the whole module available.
This CL prepares the WasmCompilationUnit for this setting.
It will also be helpful for streaming compilation.

Also, the ErrorThrower (which might heap-allocate) is not stored in the
WasmCompilationUnit any more. Instead, it is passed to the
FinishCompilation method which is allowed to heap-allocate.

R=titzer@chromium.org, ahaas@chromium.org
BUG=v8:5991

Review-Url: https://codereview.chromium.org/2726553003
Cr-Commit-Position: refs/heads/master@{#43573}
Committed:
https://chromium.googlesource.com/v8/v8/+/7f68cbbffa14376006226a427d1b42081b1...
==========

[commit-bot: I haz the power, 2017-03-03 09:47:47]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/7f68cbbffa14376006226a427d1b42081b1...

[jochen (gone - plz use gerrit), 2017-05-03 10:47:22]

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h#newcode38
src/vector.h:38: SLOW_DCHECK(from <= to);
Note that this change breaks gcc compilation.

(I just renamed the macro, it was a slow dcheck before as well)

[Clemens Hammacher, 2017-05-03 11:01:55]

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h
File src/vector.h (right):

https://codereview.chromium.org/2726553003/diff/40001/src/vector.h#newcode38
src/vector.h:38: SLOW_DCHECK(from <= to);
On 2017/05/03 at 10:47:22, jochen wrote:
> Note that this change breaks gcc compilation.

For reference: You are referring to this bug: http://crbug.com/v8/6341

How does my change cause this? Wouldn't gcc have complained about "num_chars - 1
< num_chars" as well?