Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2395793003:
[wasm] Support recompilation if deserialization fails. (Closed)
|
Created:
4 years, 2 months ago by Mircea Trofin Modified:
4 years, 2 months ago CC:
v8-reviews_googlegroups.com, Yang Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Support recompilation if deserialization fails.
One step closer to the informally-agreed upon specification
that structured cloning will always succeed, meaning, if
we fail to deserialize (e.g. because version mismatch in
serialized format and v8 version), we recompile.
As part of this work, the deserializer will need to become
more resilient to invalid input data, and fail graciously
rather than CHECK-ing. This CL addresses some of that,
sufficient to unblock the current serialization tests.
Subsequent CLs will add more testing and the appropriate
fixes.
BUG=639090
Committed: https://crrev.com/917ef616cccfcee30fc9871d8c6025c17b29b05b
Cr-Commit-Position: refs/heads/master@{#40058}
Patch Set 1 #Patch Set 2 : [wasm] recompile if needed when deserializing #
Total comments: 8
Patch Set 3 : Feedback #
Total comments: 2
Messages
Total messages: 32 (23 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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
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_chromium_gn_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
Description was changed from ========== [wasm] Support recompilation if deserialization fails. BUG= ========== to ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG= ==========
mtrofin@chromium.org changed reviewers: + bradnelson@chromium.org, vogelheim@chromium.org
I realize I need to add back-compat APIs to avoid chrome build from failing, but wanted to get some preliminary feedback on the rest of the CL. Daniel - is the deserializer part OK?
yangguo@chromium.org changed reviewers: + yangguo@chromium.org
lgtm https://codereview.chromium.org/2395793003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2395793003/diff/20001/src/api.cc#newcode7216 src/api.cc:7216: WasmCompiledModuleRawBytesManager ensure_clean_serialization(compiled_part); what's the advantage of this over of simply take compiled_part->module_bytes() here and store it back at the end? Also, do we expect compiled_part->module_bytes() to changed in SerializeWasmModule? https://codereview.chromium.org/2395793003/diff/20001/src/snapshot/code-seria... File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2395793003/diff/20001/src/snapshot/code-seria... src/snapshot/code-serializer.h:96: INVALID_HEADER = 7 Don't forget to add this item to chromium/src/tools/metrics/histograms/histograms.xml, look for "V8CodeCacheRejectReason"
lgtm Generally looks good, but the API compatibility thing needs to be fixed. Also please have a loot at the two nitpicks. https://codereview.chromium.org/2395793003/diff/20001/include/v8.h File include/v8.h (left): https://codereview.chromium.org/2395793003/diff/20001/include/v8.h#oldcode3908 include/v8.h:3908: Isolate* isolate, const SerializedModule& serialized_data); This is an incompatible API change, and one of the bots is complaining that Blink won't build. You'll briefly have to support both old + new API. https://codereview.chromium.org/2395793003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2395793003/diff/20001/src/api.cc#newcode7216 src/api.cc:7216: WasmCompiledModuleRawBytesManager ensure_clean_serialization(compiled_part); On 2016/10/06 07:35:34, Yang wrote: > what's the advantage of this over of simply take compiled_part->module_bytes() > here and store it back at the end? > > Also, do we expect compiled_part->module_bytes() to changed in > SerializeWasmModule? Agree w/ Yang. Also, in case we do want to keep this: Elsewhere in V8 we've called similar helper objects SomethingSomethingScope, which makes it clearer to people familiar w/ V8 code what this does. Maybe something like: SaveModuleBytesScope? Or SaveModuleBytesScope? https://codereview.chromium.org/2395793003/diff/20001/src/api.cc#newcode7252 src/api.cc:7252: // if we cannot deserialize, we recompile Doesn't that comment belongs into DeserializeOrCompile? (Once we're here, we have already made the decision to recompile, right?)
Thanks, will address the comments shortly and upload a new CL. https://codereview.chromium.org/2395793003/diff/20001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2395793003/diff/20001/src/api.cc#newcode7216 src/api.cc:7216: WasmCompiledModuleRawBytesManager ensure_clean_serialization(compiled_part); On 2016/10/06 10:00:50, vogelheim wrote: > On 2016/10/06 07:35:34, Yang wrote: > > what's the advantage of this over of simply take compiled_part->module_bytes() > > here and store it back at the end? Initially I thought there may be error cases to deal with, coming out of the serializer, but now I realize that's not the case, and that in itself wouldn't change the control flow in a way that'd make the simpler code unfeasible. So canning the over-complication. > > Also, do we expect compiled_part->module_bytes() to changed in > > SerializeWasmModule? > Absolutely not. We take it out, however, because: - we want the bytes to be separately handled by the caller. The scenario is: serialize to IndexedDB today; Chrome+v8 updates; deserialize. We want the uncompiled bytes to be serialized separately as raw bytes, and not depend on v8's serializer. - we don't want to pay for the bytes twice. Wasm payloads are large. Uncompiled bytes are megs or tens of megs. Especially on mobile, it'd be sloppy to save them both. > Agree w/ Yang. > > Also, in case we do want to keep this: Elsewhere in V8 we've called similar > helper objects SomethingSomethingScope, which makes it clearer to people > familiar w/ V8 code what this does. Maybe something like: SaveModuleBytesScope? > Or SaveModuleBytesScope? I like the name suggestions, but I think for now I'm taking the simpler code suggestion. https://codereview.chromium.org/2395793003/diff/20001/src/api.cc#newcode7252 src/api.cc:7252: // if we cannot deserialize, we recompile On 2016/10/06 10:00:50, vogelheim wrote: > Doesn't that comment belongs into DeserializeOrCompile? (Once we're here, we > have already made the decision to recompile, right?) yes. https://codereview.chromium.org/2395793003/diff/20001/src/snapshot/code-seria... File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2395793003/diff/20001/src/snapshot/code-seria... src/snapshot/code-serializer.h:96: INVALID_HEADER = 7 On 2016/10/06 07:35:34, Yang wrote: > Don't forget to add this item to > chromium/src/tools/metrics/histograms/histograms.xml, look for > "V8CodeCacheRejectReason" Acknowledged. Didn't know about that entire area, thanks!
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] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG= ========== to ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG=639090 ==========
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
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/2395793003/#ps40001 (title: "Feedback")
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.
Description was changed from ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG=639090 ========== to ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG=639090 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG=639090 ========== to ========== [wasm] Support recompilation if deserialization fails. One step closer to the informally-agreed upon specification that structured cloning will always succeed, meaning, if we fail to deserialize (e.g. because version mismatch in serialized format and v8 version), we recompile. As part of this work, the deserializer will need to become more resilient to invalid input data, and fail graciously rather than CHECK-ing. This CL addresses some of that, sufficient to unblock the current serialization tests. Subsequent CLs will add more testing and the appropriate fixes. BUG=639090 Committed: https://crrev.com/917ef616cccfcee30fc9871d8c6025c17b29b05b Cr-Commit-Position: refs/heads/master@{#40058} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/917ef616cccfcee30fc9871d8c6025c17b29b05b Cr-Commit-Position: refs/heads/master@{#40058}
Message was sent while issue was closed.
titzer@chromium.org changed reviewers: + titzer@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2395793003/diff/40001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2395793003/diff/40001/src/api.cc#newcode7249 src/api.cc:7249: i_isolate, data->GetChars(), data->GetChars() + data->length(), This code is getting a raw pointer to the bytes of a string and handing that out across an API that does a tons of allocation. This is failing now with GC stress. We have to be careful to avoid raw pointers into objects in general. clemensh@ has a fix in progress.
Message was sent while issue was closed.
clemensh@chromium.org changed reviewers: + clemensh@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2395793003/diff/40001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2395793003/diff/40001/src/api.cc#newcode7249 src/api.cc:7249: i_isolate, data->GetChars(), data->GetChars() + data->length(), On 2016/10/07 07:52:55, titzer wrote: > This code is getting a raw pointer to the bytes of a string and handing that out > across an API that does a tons of allocation. This is failing now with GC > stress. We have to be careful to avoid raw pointers into objects in general. > > clemensh@ has a fix in progress. Landed: https://codereview.chromium.org/2402633002
Message was sent while issue was closed.
Patchset #4 (id:60001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
