[tracing] Introduce MemoryDumpArgs to enable light and heavy dumps

[tracing] Introduce MemoryDumpArgs to enable light and heavy dumps

The memory dump providers dump a lot of details at each dump which
causes the trace size to be huge. So, this CL introduces MemoryDumpArgs
that will be passed to each DumpProvider as an argument of OnMemoryDump.
The args specify a high or low level of detail. This provides a means to
request a light or heavy dump from the providers when necessary.

TBR=reed@google.com, jam@chromium.org
BUG=499731
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/90694aeec592b22686ad54bc1a225c2b44cc468e
Cr-Commit-Position: refs/heads/master@{#342103}

[ssid, 2015-07-31 12:28:36]

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

[ssid, 2015-07-31 12:28:36]

PTAL, thanks

[petrcermak, 2015-07-31 13:23:01]

Looks good overall. Here are a couple of comments:

Patch description nits:

  * either s/lot of/a lot of/, or s/lot of/lots of/
  * s/when OnMemoryDump/as an argument of OnMemoryDump/
  * s/The args has high/The args specify a high/
  * s/helps to request/provides a means to request/

Thanks,
Petr

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:31: bool
MallocDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
Is there a reason why you put the args first as the first argument?

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_allocator_dump_unittest.cc:24: bool OnMemoryDump(const
MemoryDumpArgs&, ProcessMemoryDump* pmd) override {
nit: missing argument name (I know it's not strictly necessary, but we seems to
put it everywhere)

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:318: const MemoryDumpArgs
dump_args(MemoryDumpArgs::LEVEL_OF_DETAIL_HIGH);
No need to build the object outside the if-statement.

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_provider.h:16: struct MemoryDumpArgs {
Could you please explain the relationship between MemoryDumpArgs (added in this
patch) and MemoryDumpRequestArgs (existing class:
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m...
)? I'm wondering whether they should be the same thing.

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_provider.h:21: LevelOfDetail level_of_detail;
const?

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/trace_...
base/trace_event/trace_log.cc:473: // TODO(ssid): Use MemoryDumpArgs to create
light dumps when requested
nit: Not sure whether we will have light tracing dumps because they would cause
less precise results overall.

https://codereview.chromium.org/1262333005/diff/60001/gin/v8_isolate_memory_d...
File gin/v8_isolate_memory_dump_provider.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/gin/v8_isolate_memory_d...
gin/v8_isolate_memory_dump_provider.cc:33: // TODO(ssid): Use MemoryDumpArgs to
create light dumps when requested
Why do you put this comment into some dump providers (e.g. tracing, v8) and not
others (e.g. malloc, winheap)?

[ssid, 2015-07-31 15:14:33]

replies inline.

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:31: bool
MallocDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
On 2015/07/31 13:23:01, petrcermak wrote:
> Is there a reason why you put the args first as the first argument?

Yes according to code style, the input args should be placed before the output
ones.

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_allocator_dump_unittest.cc:24: bool OnMemoryDump(const
MemoryDumpArgs&, ProcessMemoryDump* pmd) override {
On 2015/07/31 13:23:01, petrcermak wrote:
> nit: missing argument name (I know it's not strictly necessary, but we seems
to
> put it everywhere)

Done.

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:318: const MemoryDumpArgs
dump_args(MemoryDumpArgs::LEVEL_OF_DETAIL_HIGH);
On 2015/07/31 13:23:01, petrcermak wrote:
> No need to build the object outside the if-statement.

Done.

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_provider.h:16: struct MemoryDumpArgs {
On 2015/07/31 13:23:01, petrcermak wrote:
> Could you please explain the relationship between MemoryDumpArgs (added in
this
> patch) and MemoryDumpRequestArgs (existing class:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m...
> )? I'm wondering whether they should be the same thing.

Hm, the MemoryDumpRequestArgs is used to request a global dump, contains what
type of dump is to be taken to the manager. This args is to tell the dump
provider what kind of dump to take. I can't merge both since the providers need
not know what triggered the dump or the global dump guid.

The PERIODIC_INTERVAL_WITH_MMAPS will be removed and maybe MemoryDumpArgs should
be a part of MemoryDumpRequest args for requesting.

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_provider.h:21: LevelOfDetail level_of_detail;
On 2015/07/31 13:23:01, petrcermak wrote:
> const?

Done.

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/trace_...
File base/trace_event/trace_log.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/trace_...
base/trace_event/trace_log.cc:473: // TODO(ssid): Use MemoryDumpArgs to create
light dumps when requested
On 2015/07/31 13:23:01, petrcermak wrote:
> nit: Not sure whether we will have light tracing dumps because they would
cause
> less precise results overall.

That is true, the data collected is going to be complete for this, but then not
all the details about the tracing needs to be dumps. Just dumping the totals
would be sufficient here.

https://codereview.chromium.org/1262333005/diff/60001/gin/v8_isolate_memory_d...
File gin/v8_isolate_memory_dump_provider.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/gin/v8_isolate_memory_d...
gin/v8_isolate_memory_dump_provider.cc:33: // TODO(ssid): Use MemoryDumpArgs to
create light dumps when requested
On 2015/07/31 13:23:01, petrcermak wrote:
> Why do you put this comment into some dump providers (e.g. tracing, v8) and
not
> others (e.g. malloc, winheap)?

I did not put in other dump providers since they do not need a difference
between light and heavy, since it is already light. I did not want to add a
comment saying they don't need light dumps since, if we add more args in future
this would mean that we need to keep updating the comment.

[petrcermak, 2015-08-03 10:12:36]

LGTM.

Thanks,
Petr

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
File base/trace_event/malloc_dump_provider.cc (right):

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/malloc...
base/trace_event/malloc_dump_provider.cc:31: bool
MallocDumpProvider::OnMemoryDump(const MemoryDumpArgs& args,
On 2015/07/31 15:14:32, ssid wrote:
> On 2015/07/31 13:23:01, petrcermak wrote:
> > Is there a reason why you put the args first as the first argument?
> 
> Yes according to code style, the input args should be placed before the output
> ones.

Acknowledged.

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

https://codereview.chromium.org/1262333005/diff/60001/base/trace_event/memory...
base/trace_event/memory_dump_provider.h:16: struct MemoryDumpArgs {
On 2015/07/31 15:14:33, ssid wrote:
> On 2015/07/31 13:23:01, petrcermak wrote:
> > Could you please explain the relationship between MemoryDumpArgs (added in
> this
> > patch) and MemoryDumpRequestArgs (existing class:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m...
> > )? I'm wondering whether they should be the same thing.
> 
> Hm, the MemoryDumpRequestArgs is used to request a global dump, contains what
> type of dump is to be taken to the manager. This args is to tell the dump
> provider what kind of dump to take. I can't merge both since the providers
need
> not know what triggered the dump or the global dump guid.
> 
> The PERIODIC_INTERVAL_WITH_MMAPS will be removed and maybe MemoryDumpArgs
should
> be a part of MemoryDumpRequest args for requesting.

OK, this makes sense. Thanks for the explanation.

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

https://codereview.chromium.org/1262333005/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:318: if (!skip_dump)
nit: I think this would be more readable with braces (given that it is a 3-line
statement.

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

https://codereview.chromium.org/1262333005/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_maps_dump_provider.cc:166: // Called at trace
dump point time. Creates a snapshot the memory maps for the
When you're already touching this file, could you please make the following
change in this comment:

s/snapshot the memory maps/snapshot of the memory maps/

[ssid, 2015-08-03 10:34:02]

Thanks.

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

https://codereview.chromium.org/1262333005/diff/80001/base/trace_event/memory...
base/trace_event/memory_dump_manager.cc:318: if (!skip_dump)
On 2015/08/03 10:12:35, petrcermak wrote:
> nit: I think this would be more readable with braces (given that it is a
3-line
> statement.

Done.

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

https://codereview.chromium.org/1262333005/diff/80001/base/trace_event/proces...
base/trace_event/process_memory_maps_dump_provider.cc:166: // Called at trace
dump point time. Creates a snapshot the memory maps for the
On 2015/08/03 10:12:35, petrcermak wrote:
> When you're already touching this file, could you please make the following
> change in this comment:
> 
> s/snapshot the memory maps/snapshot of the memory maps/

Done.

[ssid, 2015-08-03 10:37:21]

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

[ssid, 2015-08-03 10:37:22]

This CL adds new struct to the memory_dump_provider.h and rest of the impl files
have just mechanical changes. PTAL, thanks.

[ssid, 2015-08-03 18:26:24]

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

[ssid, 2015-08-03 18:26:24]

-danakj
+nduca
This CL adds new struct to the memory_dump_provider.h and rest of the impl files
have just mechanical changes. PTAL, thanks.

[nduca, 2015-08-04 15:50:24]

lgtm

[ssid, 2015-08-04 16:57:13]

ssid@chromium.org changed reviewers:
+ dalecurtis@chromium.org, jochen@chromium.org, reed@google.com

[ssid, 2015-08-04 16:57:14]

These are mostly mechanical changes, because of change in DumpProvider API.
PTAL.

+jochen for changes in content

+dalecurtis for changes in media

+reed for changes in skia

[jochen (gone - plz use gerrit), 2015-08-05 10:20:41]

content lgtm

[DaleCurtis, 2015-08-06 03:27:31]

media/ lgtm

[ssid, 2015-08-06 12:56:50]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-06 12:56:50]

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

[commit-bot: I haz the power, 2015-08-06 12:57:07]

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

[ssid, 2015-08-06 12:57:28]

On 2015/08/06 03:27:31, DaleCurtis wrote:
> media/ lgtm

TBRing jam@ and reed@ for mechanical in the files for API change in base/.

[commit-bot: I haz the power, 2015-08-06 13:01:36]

Committed patchset #14 (id:260001)

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

Patchset 14 (id:??) landed as
https://crrev.com/90694aeec592b22686ad54bc1a225c2b44cc468e
Cr-Commit-Position: refs/heads/master@{#342103}

[ssid, 2016-09-07 03:44:51]

Patchset #2 (id:20001) has been deleted

[ssid, 2016-09-07 03:44:55]

Patchset #1 (id:1) has been deleted

[ssid, 2016-09-07 03:45:09]

Patchset #1 (id:40001) has been deleted

[ssid, 2016-09-07 03:45:21]

Patchset #4 (id:120001) has been deleted

[ssid, 2016-09-07 03:45:30]

Patchset #4 (id:140001) has been deleted

[ssid, 2016-09-07 03:45:39]

Patchset #5 (id:180001) has been deleted

[ssid, 2016-09-07 03:45:47]

Patchset #4 (id:160001) has been deleted

[ssid, 2016-09-07 03:45:58]

Patchset #5 (id:220001) has been deleted

[ssid, 2016-09-07 03:46:10]

Patchset #4 (id:200001) has been deleted

[ssid, 2016-09-07 03:46:25]

Patchset #4 (id:240001) has been deleted