Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369073003:
[tracing] Implement Add/RemoveTraceStateObserver for default platform. (Closed)
Description[tracing] Implement Add/RemoveTraceStateObserver for default platform.
BUG=chromium:406277
Committed: https://crrev.com/fcf1bac99a6277234d35797847a01dfdb1f86f7a
Cr-Commit-Position: refs/heads/master@{#39794}
Patch Set 1 #Patch Set 2 : Speculative fix for Win build. #Patch Set 3 : rebaseline #
Total comments: 9
Patch Set 4 : addressing comments. #
Messages
Total messages: 29 (20 generated)
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...
alph@chromium.org changed reviewers: + caseq@chromium.org, fmeawad@chromium.org, jochen@chromium.org
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 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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) 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_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...) v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...) v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/13581) v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...) v8_linux_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...) v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/13539)
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.
lgtm % comment https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/tracing-controller.cc (right): https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/tracing-controller.cc:120: o->OnTraceDisabled(); I'm a bit concerned about this being potentially called from DefaultPlatform::~DefaultPlatform(), from under a lock and with parts of the default platform already dead. Should we just ignore/clear the observers within the destructor?
lgtm https://codereview.chromium.org/2369073003/diff/40001/include/libplatform/v8-... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2369073003/diff/40001/include/libplatform/v8-... include/libplatform/v8-tracing.h:254: std::unique_ptr<base::Mutex> mutex_; nit: maybe rename the mutex to specify that it is used for observers? https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/tracing-controller.cc (right): https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/tracing-controller.cc:104: observers_copy = observers_; nit: I would argue that the copy constructor has the same overhead as the iterator below. So I would put the iterator inside the lock https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/tracing-controller.cc:120: o->OnTraceDisabled(); On 2016/09/27 17:50:22, caseq wrote: > I'm a bit concerned about this being potentially called from > DefaultPlatform::~DefaultPlatform(), from under a lock and with parts of the > default platform already dead. Should we just ignore/clear the observers within > the destructor? We need to disable the observer on stop tracing, if we are collecting cpu profile, we should stop and not wait until the ~ is called. https://codereview.chromium.org/2369073003/diff/40001/test/cctest/libplatform... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2369073003/diff/40001/test/cctest/libplatform... test/cctest/libplatform/test-tracing.cc:345: tracing_controller.StartTracing(trace_config); Can you add a second observer here?
https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/tracing-controller.cc (right): https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/tracing-controller.cc:104: observers_copy = observers_; On 2016/09/27 18:05:49, fmeawad wrote: > nit: I would argue that the copy constructor has the same overhead as the > iterator below. So I would put the iterator inside the lock No. This is about not invoking callbacks from under a lock.
https://codereview.chromium.org/2369073003/diff/40001/include/libplatform/v8-... File include/libplatform/v8-tracing.h (right): https://codereview.chromium.org/2369073003/diff/40001/include/libplatform/v8-... include/libplatform/v8-tracing.h:254: std::unique_ptr<base::Mutex> mutex_; On 2016/09/27 18:05:49, fmeawad wrote: > nit: maybe rename the mutex to specify that it is used for observers? It's now guarding against mode change as well to support OnTraceEnabled firing when trace is already running. So I'd keep the name. https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... File src/libplatform/tracing/tracing-controller.cc (right): https://codereview.chromium.org/2369073003/diff/40001/src/libplatform/tracing... src/libplatform/tracing/tracing-controller.cc:120: o->OnTraceDisabled(); On 2016/09/27 18:05:49, fmeawad wrote: > On 2016/09/27 17:50:22, caseq wrote: > > I'm a bit concerned about this being potentially called from > > DefaultPlatform::~DefaultPlatform(), from under a lock and with parts of the > > default platform already dead. Should we just ignore/clear the observers > within > > the destructor? > > We need to disable the observer on stop tracing, if we are collecting cpu > profile, we should stop and not wait until the ~ is called. I moved StopTracing to the very beginning of platform destructor outside of lock. https://codereview.chromium.org/2369073003/diff/40001/test/cctest/libplatform... File test/cctest/libplatform/test-tracing.cc (right): https://codereview.chromium.org/2369073003/diff/40001/test/cctest/libplatform... test/cctest/libplatform/test-tracing.cc:345: tracing_controller.StartTracing(trace_config); On 2016/09/27 18:05:49, fmeawad wrote: > Can you add a second observer here? Done.
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...
lgtm
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 alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org, fmeawad@chromium.org Link to the patchset: https://codereview.chromium.org/2369073003/#ps60001 (title: "addressing comments.")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Implement Add/RemoveTraceStateObserver for default platform. BUG=chromium:406277 ========== to ========== [tracing] Implement Add/RemoveTraceStateObserver for default platform. BUG=chromium:406277 Committed: https://crrev.com/fcf1bac99a6277234d35797847a01dfdb1f86f7a Cr-Commit-Position: refs/heads/master@{#39794} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fcf1bac99a6277234d35797847a01dfdb1f86f7a Cr-Commit-Position: refs/heads/master@{#39794} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
