Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2836933002:
memory-infra: Never kill memory-infra background thread (Closed)
Descriptionmemory-infra: Never kill memory-infra background thread
We want ultimately to move to a model where memory-infra does not have its own
custom thread and instead uses base::scheduler. However this is risky at the
moment given the tracing entanglement and the various other refactorings going
on. For now we change the logic to never kill the background thread. This will
simplify the logic of the MemoryDumpManager significantly and pave the way for
us to disentangle tracing.
BUG=705564
BUG=702289
Review-Url: https://codereview.chromium.org/2836933002
Cr-Commit-Position: refs/heads/master@{#467671}
Committed: https://chromium.googlesource.com/chromium/src/+/7af20094a387dfa6a3f0eb5fb5b15066bdf90f19
Patch Set 1 #
Total comments: 10
Patch Set 2 : updated for comments #Patch Set 3 : memory_tracing_enabled -> is_enabled_ #Patch Set 4 : rebase #Patch Set 5 : sacrifice to the dark gods of TSAN #Messages
Total messages: 29 (20 generated)
hjd@chromium.org changed reviewers: + primiano@chromium.org, ssid@chromium.org
Second step to disentangling tracing from MDM. ptal, thanks! :)
The CQ bit was checked by hjd@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 with minor comments, but wait for a confirmation from ssid. I'd reword a bit the commit message though. We really want to ultimately move to a model where we don't spin our own thread and use the base::scheduler. However, doing so has some significant risks right now, given the tracing entanglement and all the other refactorings we are doing, which are themselves enough of a big risk. We are going to revisit this decision once the rest has settled and stuck. https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:899: dump_thread->Stop(); \o/ This, as a side effect, should fix crbug.com/702289 . Can you add it to the BUG= in the cl description? https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:500: if (!dump_thread->Start()) { I'd just violently CHECK(started) here. Trying to handle the lack of a thread is going to be too hard and IMHO not something we should try to do, unless crash/ proves us wrong :) So I'd just do here: dump_thread_ = MakeUnique<Thread>("MemoryInfra"); bool started = dump_thread_->Start(); CHECK(started); https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:571: if (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) { I think this extra check is superfluous. The callback will fail once we get to FinalizeDump() because we will find there that tracing was not enabled. Unless I am missing some other case, I'd just rip lines 568-582 (this entire block). ssid wdyt? https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:249: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(); I'd call this GetOrCreateBgTaskRunnerLocked() "GetOrCreate", to reflect the fact that will lazily initialize a new one. "Locked" to reflect the locking need. "Bg" to reflect that this is our "background" thread. Also, just a minor documentation thing. I think we could return a SequencedTaskRunner here. Nothing will change in practice in the .cc file (a SingleThread task runner is also sequenced). Is just about the recent discussions on chromium-dev. "SingleThread" should be used only in the cases where we actually need thread affinity (read: we rely on a TLS slot, which IIRC is not the case here). In all other cases where we just need sequencing, STR is the way to go.
Description was changed from ========== memory-infra: Never kill memory-infra background thread We want to move to a model where, once set up, the memory-infra background thread never goes away. This will simplify the logic of the MemoryDumpManager significantly. BUG=705564 ========== to ========== memory-infra: Never kill memory-infra background thread We want to move to a model where, once set up, the memory-infra background thread never goes away. This will simplify the logic of the MemoryDumpManager significantly. BUG=705564 BUG=702289 ==========
Description was changed from ========== memory-infra: Never kill memory-infra background thread We want to move to a model where, once set up, the memory-infra background thread never goes away. This will simplify the logic of the MemoryDumpManager significantly. BUG=705564 BUG=702289 ========== to ========== memory-infra: Never kill memory-infra background thread We want ultimately to move to a model where memory-infra does not have its own custom thread and instead uses base::scheduler. However this is risky at the moment given the tracing entanglement and the various other refactorings going on. For now we change the logic to never kill the background thread. This will simplify the logic of the MemoryDumpManager significantly and pave the way for us to disentangle tracing. BUG=705564 BUG=702289 ==========
Thanks! Also updated comment. https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:899: dump_thread->Stop(); On 2017/04/24 15:30:58, Primiano Tucci wrote: > \o/ This, as a side effect, should fix crbug.com/702289 . Can you add it to the > BUG= in the cl description? Done. https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:500: if (!dump_thread->Start()) { On 2017/04/24 15:30:58, Primiano Tucci wrote: > I'd just violently CHECK(started) here. > Trying to handle the lack of a thread is going to be too hard and IMHO not > something we should try to do, unless crash/ proves us wrong :) > So I'd just do here: > dump_thread_ = MakeUnique<Thread>("MemoryInfra"); > bool started = dump_thread_->Start(); > CHECK(started); Done. https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:571: if (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) { On 2017/04/24 15:30:58, Primiano Tucci wrote: > I think this extra check is superfluous. The callback will fail once we get to > FinalizeDump() because we will find there that tracing was not enabled. > Unless I am missing some other case, I'd just rip lines 568-582 (this entire > block). > ssid wdyt? > Removing this causes a test (MemoryDumpManagerTest.DisableTracingRightBeforeStartOfDump) to fail. Maybe we should just delete it now though? https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:249: scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner(); On 2017/04/24 15:30:58, Primiano Tucci wrote: > I'd call this GetOrCreateBgTaskRunnerLocked() > "GetOrCreate", to reflect the fact that will lazily initialize a new one. > "Locked" to reflect the locking need. > "Bg" to reflect that this is our "background" thread. > > Also, just a minor documentation thing. I think we could return a > SequencedTaskRunner here. > Nothing will change in practice in the .cc file (a SingleThread task runner is > also sequenced). Is just about the recent discussions on chromium-dev. > "SingleThread" should be used only in the cases where we actually need thread > affinity (read: we rely on a TLS slot, which IIRC is not the case here). > In all other cases where we just need sequencing, STR is the way to go. Done.
The CQ bit was checked by hjd@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.
In description: BUG=705564, 702289 This looks good, maybe we could remove more stuff too. https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:899: dump_thread->Stop(); On 2017/04/24 16:27:59, hjd wrote: > On 2017/04/24 15:30:58, Primiano Tucci wrote: > > \o/ This, as a side effect, should fix crbug.com/702289 . Can you add it to > the > > BUG= in the cl description? > > Done. Yay https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:571: if (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) { On 2017/04/24 16:27:59, hjd wrote: > On 2017/04/24 15:30:58, Primiano Tucci wrote: > > I think this extra check is superfluous. The callback will fail once we get to > > FinalizeDump() because we will find there that tracing was not enabled. > > Unless I am missing some other case, I'd just rip lines 568-582 (this entire > > block). > > ssid wdyt? > > > > Removing this causes a test > (MemoryDumpManagerTest.DisableTracingRightBeforeStartOfDump) to fail. Maybe we > should just delete it now though? I agree with primiano. We could get to a state where we can create dumps without enabling. Let's start with removing such conditions. Also why is this still called "memory_tracing_enabled"? Can you make this is_enabled_ if it's still needed? I don't see any need for this being AtomicWord anymore, we can just make it bool accessed in lock. I'd prefer to remove this entirely. Only issue is with the thread check blacklist. I will remove that in next CL. Please try to get rid of this variable as much as possible?
On 2017/04/25 03:21:48, ssid wrote: > In description: > BUG=705564, 702289 > > This looks good, maybe we could remove more stuff too. > > https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... > File base/trace_event/memory_dump_manager.cc (left): > > https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... > base/trace_event/memory_dump_manager.cc:899: dump_thread->Stop(); > On 2017/04/24 16:27:59, hjd wrote: > > On 2017/04/24 15:30:58, Primiano Tucci wrote: > > > \o/ This, as a side effect, should fix crbug.com/702289 . Can you add it to > > the > > > BUG= in the cl description? > > > > Done. > > Yay > > https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... > File base/trace_event/memory_dump_manager.cc (right): > > https://codereview.chromium.org/2836933002/diff/1/base/trace_event/memory_dum... > base/trace_event/memory_dump_manager.cc:571: if > (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) { > On 2017/04/24 16:27:59, hjd wrote: > > On 2017/04/24 15:30:58, Primiano Tucci wrote: > > > I think this extra check is superfluous. The callback will fail once we get > to > > > FinalizeDump() because we will find there that tracing was not enabled. > > > Unless I am missing some other case, I'd just rip lines 568-582 (this entire > > > block). > > > ssid wdyt? > > > > > > > Removing this causes a test > > (MemoryDumpManagerTest.DisableTracingRightBeforeStartOfDump) to fail. Maybe we > > should just delete it now though? > > I agree with primiano. We could get to a state where we can create dumps without > enabling. Let's start with removing such conditions. > Also why is this still called "memory_tracing_enabled"? Can you make this > is_enabled_ if it's still needed? > I don't see any need for this being AtomicWord anymore, we can just make it bool > accessed in lock. > > I'd prefer to remove this entirely. Only issue is with the thread check > blacklist. I will remove that in next CL. Please try to get rid of this variable > as much as possible? I moved memory_tracing_enabled -> is_enabled_. I'm scared that removing is_enabled_ are the kind of change that will get reverted a million times because introduces some subtle flakiness or something, maybe we could do it in a follow up?
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2836933002/#ps60001 (title: "rebase")
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
Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hjd@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.
The CQ bit was checked by hjd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2836933002/#ps80001 (title: "sacrifice to the dark gods of TSAN")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493303628897740,
"parent_rev": "2ce6dc221fcc30ce679d5e85e926e6fca853281e", "commit_rev":
"7af20094a387dfa6a3f0eb5fb5b15066bdf90f19"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Never kill memory-infra background thread We want ultimately to move to a model where memory-infra does not have its own custom thread and instead uses base::scheduler. However this is risky at the moment given the tracing entanglement and the various other refactorings going on. For now we change the logic to never kill the background thread. This will simplify the logic of the MemoryDumpManager significantly and pave the way for us to disentangle tracing. BUG=705564 BUG=702289 ========== to ========== memory-infra: Never kill memory-infra background thread We want ultimately to move to a model where memory-infra does not have its own custom thread and instead uses base::scheduler. However this is risky at the moment given the tracing entanglement and the various other refactorings going on. For now we change the logic to never kill the background thread. This will simplify the logic of the MemoryDumpManager significantly and pave the way for us to disentangle tracing. BUG=705564 BUG=702289 Review-Url: https://codereview.chromium.org/2836933002 Cr-Commit-Position: refs/heads/master@{#467671} Committed: https://chromium.googlesource.com/chromium/src/+/7af20094a387dfa6a3f0eb5fb5b1... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/7af20094a387dfa6a3f0eb5fb5b1... |
