Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820433005:
memory-infra: Start disentangling tracing from memory-infra (Closed)
|
Created:
3 years, 8 months ago by hjd Modified:
3 years, 8 months ago CC:
chromium-reviews, Dirk Pranke, chrome-grc-reviews_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, agrieve+watch_chromium.org, tracing+reviews_chromium.org, danakj+watch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionmemory-infra: Start disentangling tracing from memory-infra
Starts refactoring the tracing specific code from MemoryDumpManager
into a new class MemoryTracingObserver. This shouldn't change the
behaviour of the MemoryDumpManager.
BUG=703184
Review-Url: https://codereview.chromium.org/2820433005
Cr-Commit-Position: refs/heads/master@{#466936}
Committed: https://chromium.googlesource.com/chromium/src/+/4a4589fc42e386ba9b2ad1c153d17dd8180941f8
Patch Set 1 #
Total comments: 33
Patch Set 2 : update for comments #Patch Set 3 : pass config in #
Total comments: 37
Patch Set 4 : update for comments #
Total comments: 10
Patch Set 5 : address a few more comments #Patch Set 6 : rebase mk2 #Patch Set 7 : format #Patch Set 8 : add todo #
Dependent Patchsets: Messages
Total messages: 38 (14 generated)
hjd@chromium.org changed reviewers: + fmeawad@chromium.org, primiano@chromium.org, ssid@chromium.org
early preview of disentangling tracing from memory-infra, comments welcome https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:801: } Can we just remove this now? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_frontend.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:12: TRACE_DISABLED_BY_DEFAULT("memory-infra"); ^ I will remove this and move the constant in a follow up since it ends up being a pretty noisy change. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:21: } ^ This seems very dubious but I'm not sure what the correct way to do this is. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:30: // DCHECK(memory_dump_manager_->IsInitialized()); Is it worth implementing IsInitialized for this DCHECK? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:76: //} I will remove. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_frontend_unittest.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend_unittest.cc:38: #include "testing/gtest/include/gtest/gtest.h" ^I will remove the unused ones. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/trace_log.cc File base/trace_event/trace_log.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.cc:760: } Can't be inline if IsEnabled is virtual. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/trace_log.h File base/trace_event/trace_log.h (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:93: virtual bool IsEnabled(); So I can mock. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/trace_log.... base/trace_event/trace_log.h:127: virtual void RemoveEnabledStateObserver(EnabledStateObserver* listener); So I can mock.
Took an initial quick look (sorry sheriffing today) and added some comments. Overall makes lot of sense. I think the major question I have is that I was expecting to see the TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID moved to the frontend as well. Also, I am not sure I wouldn't mock methods of tracelog for tests. TraceLog is quite simple and I think it's easier to turn it on/off in tests than trying to mock it. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:123: virtual void Enable(); out of cusiority, why virtual? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_frontend.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:12: TRACE_DISABLED_BY_DEFAULT("memory-infra"); On 2017/04/13 14:16:59, hjd wrote: > ^ I will remove this and move the constant in a follow up since it ends up being > a pretty noisy change. Yeah I agree. for the moment just leave it in MDM and we can move it later separately. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:21: } On 2017/04/13 14:16:59, hjd wrote: > ^ This seems very dubious but I'm not sure what the correct way to do this is. This is a bit of an odd pattern. I think it's cleaner and safer (Threading wise) to just have a conventional: static TMF* GetInstance() { static MemoryTracingFrontend* instance = new TMF(); return instance; } and Initialize() being an instance (non-static) method https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:24: TraceLog* trace_log, both TraceLog and MDM are singletons. Why do you need to pass/inject them? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:30: // DCHECK(memory_dump_manager_->IsInitialized()); On 2017/04/13 14:16:59, hjd wrote: > Is it worth implementing IsInitialized for this DCHECK? I don't hink you need to rely on MDM being initialized (the definition of MDM being "initialized" is unfortunataely very whacky)
for the TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID I think what you really
need is some sort of callback or MDM obsever interface that tell you when a dump
happened and gives you all the data.
In the long term that callback should just be the callback that you guys are
building. Unfortunately right now that callback is summarized an not rich
enough, and we need another side channel to get the PMD out of MDM and give it
to MTF (I heard you like 3 letter acronyms).
Actually given that both are singleton I think we can start very easy and
straightforward. Just have a method in TMF like
class TraceMemoryFrontent {
void OnProcessMemoryDump(std::unique_ptr<ProcessMemoryDump>); //Called from MDM
}
and just moved the PMD from MDM::FinalizeDumpAndAddToTrace after you computed
the summarized stuff for the callback
On 2017/04/13 15:29:56, Primiano Tucci wrote:
> for the TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID I think what you
really
> need is some sort of callback or MDM obsever interface that tell you when a
dump
> happened and gives you all the data.
>
> In the long term that callback should just be the callback that you guys are
> building. Unfortunately right now that callback is summarized an not rich
> enough, and we need another side channel to get the PMD out of MDM and give it
> to MTF (I heard you like 3 letter acronyms).
s/3 letter acronyms/TLA/g :)
> Actually given that both are singleton I think we can start very easy and
> straightforward. Just have a method in TMF like
> class TraceMemoryFrontent {
> void OnProcessMemoryDump(std::unique_ptr<ProcessMemoryDump>); //Called from
MDM
> }
> and just moved the PMD from MDM::FinalizeDumpAndAddToTrace after you computed
> the summarized stuff for the callback
I agree we eventually want a callback, lets discuss the best intermediate step
over vc/in person.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.h:123: virtual void Enable(); On 2017/04/13 15:22:37, Primiano Tucci wrote: > out of cusiority, why virtual? So we can mock it in the frontend tests. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_frontend.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:21: } On 2017/04/13 15:22:37, Primiano Tucci wrote: > On 2017/04/13 14:16:59, hjd wrote: > > ^ This seems very dubious but I'm not sure what the correct way to do this is. > > This is a bit of an odd pattern. I think it's cleaner and safer (Threading wise) > to just have a conventional: > static TMF* GetInstance() { > static MemoryTracingFrontend* instance = new TMF(); > return instance; > } > > and Initialize() being an instance (non-static) method It adds a lifecycle to the object though which I was hoping to avoid :( Like it would be possible to have a reference to a TracingFrontend but for none of the methods to work since we hadn't Initialize'd it, this way we can remove the possibility of error. Maybe it would be better for the memory service client to create/hold on to an instance? Rather than creating another pseudo singleton. https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:24: TraceLog* trace_log, On 2017/04/13 15:22:37, Primiano Tucci wrote: > both TraceLog and MDM are singletons. Why do you need to pass/inject them? So this class can be tested in isolation. Otherwise we have to setup/teardown (through a private api which isn't actually used in production since they're both singletons) a ~1000 lines of multithreaded weirdness. It also makes this test faster (no threads), more actionable (failures of MDM/TraceLog won't break this) and simpler (don't have to understand MDM). https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.cc:30: // DCHECK(memory_dump_manager_->IsInitialized()); On 2017/04/13 15:22:37, Primiano Tucci wrote: > On 2017/04/13 14:16:59, hjd wrote: > > Is it worth implementing IsInitialized for this DCHECK? > > I don't hink you need to rely on MDM being initialized (the definition of MDM > being "initialized" is unfortunataely very whacky) I think I shouldn't call MDM->Enable() until after MDM->Initialize()? MDM->Initialize() does stuff like: delegate_ = std::move(delegate); and MDM->Enable() does stuff like: delegate_->IsCoordinator()
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (left): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:801: } On 2017/04/13 14:16:59, hjd wrote: > Can we just remove this now? I think this can be removed along with TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID. As long as we have this event here this should logically stay here? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum... base/trace_event/memory_dump_manager.cc:38: #include "base/trace_event/memory_tracing_frontend.h" Feels like this dependency shouldn't exist? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... File base/trace_event/memory_tracing_frontend.h (right): https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.h:17: // RequestDumpPoint(). The extension by Un(RegisterDumpProvider). What is RequestDumpPoint() https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.h:18: class BASE_EXPORT MemoryTracingFrontend Why is this called frontend? Why not MemoryTracingObserver? This class just observes tracing. What do you mean by "exposed to the rest of the codebase to deal with memory tracing"? Why would rest of the codebase use this class? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.h:23: static void Initialize(MemoryDumpManager*); Why do you need this argument? https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra... base/trace_event/memory_tracing_frontend.h:25: MemoryTracingFrontend(TraceLog*, MemoryDumpManager*); Why are you passing these pointers to singleton? https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat... File services/resource_coordinator/memory/coordinator/coordinator_impl.cc (right): https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat... services/resource_coordinator/memory/coordinator/coordinator_impl.cc:46: MemoryTracingFrontend::Initialize(MemoryDumpManager::GetInstance()); Doesn't look like the right place for initializing. IIRC there was a plan to move the coodrinator to a different process, in which case browser will never get initialized.
I've updated the patch with some changes, ideally we would like
TracingMemoryObserver to depend on MDM and MDM to not know anything about
TracingMemoryObserver but at the moment that is causing a lot of complexity:
* It's awkward to make sure TracingMemoryObserver is initialized in every
process.
* TracingMemoryObserver lifecycle is weirdly tied to MDMs lifecycle.
* We would like to move MDM's code that dumps things into the trace somewhere
else (probably TracingMemoryObserver) but we would either have to:
a) Introduce a new listening API (setOnDumpListener) only for this one case.
b) Do something like TracingMemoryObserver::GetInstance()->AddToTrace() from
MDM which seems ugly.
So to avoid those problems for now (given that we're planning to move this code
again soon to live only in the memory-infra service instead of in every process)
MDM can just create and own the TracingMemoryObserver this avoids the awkward
lifecycle and initialization problems. This means that TracingMemoryObserver
depends on MDM but hopefully we can move TracingMemoryObserver to live in the
process only soon which would remove this dependency.
On 2017/04/14 23:33:09, ssid wrote:
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
> File base/trace_event/memory_dump_manager.cc (left):
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_manager.cc:801: }
> On 2017/04/13 14:16:59, hjd wrote:
> > Can we just remove this now?
>
> I think this can be removed along with
> TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID.
> As long as we have this event here this should logically stay here?
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
> File base/trace_event/memory_dump_manager.cc (right):
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
> base/trace_event/memory_dump_manager.cc:38: #include
> "base/trace_event/memory_tracing_frontend.h"
> Feels like this dependency shouldn't exist?
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
> File base/trace_event/memory_tracing_frontend.h (right):
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
> base/trace_event/memory_tracing_frontend.h:17: // RequestDumpPoint(). The
> extension by Un(RegisterDumpProvider).
> What is RequestDumpPoint()
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
> base/trace_event/memory_tracing_frontend.h:18: class BASE_EXPORT
> MemoryTracingFrontend
> Why is this called frontend? Why not MemoryTracingObserver?
> This class just observes tracing.
> What do you mean by "exposed to the rest of the codebase to deal with memory
> tracing"? Why would rest of the codebase use this class?
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
> base/trace_event/memory_tracing_frontend.h:23: static void
> Initialize(MemoryDumpManager*);
> Why do you need this argument?
>
>
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
> base/trace_event/memory_tracing_frontend.h:25:
MemoryTracingFrontend(TraceLog*,
> MemoryDumpManager*);
> Why are you passing these pointers to singleton?
>
>
https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat...
> File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
> (right):
>
>
https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat...
> services/resource_coordinator/memory/coordinator/coordinator_impl.cc:46:
> MemoryTracingFrontend::Initialize(MemoryDumpManager::GetInstance());
> Doesn't look like the right place for initializing. IIRC there was a plan to
> move the coodrinator to a different process, in which case browser will never
> get initialized.
I've updated the patch with some changes, ideally we would like
TracingMemoryObserver to depend on MDM and MDM to not know anything about
TracingMemoryObserver but at the moment that is causing a lot of complexity:
* It's awkward to make sure TracingMemoryObserver is initialized in every
process.
* TracingMemoryObserver lifecycle is weirdly tied to MDMs lifecycle.
* We would like to move MDM's code that dumps things into the trace somewhere
else (probably TracingMemoryObserver) but we would either have to:
a) Introduce a new listening API (setOnDumpListener) only for this one case.
b) Do something like TracingMemoryObserver::GetInstance()->AddToTrace() from
MDM which seems ugly.
So to avoid those problems for now (given that we're planning to move this code
again soon to live only in the memory-infra service instead of in every process)
MDM can just create and own the TracingMemoryObserver this avoids the awkward
lifecycle and initialization problems. This means that TracingMemoryObserver
depends on MDM but hopefully we can move TracingMemoryObserver to live in the
process only soon which would remove this dependency.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (left):
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:801: }
On 2017/04/14 23:33:09, ssid wrote:
> On 2017/04/13 14:16:59, hjd wrote:
> > Can we just remove this now?
>
> I think this can be removed along with
> TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID.
> As long as we have this event here this should logically stay here?
Okay, I've moved TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID onto it's own
static method on MemoryTracingObserver!
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:38: #include
"base/trace_event/memory_tracing_frontend.h"
On 2017/04/14 23:33:09, ssid wrote:
> Feels like this dependency shouldn't exist?
Acknowledged. See message.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.h (right):
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.h:123: virtual void Enable();
On 2017/04/13 16:32:18, hjd wrote:
> On 2017/04/13 15:22:37, Primiano Tucci wrote:
> > out of cusiority, why virtual?
>
> So we can mock it in the frontend tests.
If we don't want to test this for now I will undo this, but I'll do it last
after we agreed on everything else so I can keep a branch around with all the
changes we need to re-add the tests in case we want them.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
File base/trace_event/memory_tracing_frontend.h (right):
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
base/trace_event/memory_tracing_frontend.h:17: // RequestDumpPoint(). The
extension by Un(RegisterDumpProvider).
On 2017/04/14 23:33:09, ssid wrote:
> What is RequestDumpPoint()
...I'm not sure, I think I must have pasted some random text with vim.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
base/trace_event/memory_tracing_frontend.h:18: class BASE_EXPORT
MemoryTracingFrontend
On 2017/04/14 23:33:09, ssid wrote:
> Why is this called frontend? Why not MemoryTracingObserver?
> This class just observes tracing.
> What do you mean by "exposed to the rest of the codebase to deal with memory
> tracing"? Why would rest of the codebase use this class?
Done.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
base/trace_event/memory_tracing_frontend.h:23: static void
Initialize(MemoryDumpManager*);
On 2017/04/14 23:33:09, ssid wrote:
> Why do you need this argument?
Gone in the new version.
https://codereview.chromium.org/2820433005/diff/1/base/trace_event/memory_tra...
base/trace_event/memory_tracing_frontend.h:25: MemoryTracingFrontend(TraceLog*,
MemoryDumpManager*);
On 2017/04/14 23:33:09, ssid wrote:
> Why are you passing these pointers to singleton?
Code that uses singletons directly is difficult to test well since it is
invisibly and forever coupled to the singleton code. This makes tests hard to
write but it also means any tests that are written are not isolated since they
implicitly test large amounts of behaviour (via a global). Isolation makes tests
fast, targeted and stable so this is a bad property to give. Because of this
where reasonable I try to pass pointers to singletons.
https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat...
File services/resource_coordinator/memory/coordinator/coordinator_impl.cc
(right):
https://codereview.chromium.org/2820433005/diff/1/services/resource_coordinat...
services/resource_coordinator/memory/coordinator/coordinator_impl.cc:46:
MemoryTracingFrontend::Initialize(MemoryDumpManager::GetInstance());
On 2017/04/14 23:33:09, ssid wrote:
> Doesn't look like the right place for initializing. IIRC there was a plan to
> move the coodrinator to a different process, in which case browser will never
> get initialized.
Moved to be done in MDM init.
On 2017/04/19 09:54:42, hjd wrote: > I've updated the patch with some changes, ideally we would like > TracingMemoryObserver to depend on MDM and MDM to not know anything about > TracingMemoryObserver but at the moment that is causing a lot of complexity: > * It's awkward to make sure TracingMemoryObserver is initialized in every > process. > * TracingMemoryObserver lifecycle is weirdly tied to MDMs lifecycle. > * We would like to move MDM's code that dumps things into the trace somewhere > else (probably TracingMemoryObserver) but we would either have to: > a) Introduce a new listening API (setOnDumpListener) only for this one case. > b) Do something like TracingMemoryObserver::GetInstance()->AddToTrace() from > MDM which seems ugly. > > So to avoid those problems for now (given that we're planning to move this code > again soon to live only in the memory-infra service instead of in every process) > MDM can just create and own the TracingMemoryObserver this avoids the awkward > lifecycle and initialization problems. This means that TracingMemoryObserver > depends on MDM but hopefully we can move TracingMemoryObserver to live in the > process only soon which would remove this dependency. Plan SGTM. ssid, we had some discussion. The TL;DR is that now clearing the dependency in the direction MDM -> MTO (Mem Tracing Observer) would be extra indirection layer (a callback) for very little benefit. We'll do that once we can move the MTO to the service, so that it can just consume the dump processed from chrome. Right now my major concern is clearing out all the tracing stuff away from MDM class. Until then, a callback vs a direct call isn't going to be a huge difference and the MDM -> MTO dependency will be one line (in ::FinalizeDumpAndAddToTrace), until the day where we won't need that line at all (either callback or direct call) because we can consume.
ptal, updated to pass config in and remove unittest
Looks good, some comments, nothing major really.
The only major comment is:
so far the bool |success| returned by the callback was guaranteeing that the
dump was succeeded AND added to the trace.
Now we are in a bit of a hybrid mode where the following can happen:
1) A dump is NACK-ed (because of queueing) or fails.
2) A dump succeeds (and you return the summarized values in the new callback)
but tracing is disabled, so nothing shows up in the trace
3) A dump succeeds and tracing is enabled, so it's added to the trace.
Now the question is: what is the semantic of |success| now?
For 1 it's clearly false. For 3 is clearly true. The problem now is 2.
Realistically some clients (telemetry/devtools) really want to know if the dump
was added to the trace. we spotted quite some bugs in the telemetry hardness in
the past thanks to this, so I'd like to keep this semantic.
IMHO there are two ways to solve the problem:
1) turn that bool |success| into an enum {FAILED, SUCCESS,
SUCCESS_AND_ADDED_TO_TRACE}. This requires though to update devtools and will
introduce a slight mismatch between the devtools protocol (which will still have
a boolean) and the MDM api.
2) Add a bool |needs_tracing| to RequestGlobalDump, and make it so that the
success of 2 depends on the request.
We probably need some more discussion on this, so I would say that the best
thing to do in this CL is: neither of them, for the moment keep the current
semantic. Which in practice means:
- Make the MTO function that adds to the trace a bool
- In 2 make success==false, which means that MDM::FinalizeDump should further
bitwise-AND the success value with the return value of MTO.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:739:
MemoryTracingObserver::MaybeAddDumpToTrace(&pmd_async_state->req_args, pid,
I'd call this AddDumpToTraceIf(Tracing)Enabled to make it sound a bit less
mysterious. I know that there is an extra subtlety about the max_dump_level, but
at least AddDumpToTraceIf(Tracing)Enabled gives a hint about the general use
case.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:781: const
TraceConfig::MemoryDumpConfig& memory_dump_config) {
okay for the moment, but I think that in the next CL we should rip this out
(i.e. the TraceConfig argument) and move the IsDumpModeAllowed to the observer.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:785:
TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
I think this should also be moved to the MTO
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.h (right):
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:123: // XXX
Can you add a comment explaining what Enable/Disable do? XXX is slightly too
cryptic :)
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:171: friend class MockMemoryDumpManager;
doesn't seem to be used anymore?
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager.h:289:
std::unique_ptr<MemoryTracingObserver> trace_log_observer_;
I'd just call it tracing_observer_ to keep it consistent with the class name.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1233:
TEST_F(MemoryDumpManagerTest, TestManualEnable) {
I'd s/TestManualEnable/DumpWithTracingDisabled/
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.cc (right):
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:24: enabled_ = false;
initialize these in the ctor initializer list (Ctor(...) : foo(foo), bar(bar)
{... }) so they can be const, see other comment in the h file.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:28: //
DCHECK(memory_dump_manager_->IsInitialized());
remove this?
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:58: if (!enabled_)
this extra state seems to be here only to prevent to call MDM::Disable() if we
see this wihtout seein the enable() (which shouldn't happen really, given the
code above).
I'd remove the enabled_ here and instead make sure that MDM can just no-op in
case of double-disable.
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.h (right):
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:16: // MemoryDumpManager to handle
XXX.
xxx? :)
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:21: static void
MaybeAddDumpToTrace(const MemoryDumpRequestArgs*,
plz make this non-static. It's always weird to see a singleton (well, this is
singleton de facto) class which has static methods. Instead just make
MDM::FinalizeAndAddTotrace non-static. We are slowly migrating to a saner model
where MDM is always operational, so there will be less need for that level of
paranoy
Also I think this should have a bool return value (see top-level comment to this
CL, ping me if I forgot to add it :) )
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:25: MemoryTracingObserver(TraceLog*,
MemoryDumpManager*);
since we don't have any testing need, I'd just use TraceLog::GetInstancE() and
MDM::GetInstance().
I'm in favor of this pattern if there is a need for testing, but this seems no
more
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:33: static bool
IsMemoryInfraTracingEnabled();
I think this can be just a method in the anonymouse namespace of the .cc file
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:34: bool enabled_;
nit: add a blank line between functions and var declarations
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:35: MemoryDumpManager*
memory_dump_manager_;
make both const variables (not pointers), as they are initialized only in the
ctor and never touched again, i.e.
MemoryDumpManager* const memory_dump_manager_;
(which is !== const MemoryDumpManager* because C++ syntax is great)
EDIT: I think we just don't need these at all (see comment above)
https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.h:37: };
DISALLOW_COPY_AND_ASSIGN ( +include base/macros.h)
https://codereview.chromium.org/2820433005/diff/40001/tools/gn/bootstrap/boot...
File tools/gn/bootstrap/bootstrap.py (right):
https://codereview.chromium.org/2820433005/diff/40001/tools/gn/bootstrap/boot...
tools/gn/bootstrap/bootstrap.py:519:
'base/trace_event/memory_allocator_dump.cc',
rebase conflict?
Thanks! :) https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:739: MemoryTracingObserver::MaybeAddDumpToTrace(&pmd_async_state->req_args, pid, On 2017/04/21 10:10:26, Primiano Tucci wrote: > I'd call this AddDumpToTraceIf(Tracing)Enabled to make it sound a bit less > mysterious. I know that there is an extra subtlety about the max_dump_level, but > at least AddDumpToTraceIf(Tracing)Enabled gives a hint about the general use > case. Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:781: const TraceConfig::MemoryDumpConfig& memory_dump_config) { On 2017/04/21 10:10:26, Primiano Tucci wrote: > okay for the moment, but I think that in the next CL we should rip this out > (i.e. the TraceConfig argument) and move the IsDumpModeAllowed to the observer. Acknowledged. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:785: TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported(); On 2017/04/21 10:10:26, Primiano Tucci wrote: > I think this should also be moved to the MTO Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:123: // XXX On 2017/04/21 10:10:26, Primiano Tucci wrote: > Can you add a comment explaining what Enable/Disable do? XXX is slightly too > cryptic :) Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:171: friend class MockMemoryDumpManager; On 2017/04/21 10:10:26, Primiano Tucci wrote: > doesn't seem to be used anymore? Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:289: std::unique_ptr<MemoryTracingObserver> trace_log_observer_; On 2017/04/21 10:10:26, Primiano Tucci wrote: > I'd just call it tracing_observer_ to keep it consistent with the class name. Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager_unittest.cc (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager_unittest.cc:1233: TEST_F(MemoryDumpManagerTest, TestManualEnable) { On 2017/04/21 10:10:26, Primiano Tucci wrote: > I'd s/TestManualEnable/DumpWithTracingDisabled/ Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.cc (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:24: enabled_ = false; On 2017/04/21 10:10:26, Primiano Tucci wrote: > initialize these in the ctor initializer list (Ctor(...) : foo(foo), bar(bar) > {... }) so they can be const, see other comment in the h file. Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:28: // DCHECK(memory_dump_manager_->IsInitialized()); On 2017/04/21 10:10:27, Primiano Tucci wrote: > remove this? Done, thanks! https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:58: if (!enabled_) On 2017/04/21 10:10:27, Primiano Tucci wrote: > this extra state seems to be here only to prevent to call MDM::Disable() if we > see this wihtout seein the enable() (which shouldn't happen really, given the > code above). > I'd remove the enabled_ here and instead make sure that MDM can just no-op in > case of double-disable. Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:16: // MemoryDumpManager to handle XXX. On 2017/04/21 10:10:27, Primiano Tucci wrote: > xxx? :) Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:21: static void MaybeAddDumpToTrace(const MemoryDumpRequestArgs*, On 2017/04/21 10:10:27, Primiano Tucci wrote: > plz make this non-static. It's always weird to see a singleton (well, this is > singleton de facto) class which has static methods. Instead just make > MDM::FinalizeAndAddTotrace non-static. We are slowly migrating to a saner model > where MDM is always operational, so there will be less need for that level of > paranoy > > Also I think this should have a bool return value (see top-level comment to this > CL, ping me if I forgot to add it :) ) Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:25: MemoryTracingObserver(TraceLog*, MemoryDumpManager*); On 2017/04/21 10:10:27, Primiano Tucci wrote: > since we don't have any testing need, I'd just use TraceLog::GetInstancE() and > MDM::GetInstance(). > I'm in favor of this pattern if there is a need for testing, but this seems no > more It seems to me that using the singletons directly is just making debt for ourselves to clean up later. Part of our problem is we have a bunch of singletons, some with complex life cycles, which all talk to each other. I can kind of see the argument for using TraceLog::GetInstance() but MDM constructs MTO - it seems so weird for MDM to create MTO but then for MTO to talk back to MDM via MDM::GetInstance(). Why should MTO care that MDM happens to be a singleton when it is no harder to pass MDM into MTO since MDM creates MTO? For me the principal value of using GetInstance is that it means you don't have to pass tons unrelated things to just to construct an object - but TraceLog and MDM aren't unrelated to MTO they are the principal objects MTO ought to know about to do its job. Is there a downside I'm missing? I would prefer to keep this pattern but lets discuss :) https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:33: static bool IsMemoryInfraTracingEnabled(); On 2017/04/21 10:10:27, Primiano Tucci wrote: > I think this can be just a method in the anonymouse namespace of the .cc file Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:34: bool enabled_; On 2017/04/21 10:10:27, Primiano Tucci wrote: > nit: add a blank line between functions and var declarations Done. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:35: MemoryDumpManager* memory_dump_manager_; On 2017/04/21 10:10:27, Primiano Tucci wrote: > make both const variables (not pointers), as they are initialized only in the > ctor and never touched again, i.e. > > MemoryDumpManager* const memory_dump_manager_; > (which is !== const MemoryDumpManager* because C++ syntax is great) > > EDIT: I think we just don't need these at all (see comment above) See comment above, https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:37: }; On 2017/04/21 10:10:27, Primiano Tucci wrote: > DISALLOW_COPY_AND_ASSIGN ( +include base/macros.h) Done. https://codereview.chromium.org/2820433005/diff/40001/tools/gn/bootstrap/boot... File tools/gn/bootstrap/bootstrap.py (right): https://codereview.chromium.org/2820433005/diff/40001/tools/gn/bootstrap/boot... tools/gn/bootstrap/bootstrap.py:519: 'base/trace_event/memory_allocator_dump.cc', On 2017/04/21 10:10:27, Primiano Tucci wrote: > rebase conflict? Thanks!
LGTM with some final comments. Also plz expand a little bit the CL description. I'd also add something like: this is a refactoring, no behavioral change is expected here. https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2820433005/diff/40001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:25: MemoryTracingObserver(TraceLog*, MemoryDumpManager*); On 2017/04/21 11:51:58, hjd wrote: > On 2017/04/21 10:10:27, Primiano Tucci wrote: > > since we don't have any testing need, I'd just use TraceLog::GetInstancE() and > > MDM::GetInstance(). > > I'm in favor of this pattern if there is a need for testing, but this seems no > > more > > It seems to me that using the singletons directly is just making debt for > ourselves to clean up later. I might be overlooking some future plan, but is this something we will ever cleanup later? The only reason I see right now to change this, is if we want to test this class to inject mocks. But as we discussed, there seems to be very little to test here and the test should probably be end-to-end to be meaningful. > Part of our problem is we have a bunch of > singletons, some with complex life cycles, which all talk to each other. Well IMHO extra fields just increase cognitive load. When I see a field like memory_dump_manager_ the first thing I ask myself is: "who owns this? what is its lifetime? There must be a reason why we cache this field". > I can > kind of see the argument for using TraceLog::GetInstance() but MDM constructs > MTO - it seems so weird for MDM to create MTO but then for MTO to talk back to > MDM via MDM::GetInstance(). well, MDM is a (real) singleton so in general should be fine to have something which has a narrower scope to refer to the owning singleton using GetInstance(). > Why should MTO care that MDM happens to be a > singleton when it is no harder to pass MDM into MTO since MDM creates MTO? For > me the principal value of using GetInstance is that it means you don't have to > pass tons unrelated things to just to construct an object - but TraceLog and MDM > aren't unrelated to MTO they are the principal objects MTO ought to know about > to do its job. Is there a downside I'm missing? My only driving principle here is simplicity: don't add extra code/variables unless we need them. It's nothing about performance or memory, just readability and simplicity. > I would prefer to keep this pattern but lets discuss :) Honestly this is not something I care too much, so feel free to do what you think it's better. It won't be a major game changer in either case and we have other game changer CLs to work on :) So whatever works really https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:743: // A dump only counts as successful if we also managed to add it to the not sure we really need this comment. The nice thing about having good variable names is that this statement becomes IMHO obvious by just reading the code itself. https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:128: // Tearsdown the MemoryDumpManager thread and various other state set up by nit, use blank space to separate comments when you have a situation like: code //comment code to code //comment code https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.cc (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:61: enabled_ = true; sorry I just realized my comment was a bit ambiguous. I actually meant to suggest to remove the full |enabled_| field. Down below in ::AddDumpToTrace you can just use IsMemoryInfraTracingEnabled() https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:74: if (!enabled_) this should really be IsMemoryInfraTracingEnabled(). You don't care too much if tracing itself is enabled, but if the memory-infra category is. https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:23: bool AddDumpToTraceIfEnabled(const MemoryDumpRequestArgs*, nit: now that is not a static move this down, ctor/dtor should go first
Description was changed from ========== memory-infra: Start disentangling tracing from memory-infra BUG=703184 ========== to ========== memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 ==========
Description was changed from ========== memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 ========== to ========== memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 ==========
https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.cc:743: // A dump only counts as successful if we also managed to add it to the On 2017/04/21 13:14:16, Primiano Tucci wrote: > not sure we really need this comment. The nice thing about having good variable > names is that this statement becomes IMHO obvious by just reading the code > itself. Done. https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:128: // Tearsdown the MemoryDumpManager thread and various other state set up by On 2017/04/21 13:14:16, Primiano Tucci wrote: > nit, use blank space to separate comments when you have a situation like: > > code > //comment > code > > to > > code > > //comment > code done, thanks! https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.cc (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:61: enabled_ = true; On 2017/04/21 13:14:17, Primiano Tucci wrote: > sorry I just realized my comment was a bit ambiguous. I actually meant to > suggest to remove the full |enabled_| field. Down below in ::AddDumpToTrace you > can just use IsMemoryInfraTracingEnabled() Done. https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.cc:74: if (!enabled_) On 2017/04/21 13:14:16, Primiano Tucci wrote: > this should really be IsMemoryInfraTracingEnabled(). You don't care too much if > tracing itself is enabled, but if the memory-infra category is. Done. https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... File base/trace_event/memory_tracing_observer.h (right): https://codereview.chromium.org/2820433005/diff/60001/base/trace_event/memory... base/trace_event/memory_tracing_observer.h:23: bool AddDumpToTraceIfEnabled(const MemoryDumpRequestArgs*, On 2017/04/21 13:14:17, Primiano Tucci wrote: > nit: now that is not a static move this down, ctor/dtor should go first Done.
hjd@chromium.org changed reviewers: + thakis@chromium.org
+thakis for two line change in base/BUILD.gn Thanks!
base/build.gn lgtm
On 2017/04/19 11:56:09, Primiano Tucci wrote: > On 2017/04/19 09:54:42, hjd wrote: > > I've updated the patch with some changes, ideally we would like > > TracingMemoryObserver to depend on MDM and MDM to not know anything about > > TracingMemoryObserver but at the moment that is causing a lot of complexity: > > * It's awkward to make sure TracingMemoryObserver is initialized in every > > process. > > * TracingMemoryObserver lifecycle is weirdly tied to MDMs lifecycle. > > * We would like to move MDM's code that dumps things into the trace somewhere > > else (probably TracingMemoryObserver) but we would either have to: > > a) Introduce a new listening API (setOnDumpListener) only for this one > case. > > b) Do something like TracingMemoryObserver::GetInstance()->AddToTrace() > from > > MDM which seems ugly. > > > > So to avoid those problems for now (given that we're planning to move this > code > > again soon to live only in the memory-infra service instead of in every > process) > > MDM can just create and own the TracingMemoryObserver this avoids the awkward > > lifecycle and initialization problems. This means that TracingMemoryObserver > > depends on MDM but hopefully we can move TracingMemoryObserver to live in the > > process only soon which would remove this dependency. > > Plan SGTM. ssid, we had some discussion. > The TL;DR is that now clearing the dependency in the direction MDM -> MTO (Mem > Tracing Observer) would be extra indirection layer (a callback) for very little > benefit. We'll do that once we can move the MTO to the service, so that it can > just consume the dump processed from chrome. > Right now my major concern is clearing out all the tracing stuff away from MDM > class. > Until then, a callback vs a direct call isn't going to be a huge difference and > the MDM -> MTO dependency will be one line (in ::FinalizeDumpAndAddToTrace), > until the day where we won't need that line at all (either callback or direct > call) because we can consume. Yes I think it's fine, but feels like we need a TODO saying this will be moved from MDM soon. Maybe we could do the initialization in ProcessMemoryDumpManagerImpl? For now it is MemoryDumpManagerDelegate. About the callback for creating dump, why don't we do it as part of MemoryDumpCallback? The code that is requesting a global dump should know why it is requesting the dump and what to do with it. This CL looks good. But, I think it's not a big change to move the scheduler initialization into observer and tell the scheduler to callback to observer when it requests a global dump and the observer does the right thing by adding to trace. This way if we request a global dump for UMA, the scheduler or whoever is requesting a global dump will just use the summary and not PMD. This doesn't need to wait for any service change really. In any case this CL lgtm but it's nice to fix this also.
On 2017/04/21 19:09:11, ssid wrote: > On 2017/04/19 11:56:09, Primiano Tucci wrote: > > On 2017/04/19 09:54:42, hjd wrote: > > > I've updated the patch with some changes, ideally we would like > > > TracingMemoryObserver to depend on MDM and MDM to not know anything about > > > TracingMemoryObserver but at the moment that is causing a lot of complexity: > > > * It's awkward to make sure TracingMemoryObserver is initialized in every > > > process. > > > * TracingMemoryObserver lifecycle is weirdly tied to MDMs lifecycle. > > > * We would like to move MDM's code that dumps things into the trace > somewhere > > > else (probably TracingMemoryObserver) but we would either have to: > > > a) Introduce a new listening API (setOnDumpListener) only for this one > > case. > > > b) Do something like TracingMemoryObserver::GetInstance()->AddToTrace() > > from > > > MDM which seems ugly. > > > > > > So to avoid those problems for now (given that we're planning to move this > > code > > > again soon to live only in the memory-infra service instead of in every > > process) > > > MDM can just create and own the TracingMemoryObserver this avoids the > awkward > > > lifecycle and initialization problems. This means that TracingMemoryObserver > > > depends on MDM but hopefully we can move TracingMemoryObserver to live in > the > > > process only soon which would remove this dependency. > > > > Plan SGTM. ssid, we had some discussion. > > The TL;DR is that now clearing the dependency in the direction MDM -> MTO (Mem > > Tracing Observer) would be extra indirection layer (a callback) for very > little > > benefit. We'll do that once we can move the MTO to the service, so that it can > > just consume the dump processed from chrome. > > Right now my major concern is clearing out all the tracing stuff away from MDM > > class. > > Until then, a callback vs a direct call isn't going to be a huge difference > and > > the MDM -> MTO dependency will be one line (in ::FinalizeDumpAndAddToTrace), > > until the day where we won't need that line at all (either callback or direct > > call) because we can consume. > > Yes I think it's fine, but feels like we need a TODO saying this will be moved > from MDM soon. > Maybe we could do the initialization in ProcessMemoryDumpManagerImpl? For now it > is MemoryDumpManagerDelegate. > About the callback for creating dump, why don't we do it as part of > MemoryDumpCallback? > The code that is requesting a global dump should know why it is requesting the > dump and what to do with it. This CL looks good. But, I think it's not a big > change to move the scheduler initialization into observer and tell the scheduler > to callback to observer when it requests a global dump and the observer does the > right thing by adding to trace. This way if we request a global dump for UMA, > the scheduler or whoever is requesting a global dump will just use the summary > and not PMD. This doesn't need to wait for any service change really. > > In any case this CL lgtm but it's nice to fix this also. Thanks! I've added the TODO. I'm in favour of moving the scheduler but I'll do it in a follow up so this CL doesn't block anything. As for the code that is requesting a global dump knowing why it is requesting the dump and what to do with it I think that would be ideal (so, if I understand you correctly, if an MDM caller wants the dump added to the trace they are responsible for doing so). This seems doable for the dumps requested by the scheduler (as you suggest they could go through the observer) however I think at the moment there are some problems specifically around the devtools API for requesting a memory dump. The Devtools API wants the dump added to the trace. It currently calls RequestGlobalDump which takes care of the interprocess coordination but if it had to go through the MemoryTracingObverser I think we would have to replicate that coordination. Given that it seems better to wait until we move the MTO to only live in the service.
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, thakis@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2820433005/#ps100001 (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 hjd@chromium.org
Patchset #6 (id:100001) has been deleted
On 2017/04/24 11:55:26, hjd wrote: > however I think at the moment there are some > problems specifically around the devtools API for requesting a memory dump. The > Devtools API wants the dump added to the trace. It currently calls > RequestGlobalDump which takes care of the interprocess coordination but if it > had to go through the MemoryTracingObverser I think we would have to replicate > that coordination. Given that it seems better to wait until we move the MTO to > only live in the service. Yeah ssid, essentially we can't just return the full PMD in the callback. What you suggest would work in the process where the dump is requested, but then the quesiton becomes: in the other processes, how will the client library know what to do (i.e. if append to the trace or not) with the result? We should switch to that model, but only *after* the full servicification, i.e. only after we are in a situation where one process (The one hosting the memory service) has visibility of *all* the PMDs from all processes (in which case the tracing observer will be moved into the service as we'll need only one instance of it, not one per process)
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, thakis@chromium.org, ssid@chromium.org Link to the patchset: https://codereview.chromium.org/2820433005/#ps160001 (title: "add todo")
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_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
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": 160001, "attempt_start_ts": 1493112224700500,
"parent_rev": "d003a55d1bd195ce035486d56fc0b2fd1da0720d", "commit_rev":
"4a4589fc42e386ba9b2ad1c153d17dd8180941f8"}
Message was sent while issue was closed.
Description was changed from ========== memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 ========== to ========== memory-infra: Start disentangling tracing from memory-infra Starts refactoring the tracing specific code from MemoryDumpManager into a new class MemoryTracingObserver. This shouldn't change the behaviour of the MemoryDumpManager. BUG=703184 Review-Url: https://codereview.chromium.org/2820433005 Cr-Commit-Position: refs/heads/master@{#466936} Committed: https://chromium.googlesource.com/chromium/src/+/4a4589fc42e386ba9b2ad1c153d1... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://chromium.googlesource.com/chromium/src/+/4a4589fc42e386ba9b2ad1c153d1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
