tracing v2: Introduce TraceBufferReader to read-back the trace buffer

tracing v2: Introduce TraceBufferReader to read-back the trace buffer

This CL introduces the core building blocks to read-back the
proto-encoded trace buffer. This will serve two purposes:
1. Build the table of interned strings. We use interned strings (see
   event.proto) to avoid doing full string copies for each event when
   those strings are long lived. The interned string table will be
   attached at trace finalization time via metadata events by upcoming
   CLs.
2. Build the base::Value(s) that can be converted to JSON which matches
   the current trace event format. This is to support JSON output in the
   interim stage of Tracing V2 (protobuf inside, JSON outside).

BUG=643674

[Primiano Tucci (use gerrit), 2016-09-02 15:18:52]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 15:19:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Primiano Tucci (use gerrit), 2016-09-02 15:19:23]

primiano@chromium.org changed reviewers:
+ kraynov@chromium.org, oysteine@chromium.org

[Primiano Tucci (use gerrit), 2016-09-02 15:19:23]

Ok that's all for this week :P

[commit-bot: I haz the power, 2016-09-02 15:22:23]

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

[commit-bot: I haz the power, 2016-09-02 15:22:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2016-09-05 13:36:32]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-05 13:36:47]

Dry run: 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, 2016-09-05 13:47:35]

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

[commit-bot: I haz the power, 2016-09-05 13:47:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Primiano Tucci (use gerrit), 2016-09-08 15:34:26]

PING

[Primiano Tucci (use gerrit), 2016-09-08 15:36:37]

On 2016/09/08 15:34:26, Primiano Tucci wrote:
> PING

(the red bots are due to some silly unsigned vs signed DCHECK, will deal with
them soon)

