tracing v2: Introduce TraceBufferWriter

tracing v2: Introduce TraceBufferWriter

TraceBufferWriter is the main building block that allows writing
events into the trace ring buffer. TBW is responsible of exchanging
chunks with the ring buffer and carving out events out of them.
The expected design is 1 TraceBufferWriter per thread (% very special
cases like worker pools which don't have a message loop).
The major challenge is represented by the fact that events can spread
over several chunks. TBW is responsible of owning chunks for the
all lifetime of the event, and returning them as soon as the event
is finalized.
In the next CLs:
 - This code will be properly integrated with the stubs generated by
   the protozero protoc compiler plugin.
 - The actual event.proto will be introduced, and TBW::AddEvent will
   return a more structured object other than just a ProtoZeroMessage.

BUG=608719
TEST=TraceBufferWriterTest.*

Committed: https://crrev.com/9cc78e4aea540c6eba8655d8557b068bde423152
Cr-Commit-Position: refs/heads/master@{#410669}

[Primiano Tucci (use gerrit), 2016-07-29 09:52:06]

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

[commit-bot: I haz the power, 2016-07-29 09:52:22]

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-07-29 10:21:24]

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

[commit-bot: I haz the power, 2016-07-29 10:21:25]

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

[Primiano Tucci (use gerrit), 2016-07-29 10:55:20]

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

[commit-bot: I haz the power, 2016-07-29 10:55:33]

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-07-29 10:56:26]

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

[Primiano Tucci (use gerrit), 2016-07-29 10:56:27]

ok this might take a bit to digest, but is one of the biggest pieces.

[commit-bot: I haz the power, 2016-07-29 11:52:07]

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

[commit-bot: I haz the power, 2016-07-29 11:52:08]

Dry run: This issue passed the CQ dry run.

[Primiano Tucci (use gerrit), 2016-08-04 15:42:54]

On 2016/07/29 10:56:27, Primiano Tucci wrote:
> ok this might take a bit to digest, but is one of the biggest pieces.
I might have scared with this message. I meant that it's small and very easy to
review. PING :)

[alph, 2016-08-04 19:06:52]

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

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:82: WriteEventPrambleForNewChunk(
Preamble

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:119: // Backfill the size field
of the event with the partial size acccumulated
s/ccc/cc/

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:120: // so far in the old chunk.
WriteEventPrambleForNewChunk() will take care
Preamble

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:144: // events_chunk.proto cannot
be used here because that would re-enter this
Why it would reenter. We're at the very beginning of the chunk. It should have
enough space to fit the header. right?

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:162:
ChunkProto::kFirstEventContinuesFromPrevChunkFieldNumber));
This is an optional field. We could just skip it if the event is not fragmented.
Right?

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:166: // fragment and continue in
the next chunk. For the moment we put a zero as a
Could we optionally write this field when we're reaching the end of the chunk?

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

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:36: // |trace_buffer| is the
underlying ring buffer for taking and returning
you're referencing the arg name which is missing in the declaration.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:43: ~TraceBufferWriter();
Could you please make the Delegate destructor virtual? Just to be on the safe
side.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:56: // Finalize the pending event
(if any) and returns all chunks.
doesn't seem to return anything.

[Primiano Tucci (use gerrit), 2016-08-05 11:20:33]

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

[commit-bot: I haz the power, 2016-08-05 11:20:46]

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-08-05 11:23:38]

Thanks for the suggestion, simplified a bit the code.

I am not 100% happy about the test, feels to me it tests too much the
implementation details. But I'm realistically running out of time and will look
to that once back from holidays.

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

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:82: WriteEventPrambleForNewChunk(
On 2016/08/04 19:06:52, alph_slow wrote:
> Preamble

Oops.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:119: // Backfill the size field
of the event with the partial size acccumulated
On 2016/08/04 19:06:52, alph_slow wrote:
> s/ccc/cc/

But it can acccccccumulate a lot :P

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:120: // so far in the old chunk.
WriteEventPrambleForNewChunk() will take care
On 2016/08/04 19:06:52, alph_slow wrote:
> Preamble

yeah by virtue of autocompletion, I've been very consistent with this Pramble
apparently. Fixed all instances (I hope) :)

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:144: // events_chunk.proto cannot
be used here because that would re-enter this
On 2016/08/04 19:06:52, alph_slow wrote:
> Why it would reenter. We're at the very beginning of the chunk. It should have
> enough space to fit the header. right?

SO the situation is the following:
 - somebody is appending an event to the chunk via the TBWriter.
 - we reach the end of chunk, it means that the current SSWriter's cur_range is
exhausted.
 - The exhaustion causes the SSWriter to call back GetNewBuffer() to get a new
