memory-infra: Add SUMMARY_ONLY MemoryDumpType

memory-infra: Add SUMMARY_ONLY MemoryDumpType

Add a new MemoryDumpType: SUMMARY_ONLY which allows RequestGlobalDump
callers to specify the dump should not be added to the trace.

BUG=703184
Review-Url: https://codereview.chromium.org/2838913002
Cr-Commit-Position: refs/heads/master@{#467327}
Committed: https://chromium.googlesource.com/chromium/src/+/09b8b3a25d763b86f0f935edae3a0442188872e6

[hjd, 2017-04-25 15:31:38]

hjd@chromium.org changed reviewers:
+ fmeawad@chromium.org, primiano@chromium.org, ssid@chromium.org

[hjd, 2017-04-25 15:31:38]

ptal, thanks!

[fmeawad, 2017-04-25 15:37:18]

https://codereview.chromium.org/2838913002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2838913002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_request_args.h:31: SUMMARY_ONLY,          //
Calculate just the summary & don't add to the trace.
This CL leaves the code insconsistent with memory_instrumentation.mojom

[fmeawad, 2017-04-25 16:01:01]

Can you also add some testing, maybe in memory_tracing_browsertest.cc

[Primiano Tucci (use gerrit), 2017-04-25 16:09:33]

On 2017/04/25 16:01:01, fmeawad wrote:
> Can you also add some testing, maybe in memory_tracing_browsertest.cc
+1, although memory_dump_manager_unittest.cc should be sufficient. see
DumpOnBehalfOfOtherProcess there, there is already harness to inspect the
contents of the trace.

What I'd like to see is a test which starts tracing, does a conventional (Say
EXPLICITLY_TRIGGERED) dump with level=BACKGROUND and another one with
SUMMARY_ONLY level=BACKGROUND, and check that:
both succeed, but only one is in the trace.


Also I think you need to change the dump_successful logic in
memory_dump_manager.
as-is I think that these dumps will always return false in the callback because
AddDumpToTraceIfEnabled returns false (which is good), and that will cause
dump_successful = false in FinalizeDumpAndAddToTrace

[hjd, 2017-04-25 16:21:08]

On 2017/04/25 16:09:33, Primiano Tucci wrote:
> On 2017/04/25 16:01:01, fmeawad wrote:
> > Can you also add some testing, maybe in memory_tracing_browsertest.cc
> +1, although memory_dump_manager_unittest.cc should be sufficient. see
> DumpOnBehalfOfOtherProcess there, there is already harness to inspect the
> contents of the trace.

Perhaps time for base/trace_event/memory_tracing_observer_unittests.cc? ;P

> What I'd like to see is a test which starts tracing, does a conventional (Say
> EXPLICITLY_TRIGGERED) dump with level=BACKGROUND and another one with
> SUMMARY_ONLY level=BACKGROUND, and check that:
> both succeed, but only one is in the trace.

Okay I will add that test.
 
> Also I think you need to change the dump_successful logic in
> memory_dump_manager.
> as-is I think that these dumps will always return false in the callback
because
> AddDumpToTraceIfEnabled returns false (which is good), and that will cause
> dump_successful = false in FinalizeDumpAndAddToTrace

That's on purpose right? See your comment on
https://codereview.chromium.org/2820433005/#msg12 Or am I confused about
something?

[hjd, 2017-04-25 17:10:09]

On 2017/04/25 16:21:08, hjd wrote:
> On 2017/04/25 16:09:33, Primiano Tucci wrote:
> > On 2017/04/25 16:01:01, fmeawad wrote:
> > > Can you also add some testing, maybe in memory_tracing_browsertest.cc
> > +1, although memory_dump_manager_unittest.cc should be sufficient. see
> > DumpOnBehalfOfOtherProcess there, there is already harness to inspect the
> > contents of the trace.
> 
> Perhaps time for base/trace_event/memory_tracing_observer_unittests.cc? ;P
> 
> > What I'd like to see is a test which starts tracing, does a conventional
(Say
> > EXPLICITLY_TRIGGERED) dump with level=BACKGROUND and another one with
> > SUMMARY_ONLY level=BACKGROUND, and check that:
> > both succeed, but only one is in the trace.
> 
> Okay I will add that test.
>  
> > Also I think you need to change the dump_successful logic in
> > memory_dump_manager.
> > as-is I think that these dumps will always return false in the callback
> because
> > AddDumpToTraceIfEnabled returns false (which is good), and that will cause
> > dump_successful = false in FinalizeDumpAndAddToTrace
> 
> That's on purpose right? See your comment on
> https://codereview.chromium.org/2820433005/#msg12 Or am I confused about
> something?

^ We spoke offline and I've updated :)

[Primiano Tucci (use gerrit), 2017-04-25 18:02:21]

primiano@chromium.org changed reviewers:
+ rockot@chromium.org

[Primiano Tucci (use gerrit), 2017-04-25 18:02:22]

LGTM with some final comments.
+rockot for adding 1 extra enum value to memory_instrumentation.mojom

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:743: // Some dumps are requested just to
cacluate the summary. These shouldn't
s/some dumps/SUMMARY_ONLY/
s/just to cacluate the summary/just to return the summarized result in the
MemoryDumpReqCalblback/

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1126:
MemoryDumpLevelOfDetail::DETAILED);
i'd s/DETAILED/BACKGROUND/ just in case in future we add some check that
SUMMARY_ONLY must be background and remove the frustration in that case of
having a red CQ and having to update this test :)
Shouldn't really change the semantics of this test.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1132:
trace_event::TraceResultBuffer buffer;
maybe you can factor this and the one in DumpOnBehalfOfOtherProcess into a
function in the anonymous namespace, something like

