[tracing] Prevent memory maps dump for light dump requests

[tracing] Prevent memory maps dump for light dump requests

Memory maps dump is heavy. So, prevent memory maps dump provider from
dumping, when light dump is requested.

BUG=499731
Committed: https://crrev.com/5e758ba92bdf7e576ecd9bab3f07baa1acd5f87e
Cr-Commit-Position: refs/heads/master@{#342139}

[ssid, 2015-08-05 10:29:54]

ssid@chromium.org changed reviewers:
+ petrcermak@chromium.org

[ssid, 2015-08-05 10:29:54]

PTAL

[petrcermak, 2015-08-05 10:31:40]

LGTM with one nit.

Thanks,
Petr

https://codereview.chromium.org/1267573004/diff/40001/base/trace_event/proces...
File base/trace_event/process_memory_maps_dump_provider.cc (right):

https://codereview.chromium.org/1267573004/diff/40001/base/trace_event/proces...
base/trace_event/process_memory_maps_dump_provider.cc:170: // Snapshot of memory
maps is not taken for light dump request.
nit: s/request/requests/

[ssid, 2015-08-05 10:46:22]

ssid@chromium.org changed reviewers:
+ nduca@chromium.org

[ssid, 2015-08-05 10:46:23]

+nduca PTAL, small patch for memory maps dump.

[petrcermak, 2015-08-05 10:54:37]

one more nit: Please remove the fullstop from the patch title and first line.

[petrcermak, 2015-08-05 10:59:54]

s/request/requests/ in the title as well. Sorry for sending each nit separately.

Thanks,
Petr

[picksi, 2015-08-05 11:49:14]

Not a code review or even a nit, but a thought... I wonder if putting
'implementation details' of this check (i.e. the == sign!) at this code-level is
future-proof. Not sure on actual implementation details, but hiding the check
might be worthwhile, so the low detail check would look something like:

if (args.level_od_detail.isLowDetail())

Then we can change how isLowDetail gets determined (we could even pass a
'context' parameter through at some future point if high/low/etc becomes
dynamically-changable at run time).

Simon

[ssid, 2015-08-05 11:53:40]

On 2015/08/05 11:49:14, picksi wrote:
> Not a code review or even a nit, but a thought... I wonder if putting
> 'implementation details' of this check (i.e. the == sign!) at this code-level
is
> future-proof. Not sure on actual implementation details, but hiding the check
> might be worthwhile, so the low detail check would look something like:
> 
> if (args.level_od_detail.isLowDetail())
> 
> Then we can change how isLowDetail gets determined (we could even pass a
> 'context' parameter through at some future point if high/low/etc becomes
> dynamically-changable at run time).
> 
> Simon

Yes, true. But the reason why I added a struct and enum is that this struct is
going to be a part of ipc message when it is sent to the child processes. IPC
messages can handle only basic structs. without constructors. So, a complicated
method cannot be added to this struct. But the logic for deciding can be changed
in memory_dump_manager while sending the dump request.

[nduca, 2015-08-06 15:05:27]

lgtm

[ssid, 2015-08-06 17:20:06]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-06 17:20:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
petrcermak@chromium.org, nduca@chromium.org
Link to the patchset: https://codereview.chromium.org/1267573004/#ps80001
(title: "Typo.")

[commit-bot: I haz the power, 2015-08-06 17:21:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1267573004/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1267573004/80001

[commit-bot: I haz the power, 2015-08-06 17:27:16]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-08-06 17:27:47]

Patchset 5 (id:??) landed as
https://crrev.com/5e758ba92bdf7e576ecd9bab3f07baa1acd5f87e
Cr-Commit-Position: refs/heads/master@{#342139}