Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2289593005:
Introduce v8.execute tracing category with RunMicrotasks event. (Closed)
DescriptionIntroduce v8.execute tracing category with RunMicrotasks event.
The event is used by DevTools to mark microtask execution intervals.
To reduces the overhead the event is only emitted when there are
microtasks to run.
BUG=642228
Committed: https://crrev.com/607c2c329360f1c316994015fa6a4466f6595797
Cr-Commit-Position: refs/heads/master@{#39055}
Patch Set 1 #
Total comments: 2
Patch Set 2 : addressing comment #Messages
Total messages: 19 (9 generated)
alph@chromium.org changed reviewers: + caseq@chromium.org, yangguo@chromium.org
non-owner lgtm https://codereview.chromium.org/2289593005/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2289593005/diff/1/src/isolate.cc#newcode2961 src/isolate.cc:2961: TRACE_EVENT0("v8.execute", "RunMicrotasks"); let's move down to RunMicrotasksInternal?
https://codereview.chromium.org/2289593005/diff/1/src/isolate.cc File src/isolate.cc (right): https://codereview.chromium.org/2289593005/diff/1/src/isolate.cc#newcode2961 src/isolate.cc:2961: TRACE_EVENT0("v8.execute", "RunMicrotasks"); On 2016/08/30 16:16:14, caseq wrote: > let's move down to RunMicrotasksInternal? Done. Also I noticed that originally FireMicrotasksCompletedCallback were guarded inside SuppressMicrotaskExecutionScope. Not sure if it's important, but moved the code to keep the logic.
The CQ bit was checked by alph@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.
alph@chromium.org changed reviewers: + adamk@chromium.org
Adam, could you please take a look.
Not all microtasks are scripting. One I can think of in Blink that's not is the task that handled image loading when dealing with <picture> elements (to choose among several dpi options). Not sure how much of a problem that is for this patch. Thoughts?
On 2016/08/31 18:27:37, adamk wrote: > Not all microtasks are scripting. One I can think of in Blink that's not is the > task that handled image loading when dealing with <picture> elements (to choose > among several dpi options). Not sure how much of a problem that is for this > patch. Thoughts? Thanks for pointing this out. Actually this particualar patch did not mention scripting. It just introduces an event for microtasks execution. I realize that if we later mark it as scripting in DevTools, it can cover some blink native code execution. I think that's fine as we currently do so for binding callbacks as well. So if someone wants to use v8 microtasks to execute native code that's probably ok to mark it as scripting.
lgtm
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org Link to the patchset: https://codereview.chromium.org/2289593005/#ps20001 (title: "addressing comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Introduce v8.execute tracing category with RunMicrotasks event. The event is used by DevTools to mark microtask execution intervals. To reduces the overhead the event is only emitted when there are microtasks to run. BUG=642228 ========== to ========== Introduce v8.execute tracing category with RunMicrotasks event. The event is used by DevTools to mark microtask execution intervals. To reduces the overhead the event is only emitted when there are microtasks to run. BUG=642228 Committed: https://crrev.com/607c2c329360f1c316994015fa6a4466f6595797 Cr-Commit-Position: refs/heads/master@{#39055} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/607c2c329360f1c316994015fa6a4466f6595797 Cr-Commit-Position: refs/heads/master@{#39055} |
