Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623313005:
[inspector] introduced debug::SetAsyncTaskListener (Closed)
Description[inspector] introduced debug::SetAsyncTaskListener
If installed, this listener is called instead of general DebugEventListener.
BUG=v8:5510
R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org
Review-Url: https://codereview.chromium.org/2623313005
Cr-Commit-Position: refs/heads/master@{#42340}
Committed: https://chromium.googlesource.com/v8/v8/+/f9fbaec39a201812c5e45c9b632f012ab3c531b7
Patch Set 1 #
Total comments: 8
Patch Set 2 : addressed comments #Patch Set 3 : fixed compilation #
Total comments: 12
Patch Set 4 : addressed comments #Patch Set 5 : added missing header #
Messages
Total messages: 29 (19 generated)
Description was changed from ========== [inspector] introduced debug::SetAsyncTaskListener With new listener we can avoid any allocation during AsyncTaskEvent processing. BUG=new API R=yangguo@chromium.org,jgrubber@chromium.org, dgozman@chromium.org ========== to ========== [inspector] introduced debug::SetAsyncTaskListener With new listener we can avoid any allocation during AsyncTaskEvent processing. BUG=v8:5510 R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org ==========
kozyatinskiy@chromium.org changed reviewers: + jgruber@chromium.org - jgrubber@chromium.org
Description was changed from ========== [inspector] introduced debug::SetAsyncTaskListener With new listener we can avoid any allocation during AsyncTaskEvent processing. BUG=v8:5510 R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org ========== to ========== [inspector] introduced debug::SetAsyncTaskListener If installed, this listener is called instead of general DebugEventListener. BUG=v8:5510 R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org ==========
Please take a look. I've CLs that migrates other types of events to separate listeners: https://codereview.chromium.org/2626283002/ (compile events) https://codereview.chromium.org/2622253004/ (break and exceptions events) I'll publish it if proposed implementation looks good to you. After landing we can get rid of using v8::debug::SetDebugEventListener in inspector. And it looks like we can remove support for native event listener from v8-debug.h and other things. It looks like neither node.js(it use JS listener) nor blink use it. Yang, WDYT?
inspector lgtm https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc#newcode1882 src/debug/debug.cc:1882: if (message_handler_ == NULL && nullptr https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc#newcode1882 src/debug/debug.cc:1882: if (message_handler_ == NULL && Let's extract variable |only_foreign_listener_present| or something like that. And we can comment why we ignore the foreign one. https://codereview.chromium.org/2623313005/diff/1/src/debug/interface-types.h File src/debug/interface-types.h (right): https://codereview.chromium.org/2623313005/diff/1/src/debug/interface-types.h... src/debug/interface-types.h:69: enum PromiseDebugActionName { Let's replace kDebugEnqueueRecurring with these 4. https://codereview.chromium.org/2623313005/diff/1/src/inspector/v8-debugger.cc File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2623313005/diff/1/src/inspector/v8-debugger.c... src/inspector/v8-debugger.cc:93: v8::debug::SetAsyncTaskListener(m_isolate, &V8Debugger::v8AsyncTaskListener, Move it next to SetDebugEventListener.
all done, we use name only for enqueue events on inspector side, so I merged type and name everywhere. Yang and Jacob, please take a look! https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc#newcode1882 src/debug/debug.cc:1882: if (message_handler_ == NULL && On 2017/01/13 00:01:48, dgozman wrote: > nullptr Done. https://codereview.chromium.org/2623313005/diff/1/src/debug/debug.cc#newcode1882 src/debug/debug.cc:1882: if (message_handler_ == NULL && On 2017/01/13 00:01:48, dgozman wrote: > Let's extract variable |only_foreign_listener_present| or something like that. > And we can comment why we ignore the foreign one. Done. https://codereview.chromium.org/2623313005/diff/1/src/debug/interface-types.h File src/debug/interface-types.h (right): https://codereview.chromium.org/2623313005/diff/1/src/debug/interface-types.h... src/debug/interface-types.h:69: enum PromiseDebugActionName { On 2017/01/13 00:01:48, dgozman wrote: > Let's replace kDebugEnqueueRecurring with these 4. Done. https://codereview.chromium.org/2623313005/diff/1/src/inspector/v8-debugger.cc File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2623313005/diff/1/src/inspector/v8-debugger.c... src/inspector/v8-debugger.cc:93: v8::debug::SetAsyncTaskListener(m_isolate, &V8Debugger::v8AsyncTaskListener, On 2017/01/13 00:01:48, dgozman wrote: > Move it next to SetDebugEventListener. Done.
The CQ bit was checked by kozyatinskiy@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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) 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...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/20835)
The CQ bit was checked by kozyatinskiy@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 for the AsyncTaskListener part % comments. I only looked at the AsyncTaskListener part, and it'd be great if you could split out Promise-related things into another CL. https://codereview.chromium.org/2623313005/diff/40001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/api.cc#newcode9311 src/api.cc:9311: void debug::SetAsyncTaskListener(Isolate* v8_isolate, Just a general question - what about moving debugging API code to another file? Was there a reason that it needs to be in here? https://codereview.chromium.org/2623313005/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (left): https://codereview.chromium.org/2623313005/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:667: info, PromiseResolveThenableJobInfo::kDebugNameOffset, I'm confused, why does this Cl include promise-related changes? Can you split this into another CL? And please add gsathya@ as a reviewer. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug-interfa... src/debug/debug-interface.h:205: typedef void (*AsyncTaskListener)(debug::PromiseDebugActionType type, int id, Nit: std::function as in e.g. code-stub-assembler.h might be more readable. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc#newc... src/debug/debug.cc:1874: bool only_foreign_listener_present = Not sure this was what dgozman@ intended in his previous comment. The 'only_foreign_listener_present' name does not reflect the current semantics (since it's also true if e.g. event_listener is null). Also, please add the requested comment about why foreign listeners are ignored. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc#newc... src/debug/debug.cc:2162: bool is_active = message_handler_ != NULL || !event_listener_.is_null() || Nit: You could replace the other NULL here while you're at it. https://codereview.chromium.org/2623313005/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:605: case v8::debug::kDebugEnqueueAsyncFunction: OTOH, if it's too messy with splitting out promises from this CL, I'm fine with keeping them together.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! all done. https://codereview.chromium.org/2623313005/diff/40001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/api.cc#newcode9311 src/api.cc:9311: void debug::SetAsyncTaskListener(Isolate* v8_isolate, On 2017/01/13 16:38:38, jgruber wrote: > Just a general question - what about moving debugging API code to another file? > Was there a reason that it needs to be in here? This file contains a lot of useful macros for api, we probably can extract this macros into separate header and then put debug-interface implementation into separate file. I'm strongly support extracting of this because any change of this files force half of V8 recompilation. https://codereview.chromium.org/2623313005/diff/40001/src/builtins/builtins-p... File src/builtins/builtins-promise.cc (left): https://codereview.chromium.org/2623313005/diff/40001/src/builtins/builtins-p... src/builtins/builtins-promise.cc:667: info, PromiseResolveThenableJobInfo::kDebugNameOffset, On 2017/01/13 16:38:38, jgruber wrote: > I'm confused, why does this Cl include promise-related changes? Can you split > this into another CL? And please add gsathya@ as a reviewer. Done. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug-interfa... File src/debug/debug-interface.h (right): https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug-interfa... src/debug/debug-interface.h:205: typedef void (*AsyncTaskListener)(debug::PromiseDebugActionType type, int id, On 2017/01/13 16:38:38, jgruber wrote: > Nit: std::function as in e.g. code-stub-assembler.h might be more readable. Done. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc File src/debug/debug.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc#newc... src/debug/debug.cc:1874: bool only_foreign_listener_present = On 2017/01/13 16:38:38, jgruber wrote: > Not sure this was what dgozman@ intended in his previous comment. The > 'only_foreign_listener_present' name does not reflect the current semantics > (since it's also true if e.g. event_listener is null). > > Also, please add the requested comment about why foreign listeners are ignored. added comment and renamed variable. https://codereview.chromium.org/2623313005/diff/40001/src/debug/debug.cc#newc... src/debug/debug.cc:2162: bool is_active = message_handler_ != NULL || !event_listener_.is_null() || On 2017/01/13 16:38:38, jgruber wrote: > Nit: You could replace the other NULL here while you're at it. Done. https://codereview.chromium.org/2623313005/diff/40001/src/inspector/v8-debugg... File src/inspector/v8-debugger.cc (right): https://codereview.chromium.org/2623313005/diff/40001/src/inspector/v8-debugg... src/inspector/v8-debugger.cc:605: case v8::debug::kDebugEnqueueAsyncFunction: On 2017/01/13 16:38:38, jgruber wrote: > OTOH, if it's too messy with splitting out promises from this CL, I'm fine with > keeping them together. extracted :)
Still lgtm 👍
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/2623313005/#ps60001 (title: "addressed comments")
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
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 kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, jgruber@chromium.org Link to the patchset: https://codereview.chromium.org/2623313005/#ps100001 (title: "added missing header")
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": 100001, "attempt_start_ts": 1484335991393460,
"parent_rev": "154cb8542ae1c74122299f59454b04ad1d9354bb", "commit_rev":
"f9fbaec39a201812c5e45c9b632f012ab3c531b7"}
Message was sent while issue was closed.
Description was changed from ========== [inspector] introduced debug::SetAsyncTaskListener If installed, this listener is called instead of general DebugEventListener. BUG=v8:5510 R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org ========== to ========== [inspector] introduced debug::SetAsyncTaskListener If installed, this listener is called instead of general DebugEventListener. BUG=v8:5510 R=yangguo@chromium.org, jgruber@chromium.org, dgozman@chromium.org Review-Url: https://codereview.chromium.org/2623313005 Cr-Commit-Position: refs/heads/master@{#42340} Committed: https://chromium.googlesource.com/v8/v8/+/f9fbaec39a201812c5e45c9b632f012ab3c... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/v8/v8/+/f9fbaec39a201812c5e45c9b632f012ab3c... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
