Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(77)

Issue 2094563002: [wasm] Complete separation of compilation and instantiation (Closed)

Created:
4 years, 6 months ago by Mircea Trofin
Modified:
4 years, 5 months ago
CC:
v8-reviews_googlegroups.com, Yang, rossberg
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] Complete separation of compilation and instantiation Support for serializing/deserializing the compiled wasm module. We want to reuse the javascript snapshotting mechanics, at least in the short term, when we still use the JS heap for the compiled wasm code. Given that a module may be compiled in one v8 instance and then instantiated later, in a different instance, whatever information we need at instantiation time must also be serializable. We currently hold on to the un-decoded wasm bytes, for enabling debugging scenarios. This imposes a ~20% penalty on the memory requirements of the wasm compiled code. We do not need this data otherwise, for runtime, and it is sensible to consider eventually loading it on demand. Therefore, I intentionally avoided relying on it and re- decoding the wasm module data, and instead saved the information necessary to support instantiation. Given how whatever we need to persist must be serializable, the CL uses a structure made out of serializable objects (fixed arrays mostly) for storing this information. I preferred going this route rather than adding more wasm-specific support to the serializer, given that we want to eventually move off the JS heap, and therefore the serializer. Additionally, it turns out this extra information is relatively not complex: minimal structure, little nesting depth, mostly simple data like numbers or byte blobs, or opaque data like compiled functions. This CL also moves export compilation ahead of instantiation time. This change added a helper getter to FixedArray, to make typed retrieval of elements easier. BUG= Committed: https://crrev.com/0c7ee92783eaadbd62fd37f7ae1035c4778e9c7a Cr-Commit-Position: refs/heads/master@{#37348}

Patch Set 1 #

Patch Set 2 : formatting #

Patch Set 3 : More progress. #

Patch Set 4 : More progress. #

Patch Set 5 #

Patch Set 6 #

Patch Set 7 #

Patch Set 8 #

Total comments: 2

Patch Set 9 : MaybeHandle #

Total comments: 10

Patch Set 10 : Incorporated Feedback #

Patch Set 11 : Incorporated Feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+636 lines, -338 lines) Patch
M src/compiler/wasm-compiler.h View 1 2 3 4 5 6 7 8 1 chunk +8 lines, -6 lines 0 comments Download
M src/compiler/wasm-compiler.cc View 1 2 3 4 5 6 7 8 6 chunks +56 lines, -37 lines 0 comments Download
M src/objects.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +7 lines, -0 lines 0 comments Download
M src/objects-inl.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +16 lines, -1 line 0 comments Download
M src/signature.h View 1 2 3 4 5 6 3 chunks +4 lines, -2 lines 0 comments Download
M src/wasm/wasm-js.cc View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -3 lines 0 comments Download
M src/wasm/wasm-module.h View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -3 lines 0 comments Download
M src/wasm/wasm-module.cc View 1 2 3 4 5 6 7 8 9 10 15 chunks +525 lines, -280 lines 0 comments Download
M test/cctest/wasm/wasm-run-utils.h View 1 2 3 4 5 6 2 chunks +7 lines, -6 lines 0 comments Download

Messages

Total messages: 26 (16 generated)
Mircea Trofin
PTAL. Benedikt, is adding the proposed API to FixedArray OK? I don't need that there, ...
4 years, 6 months ago (2016-06-26 02:21:04 UTC) #10
Benedikt Meurer
https://codereview.chromium.org/2094563002/diff/180001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2094563002/diff/180001/src/objects.h#newcode2651 src/objects.h:2651: Handle<T> GetValueOrNull(int index) const; This should return a MaybeHandle. ...
4 years, 5 months ago (2016-06-27 03:57:13 UTC) #11
Mircea Trofin
https://codereview.chromium.org/2094563002/diff/180001/src/objects.h File src/objects.h (right): https://codereview.chromium.org/2094563002/diff/180001/src/objects.h#newcode2651 src/objects.h:2651: Handle<T> GetValueOrNull(int index) const; On 2016/06/27 03:57:13, Benedikt Meurer ...
4 years, 5 months ago (2016-06-27 16:59:57 UTC) #13
bradnelson
lgtm The insides and outsides bleed together, but that was true before. https://codereview.chromium.org/2094563002/diff/220001/src/objects.h File src/objects.h ...
4 years, 5 months ago (2016-06-27 22:28:47 UTC) #14
Mircea Trofin
https://codereview.chromium.org/2094563002/diff/220001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2094563002/diff/220001/src/wasm/wasm-module.cc#newcode129 src/wasm/wasm-module.cc:129: enum CompiledWasmObjectFields { On 2016/06/27 22:28:47, bradnelson wrote: > ...
4 years, 5 months ago (2016-06-28 05:24:21 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2094563002/260001
4 years, 5 months ago (2016-06-28 05:24:40 UTC) #18
commit-bot: I haz the power
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/18160)
4 years, 5 months ago (2016-06-28 05:29:12 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2094563002/260001
4 years, 5 months ago (2016-06-28 20:44:37 UTC) #22
commit-bot: I haz the power
Committed patchset #11 (id:260001)
4 years, 5 months ago (2016-06-28 20:46:44 UTC) #24
commit-bot: I haz the power
4 years, 5 months ago (2016-06-28 20:49:38 UTC) #26
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/0c7ee92783eaadbd62fd37f7ae1035c4778e9c7a
Cr-Commit-Position: refs/heads/master@{#37348}

Powered by Google App Engine
This is Rietveld 408576698