memory-infra: Don't invoke all dump providers in SUMMARY_ONLY mode

memory-infra: Don't invoke all dump providers in SUMMARY_ONLY mode

The SUMMARY_ONLY dump type only uses the numbers that are in the summary struct.
All the other numbers are ignored currently. So add a separate whietlist for
this mode.

BUG=720169
Review-Url: https://codereview.chromium.org/2876543002
Cr-Commit-Position: refs/heads/master@{#471743}
Committed: https://chromium.googlesource.com/chromium/src/+/25b5cfb04820b353cc759e635b321a2cecf102bf

[hjd, 2017-05-10 13:48:08]

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

[hjd, 2017-05-10 13:48:08]

ptal, thanks!

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:316: bool
whitelisted_for_background_mode = IsMemoryDumpProviderWhitelisted(name);
Shall I rename all the methods/variables named things like
IsMemoryDumpProviderWhitelisted to IsMemoryDumpProviderWhitelistedForBackground
for consistency?

[Primiano Tucci (use gerrit), 2017-05-10 16:18:59]

awesome! lgtm with some minor comments

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:316: bool
whitelisted_for_background_mode = IsMemoryDumpProviderWhitelisted(name);
On 2017/05/10 13:48:08, hjd wrote:
> Shall I rename all the methods/variables named things like
> IsMemoryDumpProviderWhitelisted to
IsMemoryDumpProviderWhitelistedForBackground
> for consistency?

up to you. I think is worth a comment, explaining what they are for and hte fact
that they are subsets of each other.

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

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1086:
TEST_F(MemoryDumpManagerTest, TestSummaryWhitelisting) {
s/TestSummaryWhitelisting/SummaryOnlyWhitelisting/
there are enough "Test" here:)

Also would be nice if this had one MDP that is whitelisted for background but
NOT summary, and another one that is whitelisted for both, adn then check that
only one gets invoked.

also maybe add a comment explaining that summary-only is a subset of the general
background whitelist. Ourselves from the future will thank us for this comment
:)

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:43: // The names of dump
providers whitelisted for summary tracing.
/summary tracing/SUMMARY_ONLY dump type modes/

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:45: "Malloc",
"PartitionAlloc", "ProcessMemoryMetrics", "V8Isolate",
is this git cl format-ed? (if so I have no objections :) )

[ssid, 2017-05-10 17:43:09]

lgtm and please git cl format.

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:43: // The names of dump
providers whitelisted for summary tracing.
Maybe a comment here similar to the description of the cl that says why we have
another whitelist.

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_peak_detector_unittest.cc (right):

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_peak_detector_unittest.cc:180: mdp.get(), "Mock MDP",
nullptr, opt, false, false));
nit: can you make it 
false /* whitelisted_for_background_mode */,
false /* whitelisted_for_summary_mode */

[hjd, 2017-05-15 12:02:27]

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
File base/trace_event/memory_dump_manager.cc (right):

https://codereview.chromium.org/2876543002/diff/1/base/trace_event/memory_dum...
base/trace_event/memory_dump_manager.cc:316: bool
whitelisted_for_background_mode = IsMemoryDumpProviderWhitelisted(name);
On 2017/05/10 16:18:58, Primiano Tucci wrote:
> On 2017/05/10 13:48:08, hjd wrote:
> > Shall I rename all the methods/variables named things like
> > IsMemoryDumpProviderWhitelisted to
> IsMemoryDumpProviderWhitelistedForBackground
> > for consistency?
> 
> up to you. I think is worth a comment, explaining what they are for and hte
fact
> that they are subsets of each other.

Done.

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

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_dump_manager_unittest.cc:1086:
TEST_F(MemoryDumpManagerTest, TestSummaryWhitelisting) {
On 2017/05/10 16:18:58, Primiano Tucci wrote:
> s/TestSummaryWhitelisting/SummaryOnlyWhitelisting/
> there are enough "Test" here:)
> 
> Also would be nice if this had one MDP that is whitelisted for background but
> NOT summary, and another one that is whitelisted for both, adn then check that
> only one gets invoked.
> 
> also maybe add a comment explaining that summary-only is a subset of the
general
> background whitelist. Ourselves from the future will thank us for this comment
> :)

Done.

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_infra_background_whitelist.cc (right):

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_infra_background_whitelist.cc:45: "Malloc",
"PartitionAlloc", "ProcessMemoryMetrics", "V8Isolate",
On 2017/05/10 16:18:58, Primiano Tucci wrote:
> is this git cl format-ed? (if so I have no objections :) )

yep :)

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
File base/trace_event/memory_peak_detector_unittest.cc (right):

https://codereview.chromium.org/2876543002/diff/20001/base/trace_event/memory...
base/trace_event/memory_peak_detector_unittest.cc:180: mdp.get(), "Mock MDP",
nullptr, opt, false, false));
On 2017/05/10 17:43:08, ssid wrote:
> nit: can you make it 
> false /* whitelisted_for_background_mode */,
> false /* whitelisted_for_summary_mode */

Done.

[hjd, 2017-05-15 12:02:57]

The CQ bit was checked by hjd@chromium.org

[hjd, 2017-05-15 12:02:58]

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

[commit-bot: I haz the power, 2017-05-15 12:03:15]

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-05-15 13:11:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494849777896710,
"parent_rev": "d030d8d29e158494567972bcc1321d390b81329a", "commit_rev":
"25b5cfb04820b353cc759e635b321a2cecf102bf"}

[commit-bot: I haz the power, 2017-05-15 13:11:34]

Description was changed from

==========
memory-infra: Don't invoke all dump providers in SUMMARY_ONLY mode

The SUMMARY_ONLY dump type only uses the numbers that are in the summary struct.
All the other numbers are ignored currently. So add a separate whietlist for
this mode.

BUG=720169
==========

to

==========
memory-infra: Don't invoke all dump providers in SUMMARY_ONLY mode

The SUMMARY_ONLY dump type only uses the numbers that are in the summary struct.
All the other numbers are ignored currently. So add a separate whietlist for
this mode.

BUG=720169

Review-Url: https://codereview.chromium.org/2876543002
Cr-Commit-Position: refs/heads/master@{#471743}
Committed:
https://chromium.googlesource.com/chromium/src/+/25b5cfb04820b353cc759e635b32...
==========

[commit-bot: I haz the power, 2017-05-15 13:11:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/25b5cfb04820b353cc759e635b32...