Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2592793003:
Ensure runtime call stats show up for microtasks (Closed)
DescriptionEnsure runtime call stats show up for microtasks
Prior to this, traces recorded through chrome://tracing would not
include time spent in RunMicrotasks.
BUG=v8:5382
Review-Url: https://codereview.chromium.org/2592793003
Cr-Commit-Position: refs/heads/master@{#42316}
Committed: https://chromium.googlesource.com/v8/v8/+/b26cba78152362c1853ed208f0711a879cb3e828
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove TRACE_EVENT0 #Patch Set 3 : Revert "Remove TRACE_EVENT0" #Messages
Total messages: 29 (17 generated)
The CQ bit was checked by jgruber@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...
jgruber@chromium.org changed reviewers: + lpy@chromium.org
To reproduce, trace a microtask-heavy workload such as https://schuay.github.io/streams-demo/. Prior to this CL, RunMicrotasks did not have associated runtime stats data. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", "V8.RunMicrotasks"); What if we remove TRACE_EVENT0 and use "v8.execute" here?
On 2016/12/22 00:38:47, lpy wrote: > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc > File src/isolate.cc (right): > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 > src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", > "V8.RunMicrotasks"); > What if we remove TRACE_EVENT0 and use "v8.execute" here? Also maybe refer to this bug https://bugs.chromium.org/p/v8/issues/detail?id=5382
Description was changed from ========== Ensure runtime call stats show up for microtasks Prior to this, traces recorded through chrome://tracing would not include time spent in RunMicrotasks. BUG= ========== to ========== Ensure runtime call stats show up for microtasks Prior to this, traces recorded through chrome://tracing would not include time spent in RunMicrotasks. BUG=v8:5382 ==========
The CQ bit was checked by jgruber@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/2592793003/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", "V8.RunMicrotasks"); On 2016/12/22 00:38:46, lpy wrote: > What if we remove TRACE_EVENT0 and use "v8.execute" here? Removed TRACE_EVENT0, but using "v8.execute" as the category breaks the display in chrome://tracing so I kept "v8".
On 2016/12/22 09:33:28, jgruber wrote: > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc > File src/isolate.cc (right): > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 > src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", > "V8.RunMicrotasks"); > On 2016/12/22 00:38:46, lpy wrote: > > What if we remove TRACE_EVENT0 and use "v8.execute" here? > > Removed TRACE_EVENT0, but using "v8.execute" as the category breaks the display > in chrome://tracing so I kept "v8". Seems like removing TRACE_EVENT0 breaks blink tests.
The CQ bit was checked by jgruber@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...
PTAL: I went back to the first patch set since the second one broke blink tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/09 08:28:32, jgruber wrote: > PTAL: I went back to the first patch set since the second one broke blink tests. lpy@, friendly ping.
On 2017/01/13 10:00:00, jgruber wrote: > On 2017/01/09 08:28:32, jgruber wrote: > > PTAL: I went back to the first patch set since the second one broke blink > tests. > > lpy@, friendly ping. LGTM
The CQ bit was checked by jgruber@chromium.org
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": 40001, "attempt_start_ts": 1484302911160210,
"parent_rev": "74a2f9b7d3c3d9a9284ab8d5a9d08618b8194966", "commit_rev":
"b26cba78152362c1853ed208f0711a879cb3e828"}
Message was sent while issue was closed.
Description was changed from ========== Ensure runtime call stats show up for microtasks Prior to this, traces recorded through chrome://tracing would not include time spent in RunMicrotasks. BUG=v8:5382 ========== to ========== Ensure runtime call stats show up for microtasks Prior to this, traces recorded through chrome://tracing would not include time spent in RunMicrotasks. BUG=v8:5382 Review-Url: https://codereview.chromium.org/2592793003 Cr-Commit-Position: refs/heads/master@{#42316} Committed: https://chromium.googlesource.com/v8/v8/+/b26cba78152362c1853ed208f0711a879cb... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/b26cba78152362c1853ed208f0711a879cb...
Message was sent while issue was closed.
On 2016/12/22 09:52:31, jgruber wrote: > On 2016/12/22 09:33:28, jgruber wrote: > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc > > File src/isolate.cc (right): > > > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 > > src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", > > "V8.RunMicrotasks"); > > On 2016/12/22 00:38:46, lpy wrote: > > > What if we remove TRACE_EVENT0 and use "v8.execute" here? > > > > Removed TRACE_EVENT0, but using "v8.execute" as the category breaks the > display > > in chrome://tracing so I kept "v8". > > Seems like removing TRACE_EVENT0 breaks blink tests. I'd rather fix blink than have two events here.
Message was sent while issue was closed.
On 2017/01/13 19:16:41, alph wrote: > On 2016/12/22 09:52:31, jgruber wrote: > > On 2016/12/22 09:33:28, jgruber wrote: > > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc > > > File src/isolate.cc (right): > > > > > > https://codereview.chromium.org/2592793003/diff/1/src/isolate.cc#newcode3319 > > > src/isolate.cc:3319: TRACE_EVENT_CALL_STATS_SCOPED(this, "v8", > > > "V8.RunMicrotasks"); > > > On 2016/12/22 00:38:46, lpy wrote: > > > > What if we remove TRACE_EVENT0 and use "v8.execute" here? > > > > > > Removed TRACE_EVENT0, but using "v8.execute" as the category breaks the > > display > > > in chrome://tracing so I kept "v8". > > > > Seems like removing TRACE_EVENT0 breaks blink tests. > > I'd rather fix blink than have two events here. Agreed. The first test failure seems trivial, but I'm not sure why the second failure [0] has an undefined event in mainThreadedEvents. Kinda swamped at the moment, will add to my backlog [1]. [0] https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_r... [1] https://bugs.chromium.org/p/v8/issues/detail?id=5850 |
