Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2532433003:
[inspector] Split V8DebuggerScript implementation for wasm (Closed)
|
Created:
4 years ago by Clemens Hammacher Modified:
4 years ago CC:
devtools-reviews_chromium.org, v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[inspector] Split V8DebuggerScript implementation for wasm
Make some methods on V8DebuggerScript virtual and provide the
implementations ActualScript for scripts which are backed by scripts on
V8's side, and WasmVirtualScript for wasm scripts.
The added test case ensures that we at least don't crash on the attempt
to get breakable locations for wasm "scripts", which we did previously.
Returning a reasonable result for wasm will be implemented in a
follow-up commit.
R=yangguo@chromium.org, jgruber@chromium.org
BUG=chromium:667767, chromium:613110
Committed: https://crrev.com/a9017cb0184ec5f1f42b74e26aa1055b9ea6633b
Cr-Commit-Position: refs/heads/master@{#41527}
Patch Set 1 #Patch Set 2 : Remove static initializer #Patch Set 3 : Refactor a bit #
Total comments: 4
Patch Set 4 : Remove duplicate fields #Patch Set 5 : Rebase #Patch Set 6 : Minor refactoring #
Total comments: 8
Patch Set 7 : Rebase #Patch Set 8 : Fix merge error #Patch Set 9 : Rebase #Patch Set 10 : Fix rebase #
Depends on Patchset: Messages
Total messages: 60 (47 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: 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/16811) v8_linux64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng_triggered...)
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...
Description was changed from ========== [inspector] Split V8DebuggerScript implementation for wasm Make some methods on V8DebuggerScript virtual and provide the implementations ActualScript for scripts which are backed by scripts on V8's side, and WasmVirtualScript for wasm scripts. The added test case ensures that we at least don't crash on the attempt to get breakable locations for wasm "scripts", which we did previously. R=yangguo@chromium.org, jgruber@chromium.org BUG=chromium:667767,chromium:613110 ========== to ========== [inspector] Split V8DebuggerScript implementation for wasm Make some methods on V8DebuggerScript virtual and provide the implementations ActualScript for scripts which are backed by scripts on V8's side, and WasmVirtualScript for wasm scripts. The added test case ensures that we at least don't crash on the attempt to get breakable locations for wasm "scripts", which we did previously. Returning a reasonable result for wasm will be implemented in a follow-up commit. R=yangguo@chromium.org, jgruber@chromium.org BUG=chromium:667767,chromium:613110 ==========
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...
Nice! lgtm modulo comments, but you will also need inspector owner reviews. https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:102: m_hash = String16(); m_hash is duplicated in ActualScript and V8DebuggerScript. We set AS::m_hash here, but access VDS::m_hash in VDS::hash() below. https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:117: String16 m_source; Likewise for m_source.
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...
clemensh@chromium.org changed reviewers: + kozyatinskiy@chromium.org
+kozyatinskiy as inspector owner. https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:102: m_hash = String16(); On 2016/11/24 at 15:54:50, jgruber wrote: > m_hash is duplicated in ActualScript and V8DebuggerScript. We set AS::m_hash here, but access VDS::m_hash in VDS::hash() below. Wow, good catch. I remember that I wanted to check that again before uploading... ;) Fixed! https://codereview.chromium.org/2532433003/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger-script.cc:117: String16 m_source; On 2016/11/24 at 15:54:50, jgruber wrote: > Likewise for m_source. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
I have a proposal to always report all fake wasm scripts per module (as we
discussed a while ago). This will remove ad-hoc logic will materializes them at
some unpredictable moments. WDYT? I don't see any performance problems apart
from sending many messages one-by-one, but we can think in the "pack them
together" direction.
This patch aligns with that approach nicely. It will also allow to move
TranslateLocation{Back} method to V8DebuggerScript class, and use that one from
methods handling breakpoints/stack-traces. Then we can, for example, resolve
provisional breakpoints using the same code, just invoke it once in
didParseScript and multiple times in newWasmModule functions.
On 2016/11/25 at 06:20:32, dgozman wrote:
> I have a proposal to always report all fake wasm scripts per module (as we
discussed a while ago). This will remove ad-hoc logic will materializes them at
some unpredictable moments. WDYT? I don't see any performance problems apart
from sending many messages one-by-one, but we can think in the "pack them
together" direction.
>
> This patch aligns with that approach nicely. It will also allow to move
TranslateLocation{Back} method to V8DebuggerScript class, and use that one from
methods handling breakpoints/stack-traces. Then we can, for example, resolve
provisional breakpoints using the same code, just invoke it once in
didParseScript and multiple times in newWasmModule functions.
Sounds awesome, I think this all goes in the right direction. After landing your
proposal (is it implemented already?), we should definitely think about removing
the amount of messages, as it is easily a five or even six digit number. And we
have to make sure that the frontend does not query the source code of all fake
scripts right away.
Do you want to give an LGTM for this one then?
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 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.
I agree with Dmitry that we should report scriptParsed events as soon as wasm module was compiled - in the same way as we do for scripts. And I thought that we discussed it during meeting in MTV. In general this approach looks nice to me. Frontend request content for scripts only when user try to open it by click in navigator. 5 digit or 6 digit amount of scriptParsed events should be not critical for frontend. We can try it and optimize frontend code if needed. Could you implement this logic and upload CL on top of it? I think it's critical to do it before landing this patch. https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:172: script->m_sourceURL = std::move(sourceUrl); Could we move all this logic to ActualScript cstor? https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.h (right): https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.h:68: virtual void setSourceMappingURL(const String16&) {} If sourceMappingUrl getter is abstract, lets make this method abstract too. https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.h:69: virtual void setSource(v8::Local<v8::String>) {} Can we assign new source to m_source here? https://codereview.chromium.org/2532433003/diff/100001/test/inspector/debugge... File test/inspector/debugger/wasm-get-breakable-locations.js (right): https://codereview.chromium.org/2532433003/diff/100001/test/inspector/debugge... test/inspector/debugger/wasm-get-breakable-locations.js:62: function handleScriptParsed(messageObject) { Can we just dump this message object and do nothing until we land getPossibleBreakpoints implementation?
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: Try jobs failed on following builders: v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, no build URL)
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.
https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.cc (right): https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.cc:172: script->m_sourceURL = std::move(sourceUrl); On 2016/11/30 at 18:34:37, kozyatinskiy wrote: > Could we move all this logic to ActualScript cstor? Done, also for WasmVirtualScript. https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... File src/inspector/v8-debugger-script.h (right): https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.h:68: virtual void setSourceMappingURL(const String16&) {} On 2016/11/30 at 18:34:37, kozyatinskiy wrote: > If sourceMappingUrl getter is abstract, lets make this method abstract too. Done. https://codereview.chromium.org/2532433003/diff/100001/src/inspector/v8-debug... src/inspector/v8-debugger-script.h:69: virtual void setSource(v8::Local<v8::String>) {} On 2016/11/30 at 18:34:37, kozyatinskiy wrote: > Can we assign new source to m_source here? Done. But note that this requires to include string-util.h in v8-debugger-script.h. Also, I am not sure in which situations setSource will be called on wasm scripts. It will definitely not impact the code being executed. https://codereview.chromium.org/2532433003/diff/100001/test/inspector/debugge... File test/inspector/debugger/wasm-get-breakable-locations.js (right): https://codereview.chromium.org/2532433003/diff/100001/test/inspector/debugge... test/inspector/debugger/wasm-get-breakable-locations.js:62: function handleScriptParsed(messageObject) { On 2016/11/30 at 18:34:37, kozyatinskiy wrote: > Can we just dump this message object and do nothing until we land getPossibleBreakpoints implementation? Done.
lgtm, Can we move test to CL where getPossibleBreakpoints will be landed?
Are you going to move wasm-translation logic to V8DebuggerScript interface and to implement it in WasmScript?
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: 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/17397) 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_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/17366)
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...
On 2016/12/02 at 18:08:27, kozyatinskiy wrote: > Can we move test to CL where getPossibleBreakpoints will be landed? Done. > Are you going to move wasm-translation logic to V8DebuggerScript interface and to implement it in WasmScript? We can do this, but it's not on my immediate TODO list. Having the translation logic in a separate compilation unit does not hurt IMO.
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 jgruber@chromium.org, kozyatinskiy@chromium.org Link to the patchset: https://codereview.chromium.org/2532433003/#ps180001 (title: "Fix rebase")
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": 180001, "attempt_start_ts": 1481034249190730,
"parent_rev": "c166ac469d17aad52d48fbd8dc5da02864a19be7", "commit_rev":
"b3ade65d773aa7ea36776c8403b9d65cb01dd87d"}
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [inspector] Split V8DebuggerScript implementation for wasm Make some methods on V8DebuggerScript virtual and provide the implementations ActualScript for scripts which are backed by scripts on V8's side, and WasmVirtualScript for wasm scripts. The added test case ensures that we at least don't crash on the attempt to get breakable locations for wasm "scripts", which we did previously. Returning a reasonable result for wasm will be implemented in a follow-up commit. R=yangguo@chromium.org, jgruber@chromium.org BUG=chromium:667767,chromium:613110 ========== to ========== [inspector] Split V8DebuggerScript implementation for wasm Make some methods on V8DebuggerScript virtual and provide the implementations ActualScript for scripts which are backed by scripts on V8's side, and WasmVirtualScript for wasm scripts. The added test case ensures that we at least don't crash on the attempt to get breakable locations for wasm "scripts", which we did previously. Returning a reasonable result for wasm will be implemented in a follow-up commit. R=yangguo@chromium.org, jgruber@chromium.org BUG=chromium:667767,chromium:613110 Committed: https://crrev.com/a9017cb0184ec5f1f42b74e26aa1055b9ea6633b Cr-Commit-Position: refs/heads/master@{#41527} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a9017cb0184ec5f1f42b74e26aa1055b9ea6633b Cr-Commit-Position: refs/heads/master@{#41527} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
