Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2471923002:
Support structured clone of compiled WebAssembly modules. (Closed)
|
Created:
4 years, 1 month ago by jbroman Modified:
4 years, 1 month ago Reviewers:
Mircea Trofin CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionSupport structured clone of compiled WebAssembly modules.
Compatible with the current (unshipped) Blink implementation.
BUG=chromium:148757
Committed: https://crrev.com/39a1c9678e3e925f854b67e5e2ca61ac745824a8
Cr-Commit-Position: refs/heads/master@{#40775}
Patch Set 1 #Patch Set 2 : flag #Patch Set 3 : does V8_EXPORT_PRIVATE fix things? #Patch Set 4 : refactorize #Patch Set 5 : correct one invalid decode test #
Total comments: 5
Patch Set 6 : mtrofin first pass #Patch Set 7 : update unit test for flag switch #Patch Set 8 : override FLAG_expose_wasm earlier in the test #
Messages
Total messages: 45 (36 generated)
The CQ bit was checked by jbroman@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by jbroman@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_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
The CQ bit was checked by jbroman@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 jbroman@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 jbroman@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...
jbroman@chromium.org changed reviewers: + mtrofin@chromium.org
PTAL. I'm not quite sure what the right way to translate the current use of a RuntimeEnabledFeature for this to the V8 side is. I could certainly plumb a boolean to the serializer (and populate it from the RuntimeEnabledFeature), but it seemed like a V8 flag was the more typical way to do this (reachable in Chromium with --js-flags, or by having a Chromium switch map onto a V8 flag).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/03 20:05:28, jbroman wrote: > PTAL. > > I'm not quite sure what the right way to translate the current use of a > RuntimeEnabledFeature for this to the V8 side is. I could certainly plumb a > boolean to the serializer (and populate it from the RuntimeEnabledFeature), but > it seemed like a V8 flag was the more typical way to do this (reachable in > Chromium with --js-flags, or by having a Chromium switch map onto a V8 flag). I think we can do away with the second flag. The feature is tied at the hip to the wasm spec, and is now complete and tested, so may as well just check if FLAG_expose_wasm. Now that we do all this internally, I think it would make sense to delete the public APIs (so v8::WebAssemblyCompiledModule). It can be in a separate CL (actually, probably "must be in separate CL" because Chrome :) )
https://codereview.chromium.org/2471923002/diff/80001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2471923002/diff/80001/src/flag-definitions.h#... src/flag-definitions.h:934: DEFINE_BOOL(serialize_wasm_modules, false, I don't think we need this anymore. We have --expose-wasm, that should suffice. https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc... src/value-serializer.cc:137: kRawBytes = 'y', oh, if we do it this way, might as well call it "R" or something. https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc... src/value-serializer.cc:1489: const unsigned max_valid_size = std::numeric_limits<int>::max(); please use uint32_t instead of unsigned
The CQ bit was checked by jbroman@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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15670) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/15634) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11956)
The CQ bit was checked by jbroman@chromium.org to run a CQ dry run
On 2016/11/03 at 23:29:59, mtrofin wrote: > Now that we do all this internally, I think it would make sense to delete the public APIs (so v8::WebAssemblyCompiledModule). It can be in a separate CL (actually, probably "must be in separate CL" because Chrome :) ) Up to you, but it might be worth holding off until the new structured clone path is at least turned on. (I have no opinion on whether an API like v8::WebAssemblyCompiledModule is likely to be useful for other reasons, e.g. to other embedders.) https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc... src/value-serializer.cc:137: kRawBytes = 'y', On 2016/11/03 at 23:30:05, Mircea Trofin wrote: > oh, if we do it this way, might as well call it "R" or something. Since this hasn't shipped, I suppose we could change it, but we probably ought to change it in both places if so. I don't feel strongly about whether the value of this tag is 'R' or 'y' or 0x01. https://codereview.chromium.org/2471923002/diff/80001/src/value-serializer.cc... src/value-serializer.cc:1489: const unsigned max_valid_size = std::numeric_limits<int>::max(); On 2016/11/03 at 23:30:05, Mircea Trofin wrote: > please use uint32_t instead of unsigned This seemed more obviously correct (int -> unsigned, and then comparison between uint32_t and unsigned, are both guaranteed to be valid). But done (with a static assert to demonstrate this assumption).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
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 jbroman@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...
Just realized I could avoid explicitly installing the wasm js stuff if I just override FLAG_expose_wasm before the contexts are created. I've done that instead because it's shorter, simpler, and more similar to a couple other tests. Will land if CQ approves.
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 jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtrofin@chromium.org Link to the patchset: https://codereview.chromium.org/2471923002/#ps140001 (title: "override FLAG_expose_wasm earlier in the test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Support structured clone of compiled WebAssembly modules. Compatible with the current (unshipped) Blink implementation. BUG=chromium:148757 ========== to ========== Support structured clone of compiled WebAssembly modules. Compatible with the current (unshipped) Blink implementation. BUG=chromium:148757 Committed: https://crrev.com/39a1c9678e3e925f854b67e5e2ca61ac745824a8 Cr-Commit-Position: refs/heads/master@{#40775} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/39a1c9678e3e925f854b67e5e2ca61ac745824a8 Cr-Commit-Position: refs/heads/master@{#40775} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
