Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1956323002:
Introduce TraceLog::AsyncEnabledStateObserver (Closed)
DescriptionIntroduce TraceLog::AsyncEnabledStateObserver
This CL introduces AsyncEnabledStateObserver as a complement of
the potentially thread-unsafe EnabledStateObserver class. The callbacks
of the new class are called asynchronously as separate tasks, eliminating
the possibility of threading issues when TraceLog and observers are managed
in different threads.
In follow-up CLs, some subclasses of EnabledStateObserver will be migrated
to AsyncEnabledStateObserver instead.
BUG=610629
TEST=will be added in a follow-up CL
Committed: https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0
Cr-Commit-Position: refs/heads/master@{#392895}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Rename to AsyncEnabledStateObserver and add test cases #Patch Set 3 : Fix compile error #Patch Set 4 : Fix more compile error #
Total comments: 16
Patch Set 5 : Style fix #Patch Set 6 : Use dedicated lock for AsyncESO and remove unit test #Patch Set 7 : Follow the same lock usage as ESO #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 29 (11 generated)
Description was changed from ========== [WIP] Introduce ThreadSafeEnabledStateObserver This CL introduces ThreadSafeEnabledStateObserver as a replacement of the thread-unsafe EnabledStateObserver class. In follow-up CLs, subclasses of EnabledStateObserver will be modified to use ThreadSafeEnabledStateObserver instead. When it is all finished, the original EnabledStateObserver class and the prefix "ThreadSafe" of the new class will be removed. BUG=to be added TEST=to be added ========== to ========== Introduce ThreadSafeEnabledStateObserver This CL introduces ThreadSafeEnabledStateObserver as a replacement of the thread-unsafe EnabledStateObserver class. In follow-up CLs, subclasses of EnabledStateObserver will be modified to use ThreadSafeEnabledStateObserver instead. When it is all finished, the original EnabledStateObserver class and the prefix "ThreadSafe" of the new class will be removed. BUG=to be added TEST=to be added ==========
xiaochengh@chromium.org changed reviewers: + primiano@chromium.org, skyostil@chromium.org
This is the patch following the discussion in https://codereview.chromium.org/1923193002. It seems to work, as I tried it with BlameContext in https://codereview.chromium.org/1956333002 and the browser seems to be working correctly. I didn't completely follow the code snippet Primiano gave in https://codereview.chromium.org/1923193002. The current patch appears to be more natural to me, but I might be just misunderstanding something. Please point it out if that is the case. The patch is not ready for landing as I'm still working the unit tests. Guess I should also file a new tracking bug?
This makes great sense to me. RS-LGTM with some comments so you get owner unblocked. Please make sure you have a bug where you describe the issue (copy the conversation in crrev.com/1923193002) and add tests. Many thanks for doing this, sorry for the delay. As usually happens, the bug has been conceptually always here and predates your change, but you were concretely the first one hitting that for real :) https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.cc File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:342: WeakPtr<ThreadSafeEnabledStateObserver> observer_; nit since this is an internal struct, and you are directly using its fields (makes sense) these names should be without final underscore. From [1] "Data members of structs, both static and non-static, are named like ordinary nonmember variables. They do not have the trailing underscores that data members in classes have." [1] https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:343: scoped_refptr<TaskRunner> task_runner_; Can you s/TaskRunner/SequencedTaskRunner/. It will not make any practical difference in this CL. It's purely for documentation/reasoning purposes: you need a sequencing TR if you want to guarantee that you can't race on destruction. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:645: for (auto it = thread_safe_observers_.begin(); I think you can more concisely: for (const auto& it : thread_safe_observers_) which saves one precious line and improves readability a tiny bit https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:1738: int id = thread_safe_observers_.empty() Yeah I think we don't need this ID logic. Just use listener.get() as a key. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.h File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:106: // TODO(xiaochengh): Deprecate this class. Migrate all its subclasses under I will be happy if we can remove all callers to this, not 100% sure you will be able to do that. I wonder if there are cases where we register observers before we have a message loop (I am thinking specifically to the chain TracingControllerImpl::GetInstance() -> MemoryDumpManager->Initialize() -> AddEnabledStateObserver). The truth is that most of the observer out there are indefinitely lived (MemoryDumpManager,TraceNetLog, TraceEventSystemStatsMonitor, android/trace_event_binding.cc) and don't really need this. I think this is really just for BlameContext, V8 sampling profiler. What I am saying is: Makes sense what you are introducing here, not sure you will be able to deprecated this. Try, if you succeed, good. If you don't, remove the todo :) https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:126: // on-demand. I think the important information to convey to this comment is: All observer calls are posted on the task runner where AddEnabledStateObserver was called https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:140: int AddThreadSafeEnabledStateObserver( instea of returning an int, can we just make the TSESO* pointer the actual key (i.e. map<TSESO*, RegisteredTSO>). So that: when you register you can just deref the weakptr to get the key (and this is fine as you will be on the right thread). To unregister you just ask the class to pass you "this". https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:144: size_t GetThreadSafeObserverCountForTest() const; this doesn't seem to be used in the current CL
Description was changed from ========== Introduce ThreadSafeEnabledStateObserver This CL introduces ThreadSafeEnabledStateObserver as a replacement of the thread-unsafe EnabledStateObserver class. In follow-up CLs, subclasses of EnabledStateObserver will be modified to use ThreadSafeEnabledStateObserver instead. When it is all finished, the original EnabledStateObserver class and the prefix "ThreadSafe" of the new class will be removed. BUG=to be added TEST=to be added ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be modified to use ThreadSafeEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ==========
Description was changed from ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be modified to use ThreadSafeEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to use AsyncEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ==========
Description was changed from ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to use AsyncEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ==========
Description was changed from ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=to be added TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=610629 TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ==========
Thanks for the review. PTAL at the revised patch (#4). The new class is renamed to AsyncEnabledStateObserver to better summarize its behavior. Its role is changed from a replacement of ESO to a complement. The plan of deprecating ESO is deprecated. Test cases are added, and a tracking bug is also filed. This time it should be ready for landing. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.cc File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:342: WeakPtr<ThreadSafeEnabledStateObserver> observer_; On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > nit since this is an internal struct, and you are directly using its fields (makes sense) these names should be without final underscore. > > From [1] "Data members of structs, both static and non-static, are named like ordinary nonmember variables. They do not have the trailing underscores that data members in classes have." > > [1] https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:343: scoped_refptr<TaskRunner> task_runner_; On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > Can you s/TaskRunner/SequencedTaskRunner/. It will not make any practical difference in this CL. It's purely for documentation/reasoning purposes: you need a sequencing TR if you want to guarantee that you can't race on destruction. Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:645: for (auto it = thread_safe_observers_.begin(); On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > I think you can more concisely: > for (const auto& it : thread_safe_observers_) > which saves one precious line and improves readability a tiny bit Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:1738: int id = thread_safe_observers_.empty() On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > Yeah I think we don't need this ID logic. Just use listener.get() as a key. Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.h File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:106: // TODO(xiaochengh): Deprecate this class. Migrate all its subclasses under On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > I will be happy if we can remove all callers to this, not 100% sure you will be able to do that. > I wonder if there are cases where we register observers before we have a message loop (I am thinking specifically to the chain TracingControllerImpl::GetInstance() -> MemoryDumpManager->Initialize() -> AddEnabledStateObserver). > > The truth is that most of the observer out there are indefinitely lived (MemoryDumpManager,TraceNetLog, TraceEventSystemStatsMonitor, android/trace_event_binding.cc) and don't really need this. > I think this is really just for BlameContext, V8 sampling profiler. > > What I am saying is: Makes sense what you are introducing here, not sure you will be able to deprecated this. Try, if you succeed, good. If you don't, remove the todo :) Yeah, a deprecation of this class seems too much. A better idea seems to be having two observer interfaces, synchronous and asynchronous. Only some of the existing observer classes need to be migrated to the async interface. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:126: // on-demand. On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > I think the important information to convey to this comment is: > All observer calls are posted on the task runner where AddEnabledStateObserver was called Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:140: int AddThreadSafeEnabledStateObserver( On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > instea of returning an int, can we just make the TSESO* pointer the actual key (i.e. map<TSESO*, RegisteredTSO>). So that: > when you register you can just deref the weakptr to get the key (and this is fine as you will be on the right thread). > To unregister you just ask the class to pass you "this". That's neat. Done. https://codereview.chromium.org/1956323002/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:144: size_t GetThreadSafeObserverCountForTest() const; On 2016/05/09 at 17:18:29, Primiano Tucci wrote: > this doesn't seem to be used in the current CL Removed. It's not even useful in test.
The code LGTM. The test feels a bit too complex (the actual test code has more subtle things than the original code). I suggest you land this without the test, so you get unblocked, and we move the test to a separate CL. In that, I'd keep the testing strategy simpler: In the MockAsyncObserver just keep track of the count of callbacks and the thread they came from. If you want to execute some actions (for unregistering in the callback) you can just keep the methods mocked and bind a lambda using gmock. You can see a concrete example here: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... Thanks a lot for doing this, it's really appreciated. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_event_unittest.cc (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_event_unittest.cc:1195: while (!async_indicator_) hmm don't do this, I think that if this is not volatile some compilers with /o2 might optimize this as a while(true). And we don't want to start putting volatile code. I'd just simplify this test. I think you are overshooting with this test :) Given that you are introductin a rather simple feature, the only think we should cover is whether the observer is called and whether is called on the right thread and that basic unregristration works. Which should be doable using no subtle atomic / wait conditions etc. I think this is just risking to introduce a flaky test https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:644: for (auto& it : async_observers_) I think this could even be +const, i.e. const auto& (not a big deal if that's not the case) https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:1748: return async_observers_.find(listener) != async_observers_.end(); I think that .count(listener) != 0 is, in some STL implementations, a bit more efficient, as you don't need to populate the iterator https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:23: #include "build/build_config.h" add an explicit include to weak_ptr.h as you are using it below https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:105: // on-demand. Please add a comment here saying: Thread safety: due to the synchronous and off-thread nature of the callbacks, which happen on the File thread, this API can only be used only with objects that are indefinitely lived (never destroyed) and which are internally thread-safe (w.r.t state accessed in the callback methods). Usage of this interface is strongly discouraged, in favor of AsyncEnabledStateObserver (below). https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:125: class BASE_EXPORT AsyncEnabledStateObserver { Add: this allows the observer to be safely destroyed, provided that it happens on the same thread that invoked AddAsyncEnabledStateObserver(). https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:457: // and thread_colors_. + and observers lists https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:466: nit: remove the blank line 466 as this logically still belongs to the observer stuff (not sure why there was a blank line in the first place honestly)
lgtm, thanks for putting this together! I agree with Primiano that the test could probably be simplified. We just need to make sure the relevant callbacks fire on the appropriate thread -- trying to prove anything beyond that (e.g., no race conditions) in a unit test is tricky to say the least. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:645: it.second.task_runner->PostTask( nit: Please add braces around this loop. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:734: it.second.task_runner->PostTask( nit: Please add braces around this loop.
Thanks you for the precious comments. PTAL at patch 6 which actually fixes a bug in the previous design which calls TaskRunner::PostTask() inside lock_. This causes a deadlock: TraceLog::SetEnabled()/SetDisabled() => ... => TaskQueueImpl::PushOntoImmediateIncomingQueueLocked() => ... => TaskAnnotator::DidQueueTask() => trace_event_internal::AddTraceEvent(), which tries to acquire the same lock_ again. It was not caught in previous check because the event category it tries to add is disabled-by-default toplevel.flow. My solution is to put the dispatching to async observers outside lock_, and use a new dedicated |async_observer_lock_| to protect access to the |async_observers_| map. This should be thread safe as nothing fancy can be done inside |async_observer_lock_|. I have also followed Primano's comment to remove the unit test and redesign them in a separate CL. All other comments are also addressed. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:644: for (auto& it : async_observers_) On 2016/05/10 at 14:20:26, Primiano Tucci wrote: > I think this could even be +const, i.e. const auto& (not a big deal if that's not the case) Done. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:645: it.second.task_runner->PostTask( On 2016/05/10 at 14:42:16, Sami wrote: > nit: Please add braces around this loop. Done. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:734: it.second.task_runner->PostTask( On 2016/05/10 at 14:42:16, Sami wrote: > nit: Please add braces around this loop. Done. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.cc:1748: return async_observers_.find(listener) != async_observers_.end(); On 2016/05/10 at 14:20:26, Primiano Tucci wrote: > I think that .count(listener) != 0 is, in some STL implementations, a bit more efficient, as you don't need to populate the iterator Let's use ContainsKey() which looks less hacky (and makes it someone else's responsibility to care about efficiency). https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... File base/trace_event/trace_log.h (right): https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:125: class BASE_EXPORT AsyncEnabledStateObserver { On 2016/05/10 at 14:20:26, Primiano Tucci wrote: > Add: this allows the observer to be safely destroyed, provided that it happens on the same thread that invoked AddAsyncEnabledStateObserver(). Done. https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_... base/trace_event/trace_log.h:466: On 2016/05/10 at 14:20:26, Primiano Tucci wrote: > nit: remove the blank line 466 as this logically still belongs to the observer stuff (not sure why there was a blank line in the first place honestly) Done. It's just my personal bad habit of inserting blank lines wherever possible.
On 2016/05/11 05:19:20, Xiaocheng wrote:
> Thanks you for the precious comments. PTAL at patch 6 which actually fixes a
bug
> in the previous design which calls TaskRunner::PostTask() inside lock_. This
> causes a deadlock:
>
> TraceLog::SetEnabled()/SetDisabled() => ... =>
> TaskQueueImpl::PushOntoImmediateIncomingQueueLocked() => ... =>
> TaskAnnotator::DidQueueTask() =>
> trace_event_internal::AddTraceEvent(),
Oh god, yes, definitely. Sorry I should have realized when reviewing.
> which tries to acquire the same lock_ again. It was not caught in previous
check
> because the event category it tries to add is disabled-by-default
toplevel.flow.
Good spot.
> My solution is to put the dispatching to async observers outside lock_, and
use
> a new dedicated |async_observer_lock_| to protect access to the
> |async_observers_| map. This should be thread safe as nothing fancy can be
done
> inside |async_observer_lock_|.
Hmm doing stuff under a lock make always a bit nervous. Can we just do the same
thing we do with the sync observer list that is:
- Just use the lock_ when (de)registering observers.
- Copy the map before leaving the lock.
- PostTask outside the lock while iterating on a copy.
I know that is going to be less efficient, but since OnTraceLog{En,Dis}abled is
not a fastpath I prefer that to having to reason about two locks and a
lock/unlock/lock2 sequence.
> I have also followed Primano's comment to remove the unit test and redesign
them
> in a separate CL. All other comments are also addressed.
SGTM.
>
https://codereview.chromium.org/1956323002/diff/60001/base/trace_event/trace_...
> base/trace_event/trace_log.cc:1748: return async_observers_.find(listener) !=
> async_observers_.end();
> On 2016/05/10 at 14:20:26, Primiano Tucci wrote:
> > I think that .count(listener) != 0 is, in some STL implementations, a bit
more
> efficient, as you don't need to populate the iterator
>
> Let's use ContainsKey() which looks less hacky (and makes it someone else's
> responsibility to care about efficiency).
TIL base's ContainsKey. SGTM
Description was changed from ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=610629 TEST=base_unittests --gtest_filter=TraceEventTestFixture.*Async*Observer* ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=610629 TEST=will be added in a follow-up CL ==========
The CQ bit was checked by xiaochengh@chromium.org
I've followed Primiano's comment #13 and let AsyncESO use lock in the same way as ESO. Since there's no more comment to address, I'm proceeding to commit directly.
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1956323002/#ps120001 (title: "Follow the same lock usage as ESO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1956323002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1956323002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=610629 TEST=will be added in a follow-up CL ========== to ========== Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG=610629 TEST=will be added in a follow-up CL Committed: https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0 Cr-Commit-Position: refs/heads/master@{#392895} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0 Cr-Commit-Position: refs/heads/master@{#392895}
Message was sent while issue was closed.
On 2016/05/11 11:05:39, commit-bot: I haz the power wrote: > Patchset 7 (id:??) landed as > https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0 > Cr-Commit-Position: refs/heads/master@{#392895} PS7 (the one that landed) LGTM. thanks for the patience.
Message was sent while issue was closed.
alph@chromium.org changed reviewers: + alph@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... base/trace_event/trace_log.cc:647: observer_map = async_observers_; You don't need to copy the map. You can iterate over it right here.
Message was sent while issue was closed.
On 2016/05/11 22:35:52, alph wrote: > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > File base/trace_event/trace_log.cc (right): > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > base/trace_event/trace_log.cc:647: observer_map = async_observers_; > You don't need to copy the map. You can iterate over it right here. I think async_obersers_ may be modified when there's a thread adding an observer in.
Message was sent while issue was closed.
On 2016/05/11 23:11:32, lpy wrote: > On 2016/05/11 22:35:52, alph wrote: > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > File base/trace_event/trace_log.cc (right): > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > base/trace_event/trace_log.cc:647: observer_map = async_observers_; > > You don't need to copy the map. You can iterate over it right here. > > I think async_obersers_ may be modified when there's a thread adding an observer > in. It cannot. The iteration would be within AutoLock lock(lock_); block.
Message was sent while issue was closed.
On 2016/05/11 23:20:08, alph wrote: > On 2016/05/11 23:11:32, lpy wrote: > > On 2016/05/11 22:35:52, alph wrote: > > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > File base/trace_event/trace_log.cc (right): > > > > > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > base/trace_event/trace_log.cc:647: observer_map = async_observers_; > > > You don't need to copy the map. You can iterate over it right here. > > > > I think async_obersers_ may be modified when there's a thread adding an > observer > > in. > > It cannot. The iteration would be within AutoLock lock(lock_); block. Oh I see, that makes sense to me.
Message was sent while issue was closed.
On 2016/05/11 at 23:20:08, alph wrote: > On 2016/05/11 23:11:32, lpy wrote: > > On 2016/05/11 22:35:52, alph wrote: > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > File base/trace_event/trace_log.cc (right): > > > > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > base/trace_event/trace_log.cc:647: observer_map = async_observers_; > > > You don't need to copy the map. You can iterate over it right here. > > > > I think async_obersers_ may be modified when there's a thread adding an observer > > in. > > It cannot. The iteration would be within AutoLock lock(lock_); block. Sorry but I'm not sure if I understand correctly. Do you mean putting the iteration (and the PostTask() calls) inside AutoLock(lock_)? If so, there will be a deadlock (detailed in comment #6).
Message was sent while issue was closed.
On 2016/05/12 02:09:44, Xiaocheng wrote: > On 2016/05/11 at 23:20:08, alph wrote: > > On 2016/05/11 23:11:32, lpy wrote: > > > On 2016/05/11 22:35:52, alph wrote: > > > > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > > File base/trace_event/trace_log.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1956323002/diff/120001/base/trace_event/trace... > > > > base/trace_event/trace_log.cc:647: observer_map = async_observers_; > > > > You don't need to copy the map. You can iterate over it right here. > > > > > > I think async_obersers_ may be modified when there's a thread adding an > observer > > > in. > > > > It cannot. The iteration would be within AutoLock lock(lock_); block. > > Sorry but I'm not sure if I understand correctly. > > Do you mean putting the iteration (and the PostTask() calls) inside > AutoLock(lock_)? If so, there will be a deadlock (detailed in comment #6). Oh indeed. Thanks. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