[kraynov, 2016-09-08 16:10:01]

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.cc:15: // on OSX.
Yep, maybe dynamic linker has some tricks as well.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:43: DCHECK(offset >=
std::numeric_limits<int64_t>::min() &&
Static assert sizeof(ptrdiff_t) <= 8

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:124:
writer_state->partial_event_data.insert(
move "insert" to new line. Hard to read.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:26: static const uint32_t
kBuildInternedStringsTable = 1 << 0;
I don't see any point to use bit options. Is it expected to get many? It smells
like WinAPI otherwise :)

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:33: void ParseChunk(const uint8_t*
start, size_t length);
This pair of overloaded methods looks confusing. I suggest to use different
names, e.g. ParseChunk(Chunk*) ParseDataRange(...).

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:50: std::vector<uint8_t>
partial_event_data;
We took an effort to use dynamic allocation as least as we can. I think fixed
size buffer is more appropriate, but vector is also good as a temporay solution.

[oystein (OOO til 10th of July), 2016-09-09 00:27:35]

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.cc:16: const char kInternedStringBase[]
= "Chrome Tracing v2";
supernit: just "Tracing v2", there's other embedders.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:25: //    table. This case works
always (as long as the strings are long lived)
s/works always/always works/

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:30: //    of non-component builds. In
a monolithic build string offset is constatnt
s/build string offset is constatnt/build the string offset is constant/

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:60: field_payload = pos -
field_intvalue;
Can we do a DCHECK_LT(field_intvalue, limits::numerical_limits<uint32_t> in the
negative case here? Looks like all of the other cases static_cast it down or use
it as a flag.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:92: WriterState* writer_state =
&writer_state_[writer_id];
I'm finding it hard to reason about the usage of the writer_state_ entries, can
you explain it a bit? The writer state is sometimes carried over from one chunk
to the next? Or even more, in case of nested messages?

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:150:
base::StringPiece(reinterpret_cast<const char*>(pos - len), len);
DCHECK_LT(pos, len)?

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:162: switch (field_id) {
Should there be a default NOTREACHED() here?

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:164: char event_phase[2] =
{static_cast<char>(field_intvalue), '\0'};
nit: Can't you leave out the '2' here?

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:26: static const uint32_t
kBuildInternedStringsTable = 1 << 0;
On 2016/09/08 16:10:01, kraynov wrote:
> I don't see any point to use bit options. Is it expected to get many? It
smells
> like WinAPI otherwise :)

Ditto; are there actually any cases where both would be set at the same time?
Wouldn't it always be one or the other?

[Primiano Tucci (use gerrit), 2016-09-13 14:40:22]

The CQ bit was checked by primiano@chromium.org to run a CQ dry run

[Primiano Tucci (use gerrit), 2016-09-13 14:40:25]

Thanks y all. replies below

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.cc:15: // on OSX.
On 2016/09/08 16:10:01, kraynov wrote:
> Yep, maybe dynamic linker has some tricks as well.

Acknowledged.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.cc:16: const char kInternedStringBase[]
= "Chrome Tracing v2";
On 2016/09/09 00:27:34, oystein wrote:
> supernit: just "Tracing v2", there's other embedders.

Done.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/string_interning.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:25: //    table. This case works
always (as long as the strings are long lived)
On 2016/09/09 00:27:34, oystein wrote:
> s/works always/always works/

Done.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:30: //    of non-component builds. In
a monolithic build string offset is constatnt
On 2016/09/09 00:27:34, oystein wrote:
> s/build string offset is constatnt/build the string offset is constant/

Done.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/string_interning.h:43: DCHECK(offset >=
std::numeric_limits<int64_t>::min() &&
I had to remove this DCHECK completely as it was a tautological compare.

On 2016/09/08 16:10:01, kraynov wrote:
> Static assert sizeof(ptrdiff_t) <= 8
The day that sizeof(ptr*) is > 8 my grand grand nephews will be living on
another planet :)

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.cc (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:60: field_payload = pos -
field_intvalue;
On 2016/09/09 00:27:35, oystein wrote:
> Can we do a DCHECK_LT(field_intvalue, limits::numerical_limits<uint32_t> in
the
> negative case here? Looks like all of the other cases static_cast it down or
use
> it as a flag.

The reason I didn't put it is because ParseField has that sanity check in
ParseField. However I agree makes the code more legible, so I added a dcheck
here that ensures that the final payload is between the last field and the next
one.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:92: WriterState* writer_state =
&writer_state_[writer_id];
On 2016/09/09 00:27:35, oystein wrote:
> I'm finding it hard to reason about the usage of the writer_state_ entries,
can
> you explain it a bit? The writer state is sometimes carried over from one
chunk
> to the next? Or even more, in case of nested messages?

Allright, I added a comment to the WriterState struct.
Essentially for each writer (read, for each thread) i have to keep track of:
- the sequencing of chunks: all I need here is to established if I saw all
chunks for a thread, or if some chunk got dropped because of the ring-buffer
wrapping.
- accumulating the bytes of fragmented events across chunks.

In essence, when I see fragments I have to merge them, but only if I saw all the
chunks for that fragment contiguously, otherwise I just have to drop the event.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:124:
writer_state->partial_event_data.insert(
On 2016/09/08 16:10:01, kraynov wrote:
> move "insert" to new line. Hard to read.

Done.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:150:
base::StringPiece(reinterpret_cast<const char*>(pos - len), len);
On 2016/09/09 00:27:35, oystein wrote:
> DCHECK_LT(pos, len)?

Done (more strictly)

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:162: switch (field_id) {
On 2016/09/09 00:27:35, oystein wrote:
> Should there be a default NOTREACHED() here?

Hmm good question. So the case to cover here is some field that we want in the
proto but we ignore for the  readback -> JSON.
We might have fields lke that, but in that case I agree to stay conservative and
have them listed as an no-op case here, to make sure that if somebody adds a new
field during the interim phase we don't forget about it here.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:164: char event_phase[2] =
{static_cast<char>(field_intvalue), '\0'};
On 2016/09/09 00:27:34, oystein wrote:
> nit: Can't you leave out the '2' here?

Done.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.h (right):

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:26: static const uint32_t
kBuildInternedStringsTable = 1 << 0;
On 2016/09/09 00:27:35, oystein wrote:
> On 2016/09/08 16:10:01, kraynov wrote:
> > I don't see any point to use bit options. Is it expected to get many? It
> smells
> > like WinAPI otherwise :)
> 
> Ditto; are there actually any cases where both would be set at the same time?
> Wouldn't it always be one or the other?

This is intended to be or-able (see the test).
In the transition period I think we'll need both the JSON and the string table
for the proto metadata (at very least for tests)

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:33: void ParseChunk(const uint8_t*
start, size_t length);
On 2016/09/08 16:10:01, kraynov wrote:
> This pair of overloaded methods looks confusing. I suggest to use different
> names, e.g. ParseChunk(Chunk*) ParseDataRange(...).

ok. Lemme just leave the start/length one for the moment and use that also in
prod code.

https://codereview.chromium.org/2308583003/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_reader.h:50: std::vector<uint8_t>
partial_event_data;
On 2016/09/08 16:10:01, kraynov wrote:
> We took an effort to use dynamic allocation as least as we can. I think fixed
> size buffer is more appropriate, but vector is also good as a temporay
solution.

This happens when the trace is finalized, at this point we are out of the
fastpath.
This buffer is temporary. It can't be easily fixed because in theory one can
create a jumbo event as big as the trace which spans across all chunks. It would
be a waste creating such a big array, because in 99% of cases this will be
barely used.

[commit-bot: I haz the power, 2016-09-13 14:40:38]

Dry run: 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, 2016-09-13 14:53:50]

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

[commit-bot: I haz the power, 2016-09-13 14:53:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[kraynov, 2016-09-13 15:28:39]

Thanks, lgtm

[oystein (OOO til 10th of July), 2016-09-13 20:16:18]

lgtm, nice!

[Primiano Tucci (use gerrit), 2016-09-15 11:48:03]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-09-15 11:48:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, kraynov@chromium.org
Link to the patchset: https://codereview.chromium.org/2308583003/#ps60001
(title: "DISALLOW_COPY_AND_ASSIGN + cctype for isalnum")

[commit-bot: I haz the power, 2016-09-15 11:48:19]

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, 2016-09-15 11:59:57]

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

[commit-bot: I haz the power, 2016-09-15 11:59:59]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DmitrySkiba, 2016-11-29 18:09:56]

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

[DmitrySkiba, 2016-11-29 18:09:57]

https://codereview.chromium.org/2308583003/diff/60001/components/tracing/core...
File components/tracing/core/string_interning.h (right):

https://codereview.chromium.org/2308583003/diff/60001/components/tracing/core...
components/tracing/core/string_interning.h:30: //    non-component builds. In a
monolithic build the string offset is constatnt
constatnt -> constant

https://codereview.chromium.org/2308583003/diff/60001/components/tracing/core...
components/tracing/core/string_interning.h:38: inline int64_t InternString(const
char* str) {
Maybe rename "intern" to "index" or "stable offset", or something? In my head
"interning" means this:
http://docs.oracle.com/javase/7/docs/api/java/lang/String.html#intern%28%29

I.e. there is a pool of strings, and "interning" means that we take arbitrary
string and return either its counterpart from the pool, or add the string to the
pool (and return it).

https://codereview.chromium.org/2308583003/diff/60001/components/tracing/core...
File components/tracing/core/trace_buffer_reader.cc (right):

https://codereview.chromium.org/2308583003/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_reader.cc:172: interned_strings_[str_id] =
interned_string;
Hmm, str_id is int64_t here, but interned_strings_ is keyed by int32_t.