Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2784453002:
[wasm] Fix serialization after instantiation (Closed)
|
Created:
3 years, 8 months ago by Mircea Trofin Modified:
3 years, 8 months ago Reviewers:
bradnelson CC:
v8-reviews_googlegroups.com, ahaas, gdeepti, wasm-v8_google.com Target Ref:
refs/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Fix serialization after instantiation
The regression comes from attempting to serialize a module with memory
requirements after instantiation - which is what happens in common emscripten
scenarios, where the module is obtained from WebAssembly.instantiate(buffer). We then try and serialize the JSArrayBuffer
representing the instance memory. That operation fails.
Added regression test and also extended the test to cover the other 2
instance-specific values - globals and tables.
Added a discussion on WasmCompiledModule (comments) explaining design decisions.
BUG=chromium:705562
Review-Url: https://codereview.chromium.org/2784453002
Cr-Commit-Position: refs/heads/master@{#44250}
Committed: https://chromium.googlesource.com/v8/v8/+/f2531acb1e7ea7c0e1bde2f8230c6b49539dd429
Patch Set 1 : regression test #Patch Set 2 : interm #Patch Set 3 : . #Patch Set 4 : still some left #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : . #
Total comments: 14
Patch Set 8 : feedback #
Messages
Total messages: 38 (33 generated)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [wasm] Serialization must ignore instance fields. BUG= ========== to ========== The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). It turns out this is because of the memory field on the WasmCompiledModule. The field is typed as a JSArrayBuffer, which isn't serializable (nor should it be in this case). Turns out, we don't need to hang the memory on the compiled part anyway. BUG= ==========
Description was changed from ========== The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). It turns out this is because of the memory field on the WasmCompiledModule. The field is typed as a JSArrayBuffer, which isn't serializable (nor should it be in this case). Turns out, we don't need to hang the memory on the compiled part anyway. BUG= ========== to ========== The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). It turns out this is because of the memory field on the WasmCompiledModule. The field is typed as a JSArrayBuffer, which isn't serializable (nor should it be in this case). Turns out, we don't need to hang the memory on the compiled part anyway. The field on WasmCompiledModule was always set when the owning instance's was, too. So we just use that value. BUG=chromium:705562 ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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/23497) 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...)
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). It turns out this is because of the memory field on the WasmCompiledModule. The field is typed as a JSArrayBuffer, which isn't serializable (nor should it be in this case). Turns out, we don't need to hang the memory on the compiled part anyway. The field on WasmCompiledModule was always set when the owning instance's was, too. So we just use that value. BUG=chromium:705562 ========== to ========== [wasm] Fix serialization after instantiation The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). We then try and serialize the JSArrayBuffer representing the instance memory. That operation fails. Added regression test and also extended the test to cover the other 2 instance-specific values - globals and tables. Added a discussion on WasmCompiledModule (comments) explaining design decisions. BUG=chromium:705562 ==========
The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1207: // understands how to relocate the refrerences. We still need to save the references https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:951: WCM_CHECK_TYPE(NAME, obj->IsUndefined(isolate) || obj->IsSmi()) Maybe worth comments on why this and the one below can be undefined. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... File test/mjsunit/wasm/compiled-module-serialization.js (right): https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:165: module = %DeserializeWasmModule(buff, wire_bytes); Any reason to drop the link to the old one here? I.e. now a collect might drop the old one. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:172: You're inconsistent with lines skipped between tests here and above, maybe match the file? https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:178: function MakeTableExportingModule(constant) { Indent weird here. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:225: var m2_clone = %DeserializeWasmModule(serialized_m2, m2_bytes); Worth a variant where you force a GC to ensure links to the old one are gone? https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:229: var i2_prime = new WebAssembly.Instance(m2_clone, {z: {table: i3.exports.table}}); >80
https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:1207: // understands how to relocate the refrerences. We still need to save the On 2017/03/29 20:49:07, bradnelson wrote: > references Done. https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2784453002/diff/140001/src/wasm/wasm-objects.... src/wasm/wasm-objects.cc:951: WCM_CHECK_TYPE(NAME, obj->IsUndefined(isolate) || obj->IsSmi()) On 2017/03/29 20:49:07, bradnelson wrote: > Maybe worth comments on why this and the one below can be undefined. Done. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... File test/mjsunit/wasm/compiled-module-serialization.js (right): https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:165: module = %DeserializeWasmModule(buff, wire_bytes); On 2017/03/29 20:49:07, bradnelson wrote: > Any reason to drop the link to the old one here? I.e. now a collect might drop > the old one. Done. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:172: On 2017/03/29 20:49:07, bradnelson wrote: > You're inconsistent with lines skipped between tests here and above, maybe match > the file? Done. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:178: function MakeTableExportingModule(constant) { On 2017/03/29 20:49:07, bradnelson wrote: > Indent weird here. Done. https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:225: var m2_clone = %DeserializeWasmModule(serialized_m2, m2_bytes); On 2017/03/29 20:49:07, bradnelson wrote: > Worth a variant where you force a GC to ensure links to the old one are gone? We test that we execute instance-specific functionality correctly. If we didn't let go of references, we'd see leaked instance functionality, and also, we wouldn't collect anything (because the reference would stay alive). https://codereview.chromium.org/2784453002/diff/140001/test/mjsunit/wasm/comp... test/mjsunit/wasm/compiled-module-serialization.js:229: var i2_prime = new WebAssembly.Instance(m2_clone, {z: {table: i3.exports.table}}); On 2017/03/29 20:49:07, bradnelson wrote: > >80 Done.
The CQ bit was checked by mtrofin@chromium.org
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/2784453002/#ps160001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490820996544800,
"parent_rev": "5d990dbfb7c345121bb84863a4c7d790e502023d", "commit_rev":
"f2531acb1e7ea7c0e1bde2f8230c6b49539dd429"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix serialization after instantiation The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). We then try and serialize the JSArrayBuffer representing the instance memory. That operation fails. Added regression test and also extended the test to cover the other 2 instance-specific values - globals and tables. Added a discussion on WasmCompiledModule (comments) explaining design decisions. BUG=chromium:705562 ========== to ========== [wasm] Fix serialization after instantiation The regression comes from attempting to serialize a module with memory requirements after instantiation - which is what happens in common emscripten scenarios, where the module is obtained from WebAssembly.instantiate(buffer). We then try and serialize the JSArrayBuffer representing the instance memory. That operation fails. Added regression test and also extended the test to cover the other 2 instance-specific values - globals and tables. Added a discussion on WasmCompiledModule (comments) explaining design decisions. BUG=chromium:705562 Review-Url: https://codereview.chromium.org/2784453002 Cr-Commit-Position: refs/heads/master@{#44250} Committed: https://chromium.googlesource.com/v8/v8/+/f2531acb1e7ea7c0e1bde2f8230c6b49539... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/v8/v8/+/f2531acb1e7ea7c0e1bde2f8230c6b49539... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