std::unique_ptr<trace_analyzer::TraceAnalyzer> GetDeserializedTrace();

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1150: ASSERT_EQ(1u,
trace_analyzer::CountMatches(
I'd replace this with 
ASSERT_TRUE(trace_analyzer::CountMatches(trace_analyzer::EventNameIs(MemoryDumpTypeToString(MemoryDumpType::EXPLICITLY_TRIGGERED))

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:75: CHECK(req_args->dump_type !=
MemoryDumpType::SUMMARY_ONLY);
CHECK_NE(MemoryDumpType::SUMMARY_ONLY, req_args->dump_type)

so that if this fails, it spits out the actual dump_type on the logs

[ssid, 2017-04-25 18:09:51]

lgtm

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:32: LAST = SUMMARY_ONLY    // For
IPC macros.
Nit: can you remove the comment that says IPC macros. It is no longer true.

[Ken Rockot(use gerrit already), 2017-04-25 18:10:24]

lgtm

[hjd, 2017-04-26 11:10:25]

Patchset #6 (id:100001) has been deleted

[hjd, 2017-04-26 11:11:58]

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:743: // Some dumps are requested just to
cacluate the summary. These shouldn't
On 2017/04/25 18:02:22, Primiano Tucci wrote:
> s/some dumps/SUMMARY_ONLY/
> s/just to cacluate the summary/just to return the summarized result in the
> MemoryDumpReqCalblback/

Done.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_manager_unittest.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1126:
MemoryDumpLevelOfDetail::DETAILED);
On 2017/04/25 18:02:22, Primiano Tucci wrote:
> i'd s/DETAILED/BACKGROUND/ just in case in future we add some check that
> SUMMARY_ONLY must be background and remove the frustration in that case of
> having a red CQ and having to update this test :)
> Shouldn't really change the semantics of this test.

Done.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1132:
trace_event::TraceResultBuffer buffer;
On 2017/04/25 18:02:22, Primiano Tucci wrote:
> maybe you can factor this and the one in DumpOnBehalfOfOtherProcess into a
> function in the anonymous namespace, something like
> 
> std::unique_ptr<trace_analyzer::TraceAnalyzer> GetDeserializedTrace();

Done.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1150: ASSERT_EQ(1u,
trace_analyzer::CountMatches(
On 2017/04/25 18:02:22, Primiano Tucci wrote:
> I'd replace this with 
>
ASSERT_TRUE(trace_analyzer::CountMatches(trace_analyzer::EventNameIs(MemoryDumpTypeToString(MemoryDumpType::EXPLICITLY_TRIGGERED))

Done.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_dump_request_args.h (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_request_args.h:32: LAST = SUMMARY_ONLY    // For
IPC macros.
On 2017/04/25 18:09:51, ssid wrote:
> Nit: can you remove the comment that says IPC macros. It is no longer true.

Done.

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
File base/trace_event/memory_tracing_observer.cc (right):

https://codereview.chromium.org/2838913002/diff/60001/base/trace_event/memory...
base/trace_event/memory_tracing_observer.cc:75: CHECK(req_args->dump_type !=
MemoryDumpType::SUMMARY_ONLY);
On 2017/04/25 18:02:22, Primiano Tucci wrote:
> CHECK_NE(MemoryDumpType::SUMMARY_ONLY, req_args->dump_type)
> 
> so that if this fails, it spits out the actual dump_type on the logs

Done.

[hjd, 2017-04-26 11:19:33]

hjd@chromium.org changed reviewers:
+ mkwst@chromium.org

[hjd, 2017-04-26 11:19:33]

+mkwst ptal at 2 files under services/resource_coordinator/public/ Thanks!

[Mike West, 2017-04-26 11:34:28]

LGTM.

[hjd, 2017-04-26 13:18:54]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-04-26 13:18:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, rockot@chromium.org, ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2838913002/#ps80001
(title: "update for comments")

[commit-bot: I haz the power, 2017-04-26 13:19:14]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 13:28:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-26 13:28:08]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[hjd, 2017-04-26 13:39:20]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-04-26 13:39:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org, rockot@chromium.org, mkwst@chromium.org,
ssid@chromium.org
Link to the patchset: https://codereview.chromium.org/2838913002/#ps140001
(title: "fix for rebase")

[commit-bot: I haz the power, 2017-04-26 13:39:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 15:18:28]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493213960386580,
"parent_rev": "02ebdb393ac0b4c2651881ce8b6a08312c653474", "commit_rev":
"09b8b3a25d763b86f0f935edae3a0442188872e6"}

[commit-bot: I haz the power, 2017-04-26 15:18:39]

Description was changed from

==========
memory-infra: Add SUMMARY_ONLY MemoryDumpType

Add a new MemoryDumpType: SUMMARY_ONLY which allows RequestGlobalDump
callers to specify the dump should not be added to the trace.

BUG=703184
==========

to

==========
memory-infra: Add SUMMARY_ONLY MemoryDumpType

Add a new MemoryDumpType: SUMMARY_ONLY which allows RequestGlobalDump
callers to specify the dump should not be added to the trace.

BUG=703184

Review-Url: https://codereview.chromium.org/2838913002
Cr-Commit-Position: refs/heads/master@{#467327}
Committed:
https://chromium.googlesource.com/chromium/src/+/09b8b3a25d763b86f0f935edae3a...
==========

[commit-bot: I haz the power, 2017-04-26 15:18:41]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/09b8b3a25d763b86f0f935edae3a...