Commit Graph

6 Commits

Author SHA1 Message Date
Alexey Kozyatinskiy
ed9b2072a6 [inspector] reworked async instrumentation for promises
Old instrumentation was designed to collect promise creation stack and
promise scheduled stack together. In DevTools for last 6 months we
show only creation stack for promises. We got strong support from users
for new model. Now we can drop support for scheduled stacks and
simplify implementation.

New promise instrumentation is straightforward:
- we send kDebugPromiseThen when promise is created by .then call,
- we send kDebugPromiseCatch when promise is created by .catch call,
- we send kDebugWillHandle before chained callback and kDebugDidHandle
  after chained callback,
- and we send separate kDebugAsyncFunctionPromiseCreated for internal
  promise inside async await function.

Advantages:
- we reduce amount of captured stacks (we do not capture stack for
  promise that constructed not by .then or .catch),
- we can consider async task related to .then and .catch as one shot
  since chained callback is executed once,
- on V8 side we can implement required instrumentation using only
  promise hooks,

Disadvantage:
- see await-promise test, sometimes scheduled stack was useful since we
  add catch handler in native code,

Implementation details:
- on kInit promise hook we need to figure out why promise was created.
  We analyze builtin functions until first user defined function on
  current stack. If there is kAsyncFunctionPromiseCreate function then
  we send kDebugAsyncFunctionPromiseCreated event. If there is
  kPromiseThen or kPromiseCatch then only if this function is bottom
  builtin function we send corresponded event to inspector. We need it
  because Promise.all internally calls .then and in this case we have
  Promise.all and Promise.then on stack at the same time and we do not
  need to report this internally created promise to inspector.

Bug: chromium:778796
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I53f47ce8c5c4a9897655c3396c249ea59529ae47
Reviewed-on: https://chromium-review.googlesource.com/765208
Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-by: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49553}
2017-11-21 16:56:00 +00:00
kozyatinskiy
f61facfdaf [inspector] use creation stack trace as parent for async call chains
Creation stack trace points to the place where callback was actually chained, scheduled points where parent promise was resolved.
For async tasks without creation stack (e.g. setTimeout) we continue to use scheduled as creation since usually they are the same.

BUG=v8:6189
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2868493002
Cr-Original-Commit-Position: refs/heads/master@{#45198}
Committed: e118462f18
Review-Url: https://codereview.chromium.org/2868493002
Cr-Commit-Position: refs/heads/master@{#45266}
2017-05-11 19:21:24 +00:00
kozyatinskiy
fe0d5c7ca8 Revert of [inspector] use creation stack trace as parent for async call chains (patchset #2 id:20001 of https://codereview.chromium.org/2868493002/ )
Reason for revert:
CHECK is too strict.

Original issue's description:
> [inspector] use creation stack trace as parent for async call chains
>
> Creation stack trace points to the place where callback was actually chained, scheduled points where parent promise was resolved.
> For async tasks without creation stack (e.g. setTimeout) we continue to use scheduled as creation since usually they are the same.
>
> BUG=v8:6189
> R=dgozman@chromium.org
>
> Review-Url: https://codereview.chromium.org/2868493002
> Cr-Commit-Position: refs/heads/master@{#45198}
> Committed: e118462f18

TBR=dgozman@chromium.org,alexclarke@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=v8:6189

Review-Url: https://codereview.chromium.org/2868423004
Cr-Commit-Position: refs/heads/master@{#45242}
2017-05-10 21:24:37 +00:00
kozyatinskiy
e118462f18 [inspector] use creation stack trace as parent for async call chains
Creation stack trace points to the place where callback was actually chained, scheduled points where parent promise was resolved.
For async tasks without creation stack (e.g. setTimeout) we continue to use scheduled as creation since usually they are the same.

BUG=v8:6189
R=dgozman@chromium.org

Review-Url: https://codereview.chromium.org/2868493002
Cr-Commit-Position: refs/heads/master@{#45198}
2017-05-09 14:43:15 +00:00
kozyatinskiy
cb545a8c0c [inspector] change target promise for kDebugWillHandle & kDebugDidHandle
- kDebugPromiseCreated(task, parent_task)
This event occurs when promise is created (PromiseHookType::Init). V8Debugger uses this event to maintain task -> parent task map.

- kDebugEnqueueAsyncFunction(task)
This event occurs when first internal promise for async function is created. V8Debugger collects stack trace at this point.

- kDebugEnqueuePromiseResolve(task),
This event occurs when Promise fulfills with resolved status. V8Debugger collects stack trace at this point.

- kDebugEnqueuePromiseReject(task),
This event occurs when Promise fulfills with rejected status. V8Debugger collects stack trace at this point.

- kDebugPromiseCollected,
This event occurs when Promise is collected and no other chained callbacks can be added. V8Debugger removes information about async task for this promise.

- kDebugWillHandle,
This event occurs when chained promise function (either resolve or reject handler) is called. V8Debugger installs parent promise's stack (based on task -> parent_task map) as current if available or current promise's scheduled stack otherwise.

- kDebugDidHandle,
This event occurs after chained promise function has finished. V8Debugger restores asynchronous call chain to previous one.

With this change all instrumentation calls are related to current promise (before WillHandle and DidHandle were related to next async task).

Before V8Debugger supported only the following:
- asyncTaskScheduled(task1)
- asyncTaskStarted(task1)
- asyncTaskFinished(task1)

Now V8Debugger supports the following:
- asyncTaskScheduled(parent_task)
..
- asyncTaskCreated(task, parent_task),
- asyncTaskStarted(task), uses parent_task scheduled stack
- asyncTaskScheduled(task)
- asyncTaskFinished(task)

Additionally: WillHandle and DidHandle were migrated to PromiseHook API.

More details: https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

BUG=v8:5738
R=dgozman@chromium.org,gsathya@chromium.org,yangguo@chromium.org

Review-Url: https://codereview.chromium.org/2650803003
Cr-Commit-Position: refs/heads/master@{#42644}
2017-01-25 07:05:43 +00:00
kozyatinskiy
754736d26c [inspector] async stacks for Promise.then calls...
... which were done after the promise has been resolved.

Goal of this CL - change promise instrumentation to support better callbacks, chained after promise resolution and prepare instrumentation for adding new asyncTaskCreated instrumentation.

Instrumentation changes:
- asyncTaskScheduled(recurring) when promise is fulfilled or rejected,
- asyncTaskCancelled when promise is collected (since [1] we can be sure that promise will survive scheduled microtasks).

Minor changes:
- async task type in inspector <-> debugger API transferred by enum instead of string,
- Debug manages async task ids based on promise objects.

More details: https://docs.google.com/document/d/1u19N45f1gSF7M39mGsycJEK3IPyJgIXCBnWyiPeuJFE

[1] https://codereview.chromium.org/2581503003/

BUG=chromium:632829,v8:5738
R=dgozman@chromium.org,yangguo@chromium.org,gsathya@chromium.org

Review-Url: https://codereview.chromium.org/2578923002
Cr-Commit-Position: refs/heads/master@{#42178}
2017-01-10 12:54:12 +00:00