Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(194)

Issue 2844753002: [inspector] better stacks for promises (Closed)

Created:
3 years, 8 months ago by kozy
Modified:
3 years, 7 months ago
Reviewers:
dgozman
CC:
v8-reviews_googlegroups.com, devtools-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

[inspector] better stacks for promises - we should always set creation async stack if it's available regardless existing of current parent async stack, - we should cleanup parent link iff there is no creation and schedule async stack for parent. Let's consider example: Promise.resolve().then(x => x).then(x => x), there is three promises which will call following instrumentation: 1) created #1 (Promise.resolve()) - collected stack #1 2) scheduled #1 - collected stack #2 3) created #2 with #1 as parent (first .then) - collected stack #3 4) created #3 with #2 as parent (first .then) - collected stack #4 5) started #2 - use stack #2 as scheduled 6) scheduled #2 - collected stack #6 7) finished #2 8) started #3 - use stack #6 as scheduled 9) scheduled #3 - collected stack #7 10) finished #3 If we collect stacks between step 4 and 5, it's possible to collect scheduled stack #2 but still have creation stack for #2 - stack #3 - so we always need to add creation event if scheduled is collected. If we collect stacks between created and scheduled we should not remove parent link even if parent was not scheduled yet. BUG=v8:6189 R=dgozman@chromium.org Review-Url: https://codereview.chromium.org/2844753002 Cr-Commit-Position: refs/heads/master@{#44990} Committed: https://chromium.googlesource.com/v8/v8/+/f2bd913cd459854894447f5422a953d23fa4e7ba

Patch Set 1 #

Total comments: 8

Patch Set 2 : ac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+375 lines, -15 lines) Patch
M src/inspector/v8-debugger.cc View 1 3 chunks +12 lines, -9 lines 0 comments Download
M src/inspector/v8-stack-trace-impl.h View 1 chunk +1 line, -0 lines 0 comments Download
M src/inspector/v8-stack-trace-impl.cc View 1 5 chunks +11 lines, -3 lines 0 comments Download
M test/inspector/debugger/collect-old-async-call-chains-expected.txt View 3 chunks +3 lines, -3 lines 0 comments Download
A test/inspector/debugger/promise-chain-when-limit-hit.js View 1 chunk +51 lines, -0 lines 0 comments Download
A test/inspector/debugger/promise-chain-when-limit-hit-expected.txt View 1 chunk +297 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (9 generated)
kozy
Dmitry, please take a look. https://codereview.chromium.org/2844753002/diff/1/test/inspector/debugger/collect-old-async-call-chains-expected.txt File test/inspector/debugger/collect-old-async-call-chains-expected.txt (left): https://codereview.chromium.org/2844753002/diff/1/test/inspector/debugger/collect-old-async-call-chains-expected.txt#oldcode174 test/inspector/debugger/collect-old-async-call-chains-expected.txt:174: actual async chain len: ...
3 years, 8 months ago (2017-04-26 17:41:39 UTC) #1
dgozman
I failed to understand the case where previous code worked incorrectly. Can we have a ...
3 years, 7 months ago (2017-04-27 17:26:35 UTC) #3
kozy
I added details to description and answered to comments, please take another look. https://codereview.chromium.org/2844753002/diff/1/src/inspector/v8-debugger.cc File ...
3 years, 7 months ago (2017-04-27 18:35:31 UTC) #5
dgozman
https://codereview.chromium.org/2844753002/diff/1/src/inspector/v8-stack-trace-impl.cc File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2844753002/diff/1/src/inspector/v8-stack-trace-impl.cc#newcode80 src/inspector/v8-stack-trace-impl.cc:80: (asyncParent->description() != description || !asyncParent->isEmpty() || We should not ...
3 years, 7 months ago (2017-04-27 22:37:22 UTC) #6
kozy
all done, please take another look. https://codereview.chromium.org/2844753002/diff/1/src/inspector/v8-stack-trace-impl.cc File src/inspector/v8-stack-trace-impl.cc (right): https://codereview.chromium.org/2844753002/diff/1/src/inspector/v8-stack-trace-impl.cc#newcode80 src/inspector/v8-stack-trace-impl.cc:80: (asyncParent->description() != description ...
3 years, 7 months ago (2017-04-28 16:13:30 UTC) #7
dgozman
lgtm
3 years, 7 months ago (2017-04-28 20:59:03 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2844753002/20001
3 years, 7 months ago (2017-04-28 21:05:26 UTC) #14
commit-bot: I haz the power
3 years, 7 months ago (2017-04-28 21:07:11 UTC) #17
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/v8/v8/+/f2bd913cd459854894447f5422a953d23fa...

Powered by Google App Engine
This is Rietveld 408576698