Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2629853004:
[wasm] Skip serialization of breakpoints and certion stubs (Closed)
|
Created:
3 years, 11 months ago by Clemens Hammacher Modified:
3 years, 11 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Skip serialization of breakpoints and certion stubs
Breakpoints are always re-set by the embedder after compilation, so we
don't want to store the corresponding breakpoint objects.
Also don't serialize WASM_INTERPRETER_ENTRY stubs as they are replaced
by ordinary WASM_FUNCTION code at instantiation anyway, and skip
WASM_TO_JS wrappers which are recompiled on each instantiation.
Instead, we serialize the Illegal builtin, and also use that one
instead of the placeholder when compiling the wasm code initially.
R=titzer@chromium.org, yangguo@chromium.org
BUG=v8:5822
Review-Url: https://codereview.chromium.org/2629853004
Cr-Commit-Position: refs/heads/master@{#42451}
Committed: https://chromium.googlesource.com/v8/v8/+/3c89788373ee71affaba8c874d339aea9c8ef298
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Yang's comment #
Total comments: 1
Patch Set 3 : Use Illegal builtin instead of wasm code placeholder #
Total comments: 4
Patch Set 4 : Move implementation to .cc file #
Depends on Patchset: Messages
Total messages: 40 (26 generated)
The CQ bit was checked by clemensh@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.
yangguo@chromium.org changed reviewers: + yangguo@chromium.org
https://codereview.chromium.org/2629853004/diff/1/src/snapshot/code-serializer.h File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2629853004/diff/1/src/snapshot/code-serialize... src/snapshot/code-serializer.h:96: SerializerReference wasm_code_placeholder_ref_; This seems a bit hacky to me. I think the correct thing to do is to store the wasm_code_placeholder on the WasmCompiledModuleSerializer, add it as attached reference, and in SerializeCodeObject, instead of serializing code objects of type WASM_TO_JS_FUNCTION and WASM_INTERPRETER_ENTRY, replace code_object with the placeholder, and at the end call SerializeGeneric. The result is the same, but this way it's a bit clearer what's going on: for certain code types we want to serialize something else. This is what we do for function code that we do not want to serialize too: we replace it with the lazy-compile built-in.
The CQ bit was checked by clemensh@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...
https://codereview.chromium.org/2629853004/diff/1/src/snapshot/code-serializer.h File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2629853004/diff/1/src/snapshot/code-serialize... src/snapshot/code-serializer.h:96: SerializerReference wasm_code_placeholder_ref_; On 2017/01/13 at 12:45:32, Yang wrote: > This seems a bit hacky to me. I think the correct thing to do is to store the wasm_code_placeholder on the WasmCompiledModuleSerializer, add it as attached reference, and in SerializeCodeObject, instead of serializing code objects of type WASM_TO_JS_FUNCTION and WASM_INTERPRETER_ENTRY, replace code_object with the placeholder, and at the end call SerializeGeneric. > > The result is the same, but this way it's a bit clearer what's going on: for certain code types we want to serialize something else. This is what we do for function code that we do not want to serialize too: we replace it with the lazy-compile built-in. Right, that's cleaner. I don't even have to add the attached reference, right? Changed the patch accordingly.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2629853004/diff/20001/src/wasm/wasm-objects.cc File src/wasm/wasm-objects.cc (right): https://codereview.chromium.org/2629853004/diff/20001/src/wasm/wasm-objects.c... src/wasm/wasm-objects.cc:566: DCHECK(shared->breakpoint_infos()->get(i)->IsUndefined(isolate)); Might as well just make this a CHECK if the whole thing is guarded by a #if DEBUG
On 2017/01/13 16:59:38, titzer wrote: > lgtm > > https://codereview.chromium.org/2629853004/diff/20001/src/wasm/wasm-objects.cc > File src/wasm/wasm-objects.cc (right): > > https://codereview.chromium.org/2629853004/diff/20001/src/wasm/wasm-objects.c... > src/wasm/wasm-objects.cc:566: > DCHECK(shared->breakpoint_infos()->get(i)->IsUndefined(isolate)); > Might as well just make this a CHECK if the whole thing is guarded by a #if > DEBUG is the placeholder on the rootlist or where do you take it from?
On 2017/01/13 at 17:14:28, yangguo wrote: > is the placeholder on the rootlist or where do you take it from? The placeholder is generated right before (de)serialization, so it's always a new one. The identity of this object does not matter. Do you suggest adding it to the root list?
On 2017/01/16 09:01:59, Clemens Hammacher wrote: > On 2017/01/13 at 17:14:28, yangguo wrote: > > is the placeholder on the rootlist or where do you take it from? > > The placeholder is generated right before (de)serialization, so it's always a > new one. The identity of this object does not matter. > Do you suggest adding it to the root list? Looking closer at your code, I think I understand this a bit better. Let's see if my understanding is correct: at serialization time, you have a few code objects that you do not want to serialize. Instead, you want them to be replaced by one placeholder code object at deserialization. Let's do it this way: - Do not create a placeholder for serialization. This seems only to exist to have a unique heap object address. - Let's add a SerializerReference::AddUnbackedAttachedReference() to give you a SerializerReference to use. This reference is not backed by anything at serialization time, so that the serializer would not try to look it up when serializing an object. - In the constructor of WasmCompiledModuleSerializer, create an unbacked attached reference for the placeholder, similar to what you had in patch set 1. - In SerializeCodeObject, revert back to PutAttachedReference, using the unbacked attached reference. - In the deserializer, create the placeholder, and add it as (backed) attached reference so that the deserializer can hook it in. I'm just wondering what the placeholder is for. During deserialization we also have an empty code object that does nothing, and presumably will be replaced too, at some point. In that case, we probably can do something smarter than what I just described. Let's talk when you are in office.
The CQ bit was checked by clemensh@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...
I changed this like discussed offline: We now serialize the Illegal builtin instead of the wasm placeholder, and also use that initially when compiling wasm code. At instantiation, all calls to this builtin are patched with the right wasm or wasm-to-js code. PTAL
Description was changed from ========== [wasm] Skip serialization of breakpoints and certion stubs Breakpoints are always re-set by the embedder after compilation, so we don't want to store the corresponding breakpoint objects. Also don't serialize WASM_INTERPRETER_ENTRY stubs as they are replaced by ordinary WASM_FUNCTION code at instantiation anyway, and skip WASM_TO_JS wrappers which are recompiled on each instantiation. R=titzer@chromium.org BUG=v8:5822 ========== to ========== [wasm] Skip serialization of breakpoints and certion stubs Breakpoints are always re-set by the embedder after compilation, so we don't want to store the corresponding breakpoint objects. Also don't serialize WASM_INTERPRETER_ENTRY stubs as they are replaced by ordinary WASM_FUNCTION code at instantiation anyway, and skip WASM_TO_JS wrappers which are recompiled on each instantiation. Instead, we serialize the Illegal builtin, and also use that one instead of the placeholder when compiling the wasm code initially. R=titzer@chromium.org, yangguo@chromium.org BUG=v8:5822 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/16 11:27:27, Clemens Hammacher wrote: > I changed this like discussed offline: We now serialize the Illegal builtin > instead of the wasm placeholder, and also use that initially when compiling wasm > code. > At instantiation, all calls to this builtin are patched with the right wasm or > wasm-to-js code. > > PTAL lgtm with comments.
... and here are the comments. https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... src/snapshot/code-serializer.h:82: SerializeGeneric(code_object, how_to_code, where_to_point); Can we move this over to the .cc file? https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:457: code->builtin_index() == Builtins::kIllegal; Shouldn't it also be possible to just check for the builtin_index and only DCHECK the code kind?
The CQ bit was checked by clemensh@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...
https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... File src/snapshot/code-serializer.h (right): https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... src/snapshot/code-serializer.h:82: SerializeGeneric(code_object, how_to_code, where_to_point); On 2017/01/16 at 13:20:49, Yang wrote: > Can we move this over to the .cc file? Done. Moved both methods to the .cc, they are virtual so they cannot be inlined anyway. https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:457: code->builtin_index() == Builtins::kIllegal; On 2017/01/16 at 13:20:49, Yang wrote: > Shouldn't it also be possible to just check for the builtin_index and only DCHECK the code kind? I am not sure if I get what you mean. The current code only checks the builtin_index. The kind needs to be checked for the other cases, where we clone a WasmCompiledModule and re-patch the code. Checking for the BUILTIN kind, and DCHECKing that it's Illegal does not work, as we also call other builtins (like StackCheck or Abort).
On 2017/01/16 13:50:19, Clemens Hammacher wrote: > https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... > File src/snapshot/code-serializer.h (right): > > https://codereview.chromium.org/2629853004/diff/40001/src/snapshot/code-seria... > src/snapshot/code-serializer.h:82: SerializeGeneric(code_object, how_to_code, > where_to_point); > On 2017/01/16 at 13:20:49, Yang wrote: > > Can we move this over to the .cc file? > > Done. Moved both methods to the .cc, they are virtual so they cannot be inlined > anyway. > > https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc > File src/wasm/wasm-module.cc (right): > > https://codereview.chromium.org/2629853004/diff/40001/src/wasm/wasm-module.cc... > src/wasm/wasm-module.cc:457: code->builtin_index() == Builtins::kIllegal; > On 2017/01/16 at 13:20:49, Yang wrote: > > Shouldn't it also be possible to just check for the builtin_index and only > DCHECK the code kind? > > I am not sure if I get what you mean. The current code only checks the > builtin_index. The kind needs to be checked for the other cases, where we clone > a WasmCompiledModule and re-patch the code. > Checking for the BUILTIN kind, and DCHECKing that it's Illegal does not work, as > we also call other builtins (like StackCheck or Abort). I see. 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 clemensh@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 clemensh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org Link to the patchset: https://codereview.chromium.org/2629853004/#ps60001 (title: "Move implementation to .cc file")
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": 60001, "attempt_start_ts": 1484739771581620,
"parent_rev": "4f91cee32153211b4bf60856cf8068d00ece2b6e", "commit_rev":
"3c89788373ee71affaba8c874d339aea9c8ef298"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Skip serialization of breakpoints and certion stubs Breakpoints are always re-set by the embedder after compilation, so we don't want to store the corresponding breakpoint objects. Also don't serialize WASM_INTERPRETER_ENTRY stubs as they are replaced by ordinary WASM_FUNCTION code at instantiation anyway, and skip WASM_TO_JS wrappers which are recompiled on each instantiation. Instead, we serialize the Illegal builtin, and also use that one instead of the placeholder when compiling the wasm code initially. R=titzer@chromium.org, yangguo@chromium.org BUG=v8:5822 ========== to ========== [wasm] Skip serialization of breakpoints and certion stubs Breakpoints are always re-set by the embedder after compilation, so we don't want to store the corresponding breakpoint objects. Also don't serialize WASM_INTERPRETER_ENTRY stubs as they are replaced by ordinary WASM_FUNCTION code at instantiation anyway, and skip WASM_TO_JS wrappers which are recompiled on each instantiation. Instead, we serialize the Illegal builtin, and also use that one instead of the placeholder when compiling the wasm code initially. R=titzer@chromium.org, yangguo@chromium.org BUG=v8:5822 Review-Url: https://codereview.chromium.org/2629853004 Cr-Commit-Position: refs/heads/master@{#42451} Committed: https://chromium.googlesource.com/v8/v8/+/3c89788373ee71affaba8c874d339aea9c8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/3c89788373ee71affaba8c874d339aea9c8... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
