Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1427963002:
[tracing] Move memory-infra dumps to dedicated thread (Closed)
|
Created:
5 years, 1 month ago by Primiano Tucci (use gerrit) Modified:
5 years, 1 month ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@memory-infra-names Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Move memory-infra dumps to dedicated thread
After the CL the memory dump providers which don't specify a
task_runner affinity are invoked on a dedicate thread.
This is to avoid janking the main thread when tracing using
memory-infra.
Also, this CL ensures that the unbound dumpers are always
executed last (when the preemption of the other dumpers
with thread-affinity is over)
BUG=547765
Committed: https://crrev.com/f0dfbf833838e2e0209b998dac2f4333d729ceef
Cr-Commit-Position: refs/heads/master@{#357377}
Patch Set 1 #Patch Set 2 : #
Total comments: 19
Patch Set 3 : Address review + fix browsertest #
Total comments: 8
Patch Set 4 : Final petrcermak comments #
Total comments: 2
Patch Set 5 : remove extra reset() #
Messages
Total messages: 18 (5 generated)
primiano@chromium.org changed reviewers: + petrcermak@chromium.org, ruuda@google.com
Threading, how lovely. I need high-OCD reviewers for this ;-)
https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:500: dump_thread_->Start(); Is |new Thread| or |dump_thread_->Start()| an expensive operation that spins up a system thread? If so, can it be done outside of the lock? https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:556: if (dump_thread_) So |dump_thread_| is set inside the lock everywhere but read outside of it here. Instead you can do scoped_refptr<SingleThreadTaskRunner> dump_thread = nullptr; { AutoLock lock(lock_); dump_thread_.swap(dump_thread); // stuff like before } if (dump_thread) dump_thread.Stop(); https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:586: return !(task_runner < other.task_runner); You replaced < with >= here but we already know that the task runners are not equal due to the conditional above, so I think just writing > here is more readable.
Looks goof overall. Here's a few more comments... Description nit: s/invoked on a dedicate thread/invoked on a dedicated thread/ Thanks, Petr https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:337: // If the dump provider did not specify a thread affinity, dump on the nit: "dump_thread_" is a name so I'd drop the article (at the end of the line) https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:343: // The |dump_thread_| might have been Stop()-ed at this point (if tracing ditto https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:500: dump_thread_->Start(); On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > Is |new Thread| or |dump_thread_->Start()| an expensive operation that spins up > a system thread? If so, can it be done outside of the lock? It seems like that could create a race with OnTraceLogDisabled. How about starting it first and then swapping it in (like Ruud suggested in OnTraceLogDisabled). https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:556: if (dump_thread_) On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > So |dump_thread_| is set inside the lock everywhere but read outside of it here. > Instead you can do > > scoped_refptr<SingleThreadTaskRunner> dump_thread = nullptr; > { > AutoLock lock(lock_); > dump_thread_.swap(dump_thread); > // stuff like before > } > if (dump_thread) > dump_thread.Stop(); I agree that would be better. It's strange (and possibly dangerous) that you suddenly access it unlocked here. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:585: // Ensure that unbound providers (task_runner == nullptr) run always last. supernit: "always run" sounds better than "run always" in my opinion https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:744: // The RequestGlobalMemoryDump() should be NACK-ed because one of the nit: I'd drop the definite article at the beginning https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:745: // threads did die before we had a chance to PostTask onto them. nit: s/did die/died/ (unless you are doing it for emphasis, which I think is unnecessary here)
Thanks guys. Addressed comments, and fixed a browser test. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:337: // If the dump provider did not specify a thread affinity, dump on the On 2015/10/30 17:43:04, petrcermak wrote: > nit: "dump_thread_" is a name so I'd drop the article (at the end of the line) Done. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:343: // The |dump_thread_| might have been Stop()-ed at this point (if tracing On 2015/10/30 17:43:04, petrcermak wrote: > ditto Done. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:500: dump_thread_->Start(); On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > Is |new Thread| or |dump_thread_->Start()| an expensive operation that spins up > a system thread? If so, can it be done outside of the lock? Makes sense, moved to a temporary ref_ptr before and doing only the swap in the lock https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:556: if (dump_thread_) On 2015/10/30 17:43:04, petrcermak wrote: > On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > > So |dump_thread_| is set inside the lock everywhere but read outside of it > here. > > Instead you can do > > > > scoped_refptr<SingleThreadTaskRunner> dump_thread = nullptr; > > { > > AutoLock lock(lock_); > > dump_thread_.swap(dump_thread); > > // stuff like before > > } > > if (dump_thread) > > dump_thread.Stop(); > > I agree that would be better. It's strange (and possibly dangerous) that you > suddenly access it unlocked here. So, tecnically is fine as it is accessed only in OnTraceLogEnabled/Disabled, which are guaranteed to be serialized (locked) from the TraceLog. But I agree that I shouldn't rely on this , so made the change as proposed, I like it. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:585: // Ensure that unbound providers (task_runner == nullptr) run always last. On 2015/10/30 17:43:04, petrcermak wrote: > supernit: "always run" sounds better than "run always" in my opinion Done. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:586: return !(task_runner < other.task_runner); On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > You replaced < with >= here but we already know that the task runners are not > equal due to the conditional above, so I think just writing > here is more > readable. The reason why I did that is that operator< is already defined for TaskRunner. If I want to write >, I have to define a > operator, too much extra boilerplate :) https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:744: // The RequestGlobalMemoryDump() should be NACK-ed because one of the On 2015/10/30 17:43:04, petrcermak wrote: > nit: I'd drop the definite article at the beginning Done. https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager_unittest.cc:745: // threads did die before we had a chance to PostTask onto them. On 2015/10/30 17:43:04, petrcermak wrote: > nit: s/did die/died/ (unless you are doing it for emphasis, which I think is > unnecessary here) Done.
LGTM with a few more observations. Thanks, Petr https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:345: // The task_runner handle, however, should always be non-null. nit: s/The task_runner handle/|task_runner|/ https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:506: dump_thread_ = dump_thread.Pass(); Shouldn't you DCHECK that dump_thread_ was null before this? https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:564: // Thread stops are blocking and must be performed outside of the |lock_| nit: Does the comment apply to line 566 (periodic_dump_timer_.Stop();) as well? If not, then the line should come before the comment. https://codereview.chromium.org/1427963002/diff/30001/components/tracing/chil... File components/tracing/child_trace_message_filter_browsertest.cc (right): https://codereview.chromium.org/1427963002/diff/30001/components/tracing/chil... components/tracing/child_trace_message_filter_browsertest.cc:90: if (message.type() == wait_for_ipc_message_type_ && It might be a good idea to add something along the lines of DCHECK_IMPLIES(message.type() != 0, !wait_for_ipc_closure.is_null()) before the if check (it will make it easier to find potential bugs in the future). Alternatively, you could do (slightly weaker check): if (message.type() == wait_for_ipc_message_type_) { DCHECK(wait_for_ipc_closure_); ... }
lgtm https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/2/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:586: return !(task_runner < other.task_runner); On 2015/11/02 10:22:23, Primiano Tucci wrote: > On 2015/10/30 16:52:41, Ruud van Asseldonk wrote: > > You replaced < with >= here but we already know that the task runners are not > > equal due to the conditional above, so I think just writing > here is more > > readable. > > The reason why I did that is that operator< is already defined for TaskRunner. > If I want to write >, I have to define a > operator, too much extra boilerplate > :) I see. You could call |.get()| to work around.
https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:345: // The task_runner handle, however, should always be non-null. On 2015/11/02 10:35:20, petrcermak wrote: > nit: s/The task_runner handle/|task_runner|/ Done. https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:506: dump_thread_ = dump_thread.Pass(); On 2015/11/02 10:35:20, petrcermak wrote: > Shouldn't you DCHECK that dump_thread_ was null before this? Done. https://codereview.chromium.org/1427963002/diff/30001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:564: // Thread stops are blocking and must be performed outside of the |lock_| On 2015/11/02 10:35:20, petrcermak wrote: > nit: Does the comment apply to line 566 (periodic_dump_timer_.Stop();) as well? > If not, then the line should come before the comment. yes, it does. https://codereview.chromium.org/1427963002/diff/30001/components/tracing/chil... File components/tracing/child_trace_message_filter_browsertest.cc (right): https://codereview.chromium.org/1427963002/diff/30001/components/tracing/chil... components/tracing/child_trace_message_filter_browsertest.cc:90: if (message.type() == wait_for_ipc_message_type_ && On 2015/11/02 10:35:20, petrcermak wrote: > It might be a good idea to add something along the lines of > DCHECK_IMPLIES(message.type() != 0, !wait_for_ipc_closure.is_null()) before the > if check (it will make it easier to find potential bugs in the future). > > Alternatively, you could do (slightly weaker check): > > if (message.type() == wait_for_ipc_message_type_) { > DCHECK(wait_for_ipc_closure_); > ... > } make sense. there is no point not seeing the closure.
Description was changed from ========== [tracing] Move memory-infra dumps to dedicated thread After the CL the memory dump providers which don't specify a task_runner affinity are invoked on a dedicate thread. This is to avoid janking the main thread when tracing using memory-infra. BUG=547765 ========== to ========== [tracing] Move memory-infra dumps to dedicated thread After the CL the memory dump providers which don't specify a task_runner affinity are invoked on a dedicate thread. This is to avoid janking the main thread when tracing using memory-infra. Also, this CL ensures that the unbound dumpers are always executed last (when the preemption of the other dumpers with thread-affinity is over) BUG=547765 ==========
+dsinclair for an owner stamp on components/tracing/child_trace_message_filter_browsertest.cc
simonhatch@chromium.org changed reviewers: + simonhatch@chromium.org
components/tracing/child_trace_message_filter_browsertest.cc lgtm https://codereview.chromium.org/1427963002/diff/50001/components/tracing/chil... File components/tracing/child_trace_message_filter_browsertest.cc (right): https://codereview.chromium.org/1427963002/diff/50001/components/tracing/chil... components/tracing/child_trace_message_filter_browsertest.cc:93: wait_for_ipc_closure_.Reset(); nit: Why is this reset both here and below in WaitForIPCMessage()?
https://codereview.chromium.org/1427963002/diff/50001/components/tracing/chil... File components/tracing/child_trace_message_filter_browsertest.cc (right): https://codereview.chromium.org/1427963002/diff/50001/components/tracing/chil... components/tracing/child_trace_message_filter_browsertest.cc:93: wait_for_ipc_closure_.Reset(); On 2015/11/02 15:42:34, shatch wrote: > nit: Why is this reset both here and below in WaitForIPCMessage()? oh good catch. didn't realize. removed from here.
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ruuda@google.com, petrcermak@chromium.org, simonhatch@chromium.org Link to the patchset: https://codereview.chromium.org/1427963002/#ps70001 (title: "remove extra reset()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1427963002/70001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1427963002/70001
Message was sent while issue was closed.
Committed patchset #5 (id:70001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f0dfbf833838e2e0209b998dac2f4333d729ceef Cr-Commit-Position: refs/heads/master@{#357377} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