range (which TBWriter will map to a new chunk).
 - GetNewBuffer() in turn calls this function.
 - But at the moment, until this function returns, we still have the exahusted
range under our feet. The range will be changed only once this function returns.

So if I try to use the writer itself here, will re-enter because the logic loop
is "oh we ran out of space. Let me request a new chunk. Let me write the header
of the new chunk. Of we ran out of space".
I tried reshuffling this a bit around (e.g. resetting the SSWriter cur_range
here as soon as we get a new chunk) but the code looked still quite awkward,
even if it did technically work. To me the awkward bit is the the logic cycle
where: proto messages use the TBWriter to serialize. But in turn TBWriter uses a
proto message to serialize the header of the chunk itself.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:162:
ChunkProto::kFirstEventContinuesFromPrevChunkFieldNumber));
On 2016/08/04 19:06:52, alph_slow wrote:
> This is an optional field. We could just skip it if the event is not
fragmented.
> Right?

ah good point. yes. done.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:166: // fragment and continue in
the next chunk. For the moment we put a zero as a
On 2016/08/04 19:06:52, alph_slow wrote:
> Could we optionally write this field when we're reaching the end of the chunk?
Ah I think I see the suggestion here.
Instead of reserving and then backfilling, which causes to have the bit awkward
pointer to bool (continue_on_next_chunk_ptr_ ), which I never liked, I can do
the following:
When I acquire a chunk I tell the SSW that it's end is chunk_end - 2, so I
reserve the right of writing the final two bytes for the boolean.
I write them in FinalizeCurrentChunk.
Yes, just tried it out and it removes quite some complexity. Thanks!

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

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:36: // |trace_buffer| is the
underlying ring buffer for taking and returning
On 2016/08/04 19:06:52, alph_slow wrote:
> you're referencing the arg name which is missing in the declaration.

Done.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:43: ~TraceBufferWriter();
On 2016/08/04 19:06:52, alph_slow wrote:
> Could you please make the Delegate destructor virtual? Just to be on the safe
> side.

oh yeah sure.

https://codereview.chromium.org/2196663002/diff/20001/components/tracing/core...
components/tracing/core/trace_buffer_writer.h:56: // Finalize the pending event
(if any) and returns all chunks.
On 2016/08/04 19:06:52, alph_slow wrote:
> doesn't seem to return anything.

Reworded the comment. I meant "returns back" the chunks to the main buffer. Not
returns as in C return :)

[commit-bot: I haz the power, 2016-08-05 11:59:06]

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

[commit-bot: I haz the power, 2016-08-05 11:59:06]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Primiano Tucci (use gerrit), 2016-08-05 13:15:19]

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

[commit-bot: I haz the power, 2016-08-05 13:15:28]

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-08-05 14:16:36]

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

[commit-bot: I haz the power, 2016-08-05 14:16:37]

Dry run: This issue passed the CQ dry run.

[oystein (OOO til 10th of July), 2016-08-05 23:04:36]

Awesome, and sorry for the delay :).

lgtm w/ super minor comments only.

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

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:77: if
(stream_writer_.bytes_available() < 16)
nit: make this a named constant at least? Or is the idea that it'll be replaced
by a sizeof()?

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:99: size_t used_size =
stream_writer_.write_ptr() - chunk_->payload();
nit: size_t and then casting to uint32 after using it, feels weird to me. Maybe
DCHECK on write_ptr() - chunk_->payload() being within uint32_t range, then use
uint32_t instead of size_t?

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:107: *write_ptr = 1;
What's the 1?

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:108: used_size += 2;
Can you just store an original_write_ptr instead of used_size, then calculate
used_size below this conditional?

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:171: *chunk_proto++ = 1;
What does the 1 represent?

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:188: // keeps size-field the
bookeeping up to date. Returns the pointer in the chunk
nit: bookkeeping

[Primiano Tucci (use gerrit), 2016-08-08 19:36:23]

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

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:77: if
(stream_writer_.bytes_available() < 16)
On 2016/08/05 23:04:35, oystein wrote:
> nit: make this a named constant at least? Or is the idea that it'll be
replaced
> by a sizeof()?

Made a constant and moved the todo up there. It's going to be an estimation
based on the proto and the fields that we'll use for common (no arg) events.

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:99: size_t used_size =
stream_writer_.write_ptr() - chunk_->payload();
On 2016/08/05 23:04:32, oystein wrote:
> nit: size_t and then casting to uint32 after using it, feels weird to me.
Maybe
> DCHECK on write_ptr() - chunk_->payload() being within uint32_t range, then
use
> uint32_t instead of size_t?

Done.

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:107: *write_ptr = 1;
On 2016/08/05 23:04:35, oystein wrote:
> What's the 1?

