Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2436273002:
[Tracing] Implement TracingCategoryObserver. (Closed)
Description[Tracing] Implement TracingCategoryObserver.
This patch implements TracingCategoryObserver to set global flag when a V8
specific category is enabled. Previously, we set a global flag each time when we
encounter a top level trace event, and use it as a global check. With this
patch, we can set a group of flags when tracing is enabled; besides, we make
V8 tracing feature use V8 flags instead of defining its own flag in a messy way.
With this patch, whatever V8 flag we want to imply in tracing, we define another
integer flag, and the original V8 flag will set it to 0x01 when passing by
commandline, tracing will set it to 0x10 when we start tracing and reset the bit
when we stop tracing.
Committed: https://crrev.com/6df8096a00382d10cab1bb2052238e324d66fe54
Cr-Commit-Position: refs/heads/master@{#40659}
Patch Set 1 #Patch Set 2 : fix compilation error #
Total comments: 1
Patch Set 3 : move to new header file v8-tracing #Patch Set 4 : I hope it can fix windows build #Patch Set 5 : apparently last patch breaks more build. #Patch Set 6 : update #Patch Set 7 : rebase #
Messages
Total messages: 67 (57 generated)
The CQ bit was checked by lpy@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...
lpy@chromium.org changed reviewers: + cbruni@chromium.org, fmeawad@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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 lpy@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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27010)
lgtm https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h File src/flag-definitions.h (right): https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h#... src/flag-definitions.h:886: DEFINE_VALUE_IMPLICATION(runtime_call_stats, runtime_stats, 1) "runtime call stats" is by now probably an outdated name anyway :), so I'm all in for --runtime-stats!
On 2016/10/21 08:20:56, Camillo Bruni wrote: > lgtm > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h > File src/flag-definitions.h (right): > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h#... > src/flag-definitions.h:886: DEFINE_VALUE_IMPLICATION(runtime_call_stats, > runtime_stats, 1) > "runtime call stats" is by now probably an outdated name anyway :), so I'm all > in for --runtime-stats! I wish we can just go for --runtime-stats, but it looks like an integer flag will need to pass an integer, maybe I should do a patch to the where we parse the flag so that when we pass --runtime-stats, despite it's a integer flag, we set it to a default value when using like a boolean flag? I created an integer flag here because I want when we pass runtime call stats through command line, we set it to 1, and when we pass it through tracing, we set it to 2, so that we avoid the situation where we set the flag when we are tracing, remember what the original flag value is, and reset the flag back when we stop tracing, instead, we just cancel the bit.
Description was changed from ========== [Tracing] Implement TracingFlag. This patch implements TracingFlag to use as a category check in V8. Instead of checking and setting a flag each time when we encounter a top level trace event, we set a group of flags when tracing is enabled; besides, we make tracing use V8 flags instead of defining its own flag in a messy way. BUG= ========== to ========== [Tracing] Implement TracingFlag. This patch implements TracingFlag to use as a category check in V8. Instead of checking and setting a flag each time when we encounter a top level trace event, we set a group of flags when tracing is enabled; besides, we make tracing use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 1 when passing by commandline, tracing will set it to 2 when we start tracing and reset the bit to 0 when we stop tracing. BUG= ==========
On 2016/10/21 17:33:08, lpy wrote: > On 2016/10/21 08:20:56, Camillo Bruni wrote: > > lgtm > > > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h > > File src/flag-definitions.h (right): > > > > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h#... > > src/flag-definitions.h:886: DEFINE_VALUE_IMPLICATION(runtime_call_stats, > > runtime_stats, 1) > > "runtime call stats" is by now probably an outdated name anyway :), so I'm all > > in for --runtime-stats! > > I wish we can just go for --runtime-stats, but it looks like an integer flag > will need to pass an integer, maybe I should do a patch to the where we parse > the flag so that when we pass --runtime-stats, despite it's a integer flag, we > set it to a default value when using like a boolean flag? > > I created an integer flag here because I want when we pass runtime call stats > through command line, we set it to 1, and when we pass it through tracing, we > set it to 2, so that we avoid the situation where we set the flag when we are > tracing, remember what the original flag value is, and reset the flag back when > we stop tracing, instead, we just cancel the bit. I updated the description.
The CQ bit was checked by lpy@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_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
The CQ bit was checked by lpy@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_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
The CQ bit was checked by lpy@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_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/15063) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/11335) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/16683)
The CQ bit was checked by lpy@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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27105)
The CQ bit was checked by lpy@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_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/27108)
The CQ bit was checked by lpy@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...
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by lpy@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_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 lpy@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...
Patchset #6 (id:120001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@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...
Patchset #6 (id:140001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/21 at 17:38:29, lpy wrote: > On 2016/10/21 17:33:08, lpy wrote: > > On 2016/10/21 08:20:56, Camillo Bruni wrote: > > > lgtm > > > > > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h > > > File src/flag-definitions.h (right): > > > > > > > > https://codereview.chromium.org/2436273002/diff/20001/src/flag-definitions.h#... > > > src/flag-definitions.h:886: DEFINE_VALUE_IMPLICATION(runtime_call_stats, > > > runtime_stats, 1) > > > "runtime call stats" is by now probably an outdated name anyway :), so I'm all > > > in for --runtime-stats! > > > > I wish we can just go for --runtime-stats, but it looks like an integer flag > > will need to pass an integer, maybe I should do a patch to the where we parse > > the flag so that when we pass --runtime-stats, despite it's a integer flag, we > > set it to a default value when using like a boolean flag? > > > > I created an integer flag here because I want when we pass runtime call stats > > through command line, we set it to 1, and when we pass it through tracing, we > > set it to 2, so that we avoid the situation where we set the flag when we are > > tracing, remember what the original flag value is, and reset the flag back when > > we stop tracing, instead, we just cancel the bit. Oh sure make sense. I'm fine with your solution.
lpy@chromium.org changed reviewers: + jochen@chromium.org, yangguo@chromium.org
+jochen and +Yang
lgtm
The CQ bit was checked by lpy@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...
Description was changed from ========== [Tracing] Implement TracingFlag. This patch implements TracingFlag to use as a category check in V8. Instead of checking and setting a flag each time when we encounter a top level trace event, we set a group of flags when tracing is enabled; besides, we make tracing use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 1 when passing by commandline, tracing will set it to 2 when we start tracing and reset the bit to 0 when we stop tracing. BUG= ========== to ========== [Tracing] Implement TracingCategoryObserver. This patch implements TracingCategoryObserver to set global flag when a V8 specific category is enabled. Previously, we set a global flag each time when we encounter a top level trace event, and use it as a global check. With this patch, we can set a group of flags when tracing is enabled; besides, we make V8 tracing feature use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 0x01 when passing by commandline, tracing will set it to 0x10 when we start tracing and reset the bit when we stop tracing. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from cbruni@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2436273002/#ps180001 (title: "rebase")
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.
Description was changed from ========== [Tracing] Implement TracingCategoryObserver. This patch implements TracingCategoryObserver to set global flag when a V8 specific category is enabled. Previously, we set a global flag each time when we encounter a top level trace event, and use it as a global check. With this patch, we can set a group of flags when tracing is enabled; besides, we make V8 tracing feature use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 0x01 when passing by commandline, tracing will set it to 0x10 when we start tracing and reset the bit when we stop tracing. ========== to ========== [Tracing] Implement TracingCategoryObserver. This patch implements TracingCategoryObserver to set global flag when a V8 specific category is enabled. Previously, we set a global flag each time when we encounter a top level trace event, and use it as a global check. With this patch, we can set a group of flags when tracing is enabled; besides, we make V8 tracing feature use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 0x01 when passing by commandline, tracing will set it to 0x10 when we start tracing and reset the bit when we stop tracing. ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Tracing] Implement TracingCategoryObserver. This patch implements TracingCategoryObserver to set global flag when a V8 specific category is enabled. Previously, we set a global flag each time when we encounter a top level trace event, and use it as a global check. With this patch, we can set a group of flags when tracing is enabled; besides, we make V8 tracing feature use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 0x01 when passing by commandline, tracing will set it to 0x10 when we start tracing and reset the bit when we stop tracing. ========== to ========== [Tracing] Implement TracingCategoryObserver. This patch implements TracingCategoryObserver to set global flag when a V8 specific category is enabled. Previously, we set a global flag each time when we encounter a top level trace event, and use it as a global check. With this patch, we can set a group of flags when tracing is enabled; besides, we make V8 tracing feature use V8 flags instead of defining its own flag in a messy way. With this patch, whatever V8 flag we want to imply in tracing, we define another integer flag, and the original V8 flag will set it to 0x01 when passing by commandline, tracing will set it to 0x10 when we start tracing and reset the bit when we stop tracing. Committed: https://crrev.com/6df8096a00382d10cab1bb2052238e324d66fe54 Cr-Commit-Position: refs/heads/master@{#40659} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6df8096a00382d10cab1bb2052238e324d66fe54 Cr-Commit-Position: refs/heads/master@{#40659} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
