Endless thread safe timeline recorder

Endless thread safe timeline recorder

- Add a block based TimelineEventRecorder.
- Cache a block in each Thread's state.
- Locking is only done when allocating a new block.

R=koda@google.com

Committed: https://github.com/dart-lang/sdk/commit/4c9e8334c873d558c311140212934c87b69ae87c

[Cutch, 2015-08-06 20:11:10]

johnmccutchan@google.com changed reviewers:
+ koda@google.com

[koda, 2015-08-07 01:09:54]

First round of comments.

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.cc#newcod...
runtime/vm/thread.cc:117: // Thread doesn't have a timeline block or it is full,
request a new one.
Why not let these cases be handled naturally when we actually try to add another
entry?

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.h#newcode191
runtime/vm/thread.h:191: TimelineEventBlock* current_block;
Name is a bit too general; consider adding "_timeline" or "_event" to avoid
confusion with store_buffer_block. Ditto below.

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.cc
File runtime/vm/timeline.cc (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.cc#newc...
runtime/vm/timeline.cc:587: void
TimelineEventEndlessRecorder::PrintJSONEvents(JSONArray* events) const {
Since you don't lock here and in StartEvent, how do you ensure no thread is
still adding entries into its block?

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.h
File runtime/vm/timeline.h (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.h#newco...
runtime/vm/timeline.h:384: // Acquire a new block of events. Takes a lock.
Clarify that memory ownership remains with the recorder, and that only the
thread that requests the block should write into it.

[Cutch, 2015-08-07 16:47:42]

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.cc
File runtime/vm/thread.cc (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.cc#newcod...
runtime/vm/thread.cc:117: // Thread doesn't have a timeline block or it is full,
request a new one.
On 2015/08/07 01:09:54, koda wrote:
> Why not let these cases be handled naturally when we actually try to add
another
> entry?

Done.

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/thread.h#newcode191
runtime/vm/thread.h:191: TimelineEventBlock* current_block;
On 2015/08/07 01:09:54, koda wrote:
> Name is a bit too general; consider adding "_timeline" or "_event" to avoid
> confusion with store_buffer_block. Ditto below.

Done.

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.cc
File runtime/vm/timeline.cc (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.cc#newc...
runtime/vm/timeline.cc:587: void
TimelineEventEndlessRecorder::PrintJSONEvents(JSONArray* events) const {
On 2015/08/07 01:09:54, koda wrote:
> Since you don't lock here and in StartEvent, how do you ensure no thread is
> still adding entries into its block?

Acknowledged.

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.h
File runtime/vm/timeline.h (right):

https://codereview.chromium.org/1275113003/diff/1/runtime/vm/timeline.h#newco...
runtime/vm/timeline.h:384: // Acquire a new block of events. Takes a lock.
On 2015/08/07 01:09:54, koda wrote:
> Clarify that memory ownership remains with the recorder, and that only the
> thread that requests the block should write into it.

Done.

[koda, 2015-08-07 17:29:29]

lgtm

[Cutch, 2015-08-07 17:55:18]

Committed patchset #4 (id:60001) manually as
4c9e8334c873d558c311140212934c87b69ae87c (presubmit successful).