1 == true -> the event does continue on the next chunk. 
The statement above is just the preamble that says "this field will tell whether
the event continues or not".
(I know, shame that a byte has to be wasted to encode one bit)

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:108: used_size += 2;
On 2016/08/05 23:04:32, oystein wrote:
> Can you just store an original_write_ptr instead of used_size, then calculate
> used_size below this conditional?

Done.

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:171: *chunk_proto++ = 1;
On 2016/08/05 23:04:32, oystein wrote:
> What does the 1 represent?

as above, the value of the bool.

https://codereview.chromium.org/2196663002/diff/60001/components/tracing/core...
components/tracing/core/trace_buffer_writer.cc:188: // keeps size-field the
bookeeping up to date. Returns the pointer in the chunk
On 2016/08/05 23:04:33, oystein wrote:
> nit: bookkeeping

Done.

[Primiano Tucci (use gerrit), 2016-08-08 19:36:28]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-08-08 19:36:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/2196663002/#ps80001
(title: "oysteine review + rebase")

[commit-bot: I haz the power, 2016-08-08 19:37:23]

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-08-08 19:46:20]

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

[commit-bot: I haz the power, 2016-08-08 19:46:21]

Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alph, 2016-08-08 21:47:53]

Thanks. lgtm.
Btw gyp support has been dropped. So you don't need to update gyp files anymore.

[Primiano Tucci (use gerrit), 2016-08-09 13:44:38]

The CQ bit was checked by primiano@chromium.org

[Primiano Tucci (use gerrit), 2016-08-09 13:44:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org, alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2196663002/#ps100001
(title: "Fix linking errors moving kChunkSize outside of exported class")

[commit-bot: I haz the power, 2016-08-09 13:45:01]

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-08-09 14:45:42]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-09 14:48:28]

Description was changed from

==========
tracing v2: Introduce TraceBufferWriter

TraceBufferWriter is the main building block that allows writing
events into the trace ring buffer. TBW is responsible of exchanging
chunks with the ring buffer and carving out events out of them.
The expected design is 1 TraceBufferWriter per thread (% very special
cases like worker pools which don't have a message loop).
The major challenge is represented by the fact that events can spread
over several chunks. TBW is responsible of owning chunks for the
all lifetime of the event, and returning them as soon as the event
is finalized.
In the next CLs:
 - This code will be properly integrated with the stubs generated by
   the protozero protoc compiler plugin.
 - The actual event.proto will be introduced, and TBW::AddEvent will
   return a more structured object other than just a ProtoZeroMessage.

BUG=608719
TEST=TraceBufferWriterTest.*
==========

to

==========
tracing v2: Introduce TraceBufferWriter

TraceBufferWriter is the main building block that allows writing
events into the trace ring buffer. TBW is responsible of exchanging
chunks with the ring buffer and carving out events out of them.
The expected design is 1 TraceBufferWriter per thread (% very special
cases like worker pools which don't have a message loop).
The major challenge is represented by the fact that events can spread
over several chunks. TBW is responsible of owning chunks for the
all lifetime of the event, and returning them as soon as the event
is finalized.
In the next CLs:
 - This code will be properly integrated with the stubs generated by
   the protozero protoc compiler plugin.
 - The actual event.proto will be introduced, and TBW::AddEvent will
   return a more structured object other than just a ProtoZeroMessage.

BUG=608719
TEST=TraceBufferWriterTest.*

Committed: https://crrev.com/9cc78e4aea540c6eba8655d8557b068bde423152
Cr-Commit-Position: refs/heads/master@{#410669}
==========

[commit-bot: I haz the power, 2016-08-09 14:48:29]

Patchset 6 (id:??) landed as
https://crrev.com/9cc78e4aea540c6eba8655d8557b068bde423152
Cr-Commit-Position: refs/heads/master@{#410669}

[huangs, 2016-08-09 18:09:20]

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

[huangs, 2016-08-09 18:09:21]

Looks like the following tests are failing in "Win7 Tests (dbg)(1)":

TraceBufferWriterTest.ManyWriters
TraceBufferWriterTest.ManySmallEvents
TraceBufferWriterTest.OneWriterWithFragmentingEvents
TraceBufferWriterTest.SingleEvent

Failure seems to be caused by inability to find content_shell.pak ?  Going to
revert this CL.

[huangs, 2016-08-09 18:11:49]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2228593004/ by huangs@chromium.org.

The reason for reverting is: Looks like the following tests are failing in "Win7
Tests (dbg)(1)":

TraceBufferWriterTest.ManyWriters
TraceBufferWriterTest.ManySmallEvents
TraceBufferWriterTest.OneWriterWithFragmentingEvents
TraceBufferWriterTest.SingleEven
.