Record Timeline to Android Systrace

Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder (enabled via --systrace_timeline or --timeline_recoder=systrace)
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
R=chinmaygarde@google.com, rmacnak@google.com

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

[Cutch, 2016-09-08 16:30:59]

Description was changed from

==========
Add TimelineEventSystraceRecorder

BUG=
==========

to

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder
- [ ] Unit test PrintSystrace

BUG=
==========

[Cutch, 2016-09-08 16:30:59]

johnmccutchan@google.com changed reviewers:
+ chinmaygarde@google.com, rmacnak@google.com

[Cutch, 2016-09-08 17:39:45]

Description was changed from

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder
- [ ] Unit test PrintSystrace

BUG=
==========

to

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
==========

[Cutch, 2016-09-08 17:40:54]

Description was changed from

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
==========

to

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder (enabled via --systrace_timeline or
--timeline_recoder=systrace)
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
==========

[Chinmay, 2016-09-08 17:58:12]

lgtm with some minor comments.

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

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:96: if (use_systrace_recorder || (strcmp("systrace",
flag) == 0)) {
Maybe these two if statements can be rewritten as |use_systrace_recorder ||
(flag != NULL && strcmp("systrace", flag) == 0)|.

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1442: const intptr_t kBufferLength = 1024;
This might be larger than necessary. I think systrace itself has a limit on the
size of the string it accepts (though I am not able to find what it is ATM). 256
should be sufficient.

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1447: ssize_t result = write(systrace_fd_, buffer,
event_length);
Might be useful to not ignore at least EINTR. I find it matters only when a
debugger is attached.

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

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.h#n...
runtime/vm/timeline.h:812: int systrace_fd_;
Should this be private? Along with the usual `DISALLOW_COPY_AND_ASSIGN`
decoration on the class.

[Cutch, 2016-09-08 18:06:10]

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

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:96: if (use_systrace_recorder || (strcmp("systrace",
flag) == 0)) {
On 2016/09/08 17:58:11, Chinmay wrote:
> Maybe these two if statements can be rewritten as |use_systrace_recorder ||
> (flag != NULL && strcmp("systrace", flag) == 0)|.

Acknowledged.

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1442: const intptr_t kBufferLength = 1024;
On 2016/09/08 17:58:11, Chinmay wrote:
> This might be larger than necessary. I think systrace itself has a limit on
the
> size of the string it accepts (though I am not able to find what it is ATM).
256
> should be sufficient.

I took this number from Android's sources.

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1447: ssize_t result = write(systrace_fd_, buffer,
event_length);
On 2016/09/08 17:58:11, Chinmay wrote:
> Might be useful to not ignore at least EINTR. I find it matters only when a
> debugger is attached.

Done.

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

https://codereview.chromium.org/2315353004/diff/20001/runtime/vm/timeline.h#n...
runtime/vm/timeline.h:812: int systrace_fd_;
On 2016/09/08 17:58:11, Chinmay wrote:
> Should this be private? Along with the usual `DISALLOW_COPY_AND_ASSIGN`
> decoration on the class.

The base class has the decoration.

[rmacnak, 2016-09-08 18:17:09]

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

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:520: length = snprintf(buffer, buffer_size, "B|%" Pd64
"|%s", pid, label());
OS::SNPrint

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:524: length = snprintf(buffer, buffer_size, "E");
"

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1408: systrace_fd_ = open(kSystracePath, O_WRONLY);
ifdef TARGET_OS_LINUX or TARGET_OS_ANDROID

else
  OS::PrintErr("Warning: systrace timeline recorder is equivalent to the ring
record on this platform");

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1440: if (systrace_fd_ >= 0) {
ifdef TARGET_OS_LINUX or TARGET_OS_ANDROID

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline_tes...
File runtime/vm/timeline_test.cc (right):

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline_tes...
runtime/vm/timeline_test.cc:214: // Test a duration event. This event kind is
not supported so we should
Most of our events are durations, maybe we should emit a begin/end pair to
systrace?

[Cutch, 2016-09-08 18:24:08]

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

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:520: length = snprintf(buffer, buffer_size, "B|%" Pd64
"|%s", pid, label());
On 2016/09/08 18:17:09, rmacnak wrote:
> OS::SNPrint

Done.

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:524: length = snprintf(buffer, buffer_size, "E");
On 2016/09/08 18:17:09, rmacnak wrote:
> "

Done.

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1408: systrace_fd_ = open(kSystracePath, O_WRONLY);
On 2016/09/08 18:17:09, rmacnak wrote:
> ifdef TARGET_OS_LINUX or TARGET_OS_ANDROID
> 
> else
>   OS::PrintErr("Warning: systrace timeline recorder is equivalent to the ring
> record on this platform");

Done.

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline.cc#...
runtime/vm/timeline.cc:1440: if (systrace_fd_ >= 0) {
On 2016/09/08 18:17:09, rmacnak wrote:
> ifdef TARGET_OS_LINUX or TARGET_OS_ANDROID

Done.

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline_tes...
File runtime/vm/timeline_test.cc (right):

https://codereview.chromium.org/2315353004/diff/40001/runtime/vm/timeline_tes...
runtime/vm/timeline_test.cc:214: // Test a duration event. This event kind is
not supported so we should
On 2016/09/08 18:17:09, rmacnak wrote:
> Most of our events are durations, maybe we should emit a begin/end pair to
> systrace?

Chinmay and I discussed this and we've decided to wait and see.

[rmacnak, 2016-09-08 18:27:33]

lgtm

[Cutch, 2016-09-08 18:28:40]

Description was changed from

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder (enabled via --systrace_timeline or
--timeline_recoder=systrace)
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
==========

to

==========
Record Timeline to Android Systrace

- [x] Add TimelineEventSystraceRecorder (enabled via --systrace_timeline or
--timeline_recoder=systrace)
- [x] Support Begin, End, and Counter events
- [x] Unit test Systrace serialization

BUG=
R=chinmaygarde@google.com, rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/ed3f4f2dfebccf267f508472c9669046cc9b4b54
==========

[Cutch, 2016-09-08 18:28:41]

Committed patchset #5 (id:80001) manually as
ed3f4f2dfebccf267f508472c9669046cc9b4b54 (presubmit successful).