[tracing] Improve the memory maps dumper generation format.

[tracing] Improve the memory maps dumper generation format.

This CL reworks the trace generation format for the memory maps
dumper according to latest discussion in the design doc
"Memory Dumping: Mmap and Smap data dumping details"
(http://goo.gl/e6WRnQ).
Furthermore this CL changes the output format of the total dumper to
dump a hex string rather than a float, in order to be consistent with
the mmaps dumper.

See http://pastebin.com/EHdNDMXN for an example of the generated JSON.

BUG=460884
Committed: https://crrev.com/165d819227077a11933773b7d2dfd3c538b24a5e
Cr-Commit-Position: refs/heads/master@{#321570}

[Primiano Tucci (use gerrit), 2015-02-26 17:30:57]

Patchset #1 (id:1) has been deleted

[Primiano Tucci (use gerrit), 2015-02-26 17:32:08]

primiano@chromium.org changed reviewers:
+ dsinclair@chromium.org, nduca@chromium.org

[dsinclair, 2015-02-26 18:00:01]

Not really a fan of this dump format over the old one. Do you have an idea of
the size comparison between this format and the old with shortened names?

https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
File base/trace_event/process_memory_maps.cc (right):

https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
base/trace_event/process_memory_maps.cc:12: const int kDumpVersion = 1;  // For
tracing backwards compatibility,
Named keys would get rid of the need for this versioning ...

[Primiano Tucci (use gerrit), 2015-03-01 12:10:27]

On 2015/02/26 18:00:01, dsinclair wrote:
> Not really a fan of this dump format over the old one. Do you have an idea of
> the size comparison between this format and the old with shortened names?
> 
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> File base/trace_event/process_memory_maps.cc (right):
> 
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> base/trace_event/process_memory_maps.cc:12: const int kDumpVersion = 1;  //
For
> tracing backwards compatibility,
> Named keys would get rid of the need for this versioning ...

I did some rough math.
In essence we are talking of

[This CL]
  (1) "vm_regions_headers":[...stuff...] (150 bytes) +
  (N) x [x1,x2,x3,x4,x5,x6,x7]

versus

[Shortened names]
  (N) x {"sa":x1,"sz":x2,"pf":x3,"mf":x4,"mo":x5,"bs":{"a":x6,"r":x7}}

See the doc [1] for a more detailed explanation of the two variants.

The verbosity of the shortened names format has an extra cost of ~40 chars per
mmap entry.

The order of magnitude here is around 500-1000 mmaps per dump per (chrome)
process.
Therefore, The difference between the compressed keys vs array translates into
20k / 40k of overhead per dump per process.


--------------------------------------------------
Let's take a look to actual numbers (full traces)
--------------------------------------------------
Actual full numbers (i.e. structure overhead + actual data).

For a trace consisting of 10 mmaps dump points, the sizes of the two variants
are:
503 kB (headers+array) vs
959 kB (compact dict).

If I gzip the entire trace file, the sizes become
86 kB (headers+array) vs
96 kB (compact dict).

If I gzip + b64encode the individual dump lines (i.e. to emulate the case where
the trace is still plaintext JSON and the "args:" argument of each dump is
compressed and b64 encoded) it becomes
117 kB (headers+array) vs
132 kB (compact dict).


In the light of these numbers, I'd tend to agree with you. it sounds to me we
want to go for the compact dict and eventually look at some form of compression
in the future.
Or eventually, in the future, do something more fancy like:
most (say 80%) of the mmaps are unchanged between each snapshot. At trace
generation time, we could therefore delta-encode the mmaps, s.t. each dump
points become essentially a set of: (+) new mmaps 
(-) deleted mmaps, (M) changes mmaps w.r.t the previous dump point.
(Not sure, though, whether this is feasible and what is the impact in the
tracing architecture)


[1]
https://docs.google.com/a/chromium.org/document/d/1mN6K2OOBHsWTl3IkFsTdoC925C...

[dsinclair, 2015-03-02 14:35:46]

On 2015/03/01 at 12:10:27, primiano wrote:
> On 2015/02/26 18:00:01, dsinclair wrote:
> > Not really a fan of this dump format over the old one. Do you have an idea
of
> > the size comparison between this format and the old with shortened names?
> > 
> >
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > File base/trace_event/process_memory_maps.cc (right):
> > 
> >
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > base/trace_event/process_memory_maps.cc:12: const int kDumpVersion = 1;  //
For
> > tracing backwards compatibility,
> > Named keys would get rid of the need for this versioning ...
> 
> I did some rough math.
> In essence we are talking of
> 
> [This CL]
>   (1) "vm_regions_headers":[...stuff...] (150 bytes) +
>   (N) x [x1,x2,x3,x4,x5,x6,x7]
> 
> versus
> 
> [Shortened names]
>   (N) x {"sa":x1,"sz":x2,"pf":x3,"mf":x4,"mo":x5,"bs":{"a":x6,"r":x7}}
> 
> See the doc [1] for a more detailed explanation of the two variants.
> 
> The verbosity of the shortened names format has an extra cost of ~40 chars per
mmap entry.
> 
> The order of magnitude here is around 500-1000 mmaps per dump per (chrome)
process.
> Therefore, The difference between the compressed keys vs array translates into
20k / 40k of overhead per dump per process.
> 
> 
> --------------------------------------------------
> Let's take a look to actual numbers (full traces)
> --------------------------------------------------
> Actual full numbers (i.e. structure overhead + actual data).
> 
> For a trace consisting of 10 mmaps dump points, the sizes of the two variants
are:
> 503 kB (headers+array) vs
> 959 kB (compact dict).
> 
> If I gzip the entire trace file, the sizes become
> 86 kB (headers+array) vs
> 96 kB (compact dict).
> 
> If I gzip + b64encode the individual dump lines (i.e. to emulate the case
where the trace is still plaintext JSON and the "args:" argument of each dump is
compressed and b64 encoded) it becomes
> 117 kB (headers+array) vs
> 132 kB (compact dict).
> 
> 
> In the light of these numbers, I'd tend to agree with you. it sounds to me we
want to go for the compact dict and eventually look at some form of compression
in the future.
> Or eventually, in the future, do something more fancy like:
> most (say 80%) of the mmaps are unchanged between each snapshot. At trace
generation time, we could therefore delta-encode the mmaps, s.t. each dump
points become essentially a set of: (+) new mmaps 
> (-) deleted mmaps, (M) changes mmaps w.r.t the previous dump point.
> (Not sure, though, whether this is feasible and what is the impact in the
tracing architecture)
> 
> 
> [1]
https://docs.google.com/a/chromium.org/document/d/1mN6K2OOBHsWTl3IkFsTdoC925C...

 This is looking a lot better, for completeness, can you pull the same numbers
with the non-compact dictionary (i.e. the current code)?

[Primiano Tucci (use gerrit), 2015-03-02 19:09:00]

On 2015/03/02 14:35:46, dsinclair wrote:
> On 2015/03/01 at 12:10:27, primiano wrote:
> > On 2015/02/26 18:00:01, dsinclair wrote:
> > > Not really a fan of this dump format over the old one. Do you have an idea
> of
> > > the size comparison between this format and the old with shortened names?
> > > 
> > >
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > > File base/trace_event/process_memory_maps.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > > base/trace_event/process_memory_maps.cc:12: const int kDumpVersion = 1; 
//
> For
> > > tracing backwards compatibility,
> > > Named keys would get rid of the need for this versioning ...
> > 
> > I did some rough math.
> > In essence we are talking of
> > 
> > [This CL]
> >   (1) "vm_regions_headers":[...stuff...] (150 bytes) +
> >   (N) x [x1,x2,x3,x4,x5,x6,x7]
> > 
> > versus
> > 
> > [Shortened names]
> >   (N) x {"sa":x1,"sz":x2,"pf":x3,"mf":x4,"mo":x5,"bs":{"a":x6,"r":x7}}
> > 
> > See the doc [1] for a more detailed explanation of the two variants.
> > 
> > The verbosity of the shortened names format has an extra cost of ~40 chars
per
> mmap entry.
> > 
> > The order of magnitude here is around 500-1000 mmaps per dump per (chrome)
> process.
> > Therefore, The difference between the compressed keys vs array translates
into
> 20k / 40k of overhead per dump per process.
> > 
> > 
> > --------------------------------------------------
> > Let's take a look to actual numbers (full traces)
> > --------------------------------------------------
> > Actual full numbers (i.e. structure overhead + actual data).
> > 
> > For a trace consisting of 10 mmaps dump points, the sizes of the two
variants
> are:
> > 503 kB (headers+array) vs
> > 959 kB (compact dict).
> > 
> > If I gzip the entire trace file, the sizes become
> > 86 kB (headers+array) vs
> > 96 kB (compact dict).
> > 
> > If I gzip + b64encode the individual dump lines (i.e. to emulate the case
> where the trace is still plaintext JSON and the "args:" argument of each dump
is
> compressed and b64 encoded) it becomes
> > 117 kB (headers+array) vs
> > 132 kB (compact dict).
> > 
> > 
> > In the light of these numbers, I'd tend to agree with you. it sounds to me
we
> want to go for the compact dict and eventually look at some form of
compression
> in the future.
> > Or eventually, in the future, do something more fancy like:
> > most (say 80%) of the mmaps are unchanged between each snapshot. At trace
> generation time, we could therefore delta-encode the mmaps, s.t. each dump
> points become essentially a set of: (+) new mmaps 
> > (-) deleted mmaps, (M) changes mmaps w.r.t the previous dump point.
> > (Not sure, though, whether this is feasible and what is the impact in the
> tracing architecture)
> > 
> > 
> > [1]
>
https://docs.google.com/a/chromium.org/document/d/1mN6K2OOBHsWTl3IkFsTdoC925C...
> 
>  This is looking a lot better, for completeness, can you pull the same numbers
> with the non-compact dictionary (i.e. the current code)?

Right. I did a similar comparison using the another trace, still consisting of
10 dumps (the long trace is generated expanding the keys, so this time is 100%
the same data):

Uncompressed trace:
2292 K (long dict)
1367 K (compact dict)

Compressing entire trace:
158 K (long dict)
147 K (compact dict)

Compressing each individual line:
160 k  (long dict)
148 k  (compact dict)

[Primiano Tucci (use gerrit), 2015-03-02 19:11:06]

On 2015/03/02 19:09:00, Primiano Tucci wrote:
> On 2015/03/02 14:35:46, dsinclair wrote:
> > On 2015/03/01 at 12:10:27, primiano wrote:
> > > On 2015/02/26 18:00:01, dsinclair wrote:
> > > > Not really a fan of this dump format over the old one. Do you have an
idea
> > of
> > > > the size comparison between this format and the old with shortened
names?
> > > > 
> > > >
> >
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > > > File base/trace_event/process_memory_maps.cc (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/947103003/diff/40001/base/trace_event/process...
> > > > base/trace_event/process_memory_maps.cc:12: const int kDumpVersion = 1; 
> //
> > For
> > > > tracing backwards compatibility,
> > > > Named keys would get rid of the need for this versioning ...
> > > 
> > > I did some rough math.
> > > In essence we are talking of
> > > 
> > > [This CL]
> > >   (1) "vm_regions_headers":[...stuff...] (150 bytes) +
> > >   (N) x [x1,x2,x3,x4,x5,x6,x7]
> > > 
> > > versus
> > > 
> > > [Shortened names]
> > >   (N) x {"sa":x1,"sz":x2,"pf":x3,"mf":x4,"mo":x5,"bs":{"a":x6,"r":x7}}
> > > 
> > > See the doc [1] for a more detailed explanation of the two variants.
> > > 
> > > The verbosity of the shortened names format has an extra cost of ~40 chars
> per
> > mmap entry.
> > > 
> > > The order of magnitude here is around 500-1000 mmaps per dump per (chrome)
> > process.
> > > Therefore, The difference between the compressed keys vs array translates
> into
> > 20k / 40k of overhead per dump per process.
> > > 
> > > 
> > > --------------------------------------------------
> > > Let's take a look to actual numbers (full traces)
> > > --------------------------------------------------
> > > Actual full numbers (i.e. structure overhead + actual data).
> > > 
> > > For a trace consisting of 10 mmaps dump points, the sizes of the two
> variants
> > are:
> > > 503 kB (headers+array) vs
> > > 959 kB (compact dict).
> > > 
> > > If I gzip the entire trace file, the sizes become
> > > 86 kB (headers+array) vs
> > > 96 kB (compact dict).
> > > 
> > > If I gzip + b64encode the individual dump lines (i.e. to emulate the case
> > where the trace is still plaintext JSON and the "args:" argument of each
dump
> is
> > compressed and b64 encoded) it becomes
> > > 117 kB (headers+array) vs
> > > 132 kB (compact dict).
> > > 
> > > 
> > > In the light of these numbers, I'd tend to agree with you. it sounds to me
> we
> > want to go for the compact dict and eventually look at some form of
> compression
> > in the future.
> > > Or eventually, in the future, do something more fancy like:
> > > most (say 80%) of the mmaps are unchanged between each snapshot. At trace
> > generation time, we could therefore delta-encode the mmaps, s.t. each dump
> > points become essentially a set of: (+) new mmaps 
> > > (-) deleted mmaps, (M) changes mmaps w.r.t the previous dump point.
> > > (Not sure, though, whether this is feasible and what is the impact in the
> > tracing architecture)
> > > 
> > > 
> > > [1]
> >
>
https://docs.google.com/a/chromium.org/document/d/1mN6K2OOBHsWTl3IkFsTdoC925C...
> > 
> >  This is looking a lot better, for completeness, can you pull the same
numbers
> > with the non-compact dictionary (i.e. the current code)?
> 
> Right. I did a similar comparison using the another trace, still consisting of
> 10 dumps (the long trace is generated expanding the keys, so this time is 100%
> the same data):
> 
> Uncompressed trace:
> 2292 K (long dict)
> 1367 K (compact dict)
> 
> Compressing entire trace:
> 158 K (long dict)
> 147 K (compact dict)
> 
> Compressing each individual line:
> 160 k  (long dict)
> 148 k  (compact dict)

Ah actually I just remembered that the actual compressed numbers for the
"compress individual line" case should be inflated by 4/3, to account for the
base64 encoding that would be required in that case (we can't just stuff args:
with random binary)

[dsinclair, 2015-03-16 15:28:10]

Doh, sorry for the delay, lost this in my review queue. LGTM.

[petrcermak, 2015-03-20 11:23:34]

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

[petrcermak, 2015-03-20 11:23:35]

Just a couple of comments :-)

Petr

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
File base/trace_event/process_memory_maps.cc (right):

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps.cc:26: static const char kHexFmt[] = "%"
PRIx64;
Is there an easy way to share this constant with
base/trace_event/process_memory_totals.cc as well?

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps.cc:41: value->SetString("prv",
nit: I'd make the coding style more consistent here by putting the line break
before "prv" and "shr" as well (like you did for "pss").

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

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
No reason for parentheses around 'counter_value * 1024' (same below).

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
Why do you *assign* to byte_stats_proportional_resident but *add* to
byte_stats_private_resident and byte_stats_shared_resident? If that's
intentional, you should probably add a comment explaining it.

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
There seems to be a little bit of code duplication here now. How about factoring
some code out into a function:

uint64 ExtractSmapsCounterBytes(std::istream* smaps) {
  uint64 counter_value;
  *smaps >> std::dec >> counter_value;
  return counter_value * 1024;
}

This would allow you to get rid of the counter_value variable and simplify the
if statement:

if (counter_name == "Pss:") {
  region->byte_states_proportional_resident += ExtractSmapsCounterBytes(smaps);
  res = 1;
} else if ...

[Primiano Tucci (use gerrit), 2015-03-20 11:48:49]

primiano@chromium.org changed reviewers:
- petrcermak@chromium.org

[Primiano Tucci (use gerrit), 2015-03-20 13:59:14]

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
File base/trace_event/process_memory_maps.cc (right):

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps.cc:26: static const char kHexFmt[] = "%"
PRIx64;
On 2015/03/20 11:23:34, petrcermak wrote:
> Is there an easy way to share this constant with
> base/trace_event/process_memory_totals.cc as well?

Yeah I thought about that, but sharing this would be more boilerplate and less
readability than just repeating this.
The drama becomes: which header should define kHexFmt (or the entire
StringPrintf?). A new one would be overkill

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps.cc:41: value->SetString("prv",
On 2015/03/20 11:23:34, petrcermak wrote:
> nit: I'd make the coding style more consistent here by putting the line break
> before "prv" and "shr" as well (like you did for "pss").

I agree, but then git cl-format insists with this format, and IMHO git cl-format
should have the last word.

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

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
On 2015/03/20 11:23:34, petrcermak wrote:
> There seems to be a little bit of code duplication here now. How about
factoring
> some code out into a function:
> 
> uint64 ExtractSmapsCounterBytes(std::istream* smaps) {
>   uint64 counter_value;
>   *smaps >> std::dec >> counter_value;
>   return counter_value * 1024;
> }
> 
> This would allow you to get rid of the counter_value variable and simplify the
> if statement:
> 
> if (counter_name == "Pss:") {
>   region->byte_states_proportional_resident +=
ExtractSmapsCounterBytes(smaps);
>   res = 1;
> } else if ...

Done.

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
On 2015/03/20 11:23:34, petrcermak wrote:
> Why do you *assign* to byte_stats_proportional_resident but *add* to
> byte_stats_private_resident and byte_stats_shared_resident? If that's
> intentional, you should probably add a comment explaining it.

I thought that was clear by the fact that Private/Shared has two items (shared
and clean) which are added up.
Added comment.

https://codereview.chromium.org/947103003/diff/80001/base/trace_event/process...
base/trace_event/process_memory_maps_dump_provider.cc:89:
region->byte_stats_proportional_resident = (counter_value * 1024);
On 2015/03/20 11:23:34, petrcermak wrote:
> No reason for parentheses around 'counter_value * 1024' (same below).

Done.

[Primiano Tucci (use gerrit), 2015-03-20 14:01:03]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2015-03-20 14:01:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/947103003/#ps100001
(title: "Address Petr's comments")

[commit-bot: I haz the power, 2015-03-20 14:01:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/947103003/100001

[commit-bot: I haz the power, 2015-03-20 15:20:44]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-03-20 15:21:24]

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