Add support for TraceEvent before the native library is loaded.

Add support for TraceEvent before the native library is loaded.

Currently trace events that occur before the native library is initialized are
ignored.

This CL fixes that by buffering the events that occur before the initialization
of the native library on the Java side. Those events then get sent to the
native TraceLog before the trace file gets written to disk.

NOTE: this is only reviving pliard@'s CL (issue 230603003).

BUG=317481

[Benoit L, 2015-01-27 13:54:52]

lizeb@chromium.org changed reviewers:
+ pasko@chromium.org

[Benoit L, 2015-01-27 13:54:53]

Hello,

Here is the CL reviving the early TraceEvent CL from pliard@.

[dsinclair, 2015-01-27 14:25:39]

On 2015/01/27 at 13:54:53, lizeb wrote:
> Hello,
> 
> Here is the CL reviving the early TraceEvent CL from pliard@.

The base/debug/trace_event* files just moved to base/trace_event/* with
https://codereview.chromium.org/837303004 so you'll need to update this CL.

[dsinclair, 2015-01-27 14:43:29]

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

[dsinclair, 2015-01-27 14:43:31]

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1754: // Note that the TraceEvent frontend
expects the trace events to be sorted.
This seems like it's adding a lot of extra work. We have iterate the chunks, and
to the buffer, sort the buffer and then iterate the buffer where as before we
just walked over the chunks.

Why can Add custom send events out of order? As soon as tracing starts can it
not send all of it's events through? Or do the still end up slightly out of
order?  We're doing a lot of work here for that special case.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1769: std::stable_sort(trace_events.begin(),
trace_events.end(),
This seems like it could be expensive if we have to run the sort on each of the
buffers.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1775: std::vector<const
TraceEvent*>::const_iterator it = trace_events.begin();
The change to use an iterator seems unrelated to this CL, can you pull it out
into a separate CL?

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:2095: if (!(flags &
TRACE_EVENT_FLAG_DONT_REPORT_OVERHEAD))
Why would we want to skip reporting the overhead? Seems like we'll just end up
hiding causes of slowness in the tracing system.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:550: TraceEventHandle AddCustomTraceEvent(
Instead of AddCustomTraceEvent, I think we should either update
AddTraceEventWithThreadIdAndTimestamp to also take the thread_now or add a
variant of that method that takes thread_now.

[pasko, 2015-01-27 15:36:01]

pasko@chromium.org changed reviewers:
+ pliard@google.com

[pasko, 2015-01-27 15:36:02]

I don't have a strong opinion about AddCustomTraceEvent, but if we can avoid
sorting the events with early startup events , let's try avoiding it.

Philippe, was there any reason why we wanted to be very flexible with
AddCustomTraceEvent?

https://codereview.chromium.org/874543003/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/874543003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:122:
if (CommandLine.getInstance().hasSwitch("trace-startup")) {
FYI: we are going to have global tracing state in the system, so this piece
would check for a special file on the device

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:15:
import org.chromium.base.TraceEvent;
nit: the TraceEvent.java has a comment at the top about ignoring the early trace
events, please remove that

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:251:
TraceEvent.begin();
does this compile? All the instances of TraceEvent.begin() I am seeing require a
string parameter.

[chromium-reviews, 2015-02-03 16:01:51]

This is quite far for me now and wasn't meant at that time to be sent for
review as is (i.e. this was more a hack). I think I had added
AddCustomTraceEvent() mainly to avoid changing the signature of the
existing method and having to update all the call sites. This should be
doable though.

As for sorting, this was the easiest solution to me to ensure that all
(Java + C++) the tracing events were ordered. Without sorting, they are out
of order since all the Java early events get inserted afterwards in the
native trace log all at once, i.e. they are not inserted in the native
trace log as they are recorded (since there is no native trace log that
early).
There are probably more efficient solutions though like merging two sorted
arrays if you keep the Java events somehow in a separate native trace log
but I don't know how well that would integrate with the current design. It
seemed to me at that time that increased dumping overhead may be reasonable
as long as recording overhead stay the same. You may also be able to do the
sort only when Java events get inserted or when detecting an event that is
out of order. That way you wouldn't impact performance much when early
tracing is not used.

On Tue, Jan 27, 2015 at 10:36 AM, <pasko@chromium.org> wrote:

> I don't have a strong opinion about AddCustomTraceEvent, but if we can
> avoid
> sorting the events with early startup events , let's try avoiding it.
>
> Philippe, was there any reason why we wanted to be very flexible with
> AddCustomTraceEvent?
>
>
> https://codereview.chromium.org/874543003/diff/20001/
> chrome/android/shell/java/src/org/chromium/chrome/shell/
> ChromeShellActivity.java
> File
> chrome/android/shell/java/src/org/chromium/chrome/shell/
> ChromeShellActivity.java
> (right):
>
> https://codereview.chromium.org/874543003/diff/20001/
> chrome/android/shell/java/src/org/chromium/chrome/shell/
> ChromeShellActivity.java#newcode122
> chrome/android/shell/java/src/org/chromium/chrome/shell/
> ChromeShellActivity.java:122:
> if (CommandLine.getInstance().hasSwitch("trace-startup")) {
> FYI: we are going to have global tracing state in the system, so this
> piece would check for a special file on the device
>
> https://codereview.chromium.org/874543003/diff/20001/
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java
> File
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java
> (right):
>
> https://codereview.chromium.org/874543003/diff/20001/
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java#newcode15
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java:15:
> import org.chromium.base.TraceEvent;
> nit: the TraceEvent.java has a comment at the top about ignoring the
> early trace events, please remove that
>
> https://codereview.chromium.org/874543003/diff/20001/
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java#newcode251
> content/public/android/java/src/org/chromium/content/browser/
> BrowserStartupController.java:251:
> TraceEvent.begin();
> does this compile? All the instances of TraceEvent.begin() I am seeing
> require a string parameter.
>
> https://codereview.chromium.org/874543003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nduca, 2015-02-03 16:31:35]

Trace event's dont have to be inserted in timestamp order into the file.

On Tue, Feb 3, 2015 at 8:01 AM, Philippe Liard <pliard@google.com> wrote:

> This is quite far for me now and wasn't meant at that time to be sent for
> review as is (i.e. this was more a hack). I think I had added
> AddCustomTraceEvent() mainly to avoid changing the signature of the
> existing method and having to update all the call sites. This should be
> doable though.
>
> As for sorting, this was the easiest solution to me to ensure that all
> (Java + C++) the tracing events were ordered. Without sorting, they are out
> of order since all the Java early events get inserted afterwards in the
> native trace log all at once, i.e. they are not inserted in the native
> trace log as they are recorded (since there is no native trace log that
> early).
> There are probably more efficient solutions though like merging two sorted
> arrays if you keep the Java events somehow in a separate native trace log
> but I don't know how well that would integrate with the current design. It
> seemed to me at that time that increased dumping overhead may be reasonable
> as long as recording overhead stay the same. You may also be able to do the
> sort only when Java events get inserted or when detecting an event that is
> out of order. That way you wouldn't impact performance much when early
> tracing is not used.
>
> On Tue, Jan 27, 2015 at 10:36 AM, <pasko@chromium.org> wrote:
>
>> I don't have a strong opinion about AddCustomTraceEvent, but if we can
>> avoid
>> sorting the events with early startup events , let's try avoiding it.
>>
>> Philippe, was there any reason why we wanted to be very flexible with
>> AddCustomTraceEvent?
>>
>>
>> https://codereview.chromium.org/874543003/diff/20001/
>> chrome/android/shell/java/src/org/chromium/chrome/shell/
>> ChromeShellActivity.java
>> File
>> chrome/android/shell/java/src/org/chromium/chrome/shell/
>> ChromeShellActivity.java
>> (right):
>>
>> https://codereview.chromium.org/874543003/diff/20001/
>> chrome/android/shell/java/src/org/chromium/chrome/shell/
>> ChromeShellActivity.java#newcode122
>> chrome/android/shell/java/src/org/chromium/chrome/shell/
>> ChromeShellActivity.java:122:
>> if (CommandLine.getInstance().hasSwitch("trace-startup")) {
>> FYI: we are going to have global tracing state in the system, so this
>> piece would check for a special file on the device
>>
>> https://codereview.chromium.org/874543003/diff/20001/
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java
>> File
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java
>> (right):
>>
>> https://codereview.chromium.org/874543003/diff/20001/
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java#newcode15
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java:15:
>> import org.chromium.base.TraceEvent;
>> nit: the TraceEvent.java has a comment at the top about ignoring the
>> early trace events, please remove that
>>
>> https://codereview.chromium.org/874543003/diff/20001/
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java#newcode251
>> content/public/android/java/src/org/chromium/content/browser/
>> BrowserStartupController.java:251:
>> TraceEvent.begin();
>> does this compile? All the instances of TraceEvent.begin() I am seeing
>> require a string parameter.
>>
>> https://codereview.chromium.org/874543003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Benoit L, 2015-02-09 16:34:01]

Thank you for the review.
I have addressed the first round of comments. Indeed, not sorting the events
works and simplifies the code quite a bit.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1754: // Note that the TraceEvent frontend
expects the trace events to be sorted.
On 2015/01/27 14:43:30, dsinclair wrote:
> This seems like it's adding a lot of extra work. We have iterate the chunks,
and
> to the buffer, sort the buffer and then iterate the buffer where as before we
> just walked over the chunks.
> 
> Why can Add custom send events out of order? As soon as tracing starts can it
> not send all of it's events through? Or do the still end up slightly out of
> order?  We're doing a lot of work here for that special case.

Done.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1769: std::stable_sort(trace_events.begin(),
trace_events.end(),
On 2015/01/27 14:43:30, dsinclair wrote:
> This seems like it could be expensive if we have to run the sort on each of
the
> buffers.

Done.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:1775: std::vector<const
TraceEvent*>::const_iterator it = trace_events.begin();
On 2015/01/27 14:43:30, dsinclair wrote:
> The change to use an iterator seems unrelated to this CL, can you pull it out
> into a separate CL?

Done.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.cc:2095: if (!(flags &
TRACE_EVENT_FLAG_DONT_REPORT_OVERHEAD))
On 2015/01/27 14:43:30, dsinclair wrote:
> Why would we want to skip reporting the overhead? Seems like we'll just end up
> hiding causes of slowness in the tracing system.

Done.

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/874543003/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:550: TraceEventHandle AddCustomTraceEvent(
On 2015/01/27 14:43:30, dsinclair wrote:
> Instead of AddCustomTraceEvent, I think we should either update
> AddTraceEventWithThreadIdAndTimestamp to also take the thread_now or add a
> variant of that method that takes thread_now. 

Done.

https://codereview.chromium.org/874543003/diff/20001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/874543003/diff/20001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:122:
if (CommandLine.getInstance().hasSwitch("trace-startup")) {
On 2015/01/27 15:36:02, pasko wrote:
> FYI: we are going to have global tracing state in the system, so this piece
> would check for a special file on the device

Acknowledged.

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:15:
import org.chromium.base.TraceEvent;
On 2015/01/27 15:36:02, pasko wrote:
> nit: the TraceEvent.java has a comment at the top about ignoring the early
trace
> events, please remove that

Done.

https://codereview.chromium.org/874543003/diff/20001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:251:
TraceEvent.begin();
On 2015/01/27 15:36:02, pasko wrote:
> does this compile? All the instances of TraceEvent.begin() I am seeing require
a
> string parameter.

Done.

[dsinclair, 2015-02-09 21:11:20]

https://codereview.chromium.org/874543003/diff/80001/base/android/base_jni_re...
File base/android/base_jni_registrar.cc (right):

https://codereview.chromium.org/874543003/diff/80001/base/android/base_jni_re...
base/android/base_jni_registrar.cc:43: { "EarlyTraceEvent",
base::android::RegisterEarlyTraceEvent },
nit: looks like the spacing is off here relative to the other lines.

https://codereview.chromium.org/874543003/diff/80001/base/android/early_trace...
File base/android/early_trace_event.h (right):

https://codereview.chromium.org/874543003/diff/80001/base/android/early_trace...
base/android/early_trace_event.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
nit: 2015  (and other new files)

https://codereview.chromium.org/874543003/diff/80001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:1920: return AddTraceEventInternal(
Instead of adding the Internal version, just have this one call the above and
pass in ThreadNow().

https://codereview.chromium.org/874543003/diff/80001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/874543003/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:125:
TraceEvent.begin("ChromeShellActivity.onCreate");
This trace event seems unrelated to this CL, can you put it in it's own CL?

https://codereview.chromium.org/874543003/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:125:
TraceEvent.begin("ChromeShellActivity.onCreate");
This trace event seems unrelated to this CL, can you put it in it's own CL?

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
content/browser/android/tracing_controller_android.cc:19: using
base::debug::TraceLog;
I believe this has now changed to be base::trace_event::TraceLog.

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
content/browser/android/tracing_controller_android.cc:32:
TraceLog::GetInstance()->SetEnabled(
Why force the log to be enabled, is there a check we're doing on the tracing
side that this is to get past?

https://codereview.chromium.org/874543003/diff/80001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/874543003/diff/80001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:22: class TraceLog;
What is this for?

https://codereview.chromium.org/874543003/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/874543003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:251:
TraceEvent.begin("BrowserStartupController.prepareToStartBrowserProcess");
Seems unrelated to this CL, can you pull it out?

[Benoit L, 2015-02-10 15:31:35]

Thank you for the review!
It pointed out a potential race. Aside from that, I have reintroduced the "don't
report overhead" flag, this time with a comment explaining why it is necessary.

https://codereview.chromium.org/874543003/diff/80001/base/android/base_jni_re...
File base/android/base_jni_registrar.cc (right):

https://codereview.chromium.org/874543003/diff/80001/base/android/base_jni_re...
base/android/base_jni_registrar.cc:43: { "EarlyTraceEvent",
base::android::RegisterEarlyTraceEvent },
On 2015/02/09 21:11:20, dsinclair wrote:
> nit: looks like the spacing is off here relative to the other lines.

Done.

https://codereview.chromium.org/874543003/diff/80001/base/android/early_trace...
File base/android/early_trace_event.h (right):

https://codereview.chromium.org/874543003/diff/80001/base/android/early_trace...
base/android/early_trace_event.h:1: // Copyright 2014 The Chromium Authors. All
rights reserved.
On 2015/02/09 21:11:20, dsinclair wrote:
> nit: 2015  (and other new files)

Done.

https://codereview.chromium.org/874543003/diff/80001/base/trace_event/trace_e...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/80001/base/trace_event/trace_e...
base/trace_event/trace_event_impl.cc:1920: return AddTraceEventInternal(
On 2015/02/09 21:11:20, dsinclair wrote:
> Instead of adding the Internal version, just have this one call the above and
> pass in ThreadNow().

Done.

https://codereview.chromium.org/874543003/diff/80001/chrome/android/shell/jav...
File
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java
(right):

https://codereview.chromium.org/874543003/diff/80001/chrome/android/shell/jav...
chrome/android/shell/java/src/org/chromium/chrome/shell/ChromeShellActivity.java:125:
TraceEvent.begin("ChromeShellActivity.onCreate");
On 2015/02/09 21:11:20, dsinclair wrote:
> This trace event seems unrelated to this CL, can you put it in it's own CL?

I left it here as an example of use for the early tracing. I can remove it
though. wdyt ?

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
content/browser/android/tracing_controller_android.cc:19: using
base::debug::TraceLog;
On 2015/02/09 21:11:20, dsinclair wrote:
> I believe this has now changed to be base::trace_event::TraceLog.

Done.

https://codereview.chromium.org/874543003/diff/80001/content/browser/android/...
content/browser/android/tracing_controller_android.cc:32:
TraceLog::GetInstance()->SetEnabled(
On 2015/02/09 21:11:20, dsinclair wrote:
> Why force the log to be enabled, is there a check we're doing on the tracing
> side that this is to get past?

Yes, precisely, otherwise the AddTraceEventWithThreadAndTimestamp() calls would
be ignored in AddTraceEventInternal.
However, we only enable the "EarlyJava" category here that is not used anywhere
else (just made the change).
Thank you for pointing this out. I've also added a comment to make it clearer.

https://codereview.chromium.org/874543003/diff/80001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.h (right):

https://codereview.chromium.org/874543003/diff/80001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.h:22: class TraceLog;
On 2015/02/09 21:11:20, dsinclair wrote:
> What is this for?

Done.

https://codereview.chromium.org/874543003/diff/80001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/874543003/diff/80001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:251:
TraceEvent.begin("BrowserStartupController.prepareToStartBrowserProcess");
On 2015/02/09 21:11:20, dsinclair wrote:
> Seems unrelated to this CL, can you pull it out?

Done.

[Benoit L, 2015-03-04 12:47:44]

Hello,

The CL has been rebased, moving to the new flag
TRACE_EVENT_FLAG_EXPLICIT_TIMESTAMP.

PTAL.

[dsinclair, 2015-03-16 14:41:23]

There is a lot of stuff going on in here, can we get some tests around this
code?

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right):

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/EarlyTraceEvent.java:170: assert false;
Should this be removed?

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/TraceEvent.java (right):

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/TraceEvent.java:306: // This was
measured to take about 1ms on Trygon device.
nit: No idea what Trygon is ....

And, what was 1ms, the return or having the commented out lines enabled?

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
base/trace_event/trace_event_impl.cc:1961: TimeTicks thread_now = ThreadNow();
Can this be removed and we just use thread_time?

https://codereview.chromium.org/874543003/diff/120001/content/browser/android...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/874543003/diff/120001/content/browser/android...
content/browser/android/tracing_controller_android.cc:63: phases[i].phase,
category_group_enabled, trace_event.name.c_str(),
All of these are going to end up with the category of EarlyJava. Do we have a
need for them to have other categories? Or is EarlyJava fine?

[Benoit L, 2015-03-19 16:50:47]

Hello,

Thank you for the review. Sorry about the large diff.

I have added Java tests to the code, removed some code, and clarified comments.

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/EarlyTraceEvent.java (right):

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/EarlyTraceEvent.java:170: assert false;
On 2015/03/16 14:41:23, dsinclair wrote:
> Should this be removed?

Done.

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/TraceEvent.java (right):

https://codereview.chromium.org/874543003/diff/120001/base/android/java/src/o...
base/android/java/src/org/chromium/base/TraceEvent.java:306: // This was
measured to take about 1ms on Trygon device.
On 2015/03/16 14:41:23, dsinclair wrote:
> nit: No idea what Trygon is ....
> 
> And, what was 1ms, the return or having the commented out lines enabled?

This code is not actually required, so it's gone.

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
base/trace_event/trace_event_impl.cc:1961: TimeTicks thread_now = ThreadNow();
On 2015/03/16 14:41:23, dsinclair wrote:
> Can this be removed and we just use thread_time?

I'm not sure that I understand what you mean here. This is not part of this CL,
it was added for something else.

https://codereview.chromium.org/874543003/diff/120001/content/browser/android...
File content/browser/android/tracing_controller_android.cc (right):

https://codereview.chromium.org/874543003/diff/120001/content/browser/android...
content/browser/android/tracing_controller_android.cc:63: phases[i].phase,
category_group_enabled, trace_event.name.c_str(),
On 2015/03/16 14:41:23, dsinclair wrote:
> All of these are going to end up with the category of EarlyJava. Do we have a
> need for them to have other categories? Or is EarlyJava fine?

I think that EarlyJava is fine. I have added a comment to clarify.

[dsinclair, 2015-03-19 18:35:32]

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/874543003/diff/120001/base/trace_event/trace_...
base/trace_event/trace_event_impl.cc:1961: TimeTicks thread_now = ThreadNow();
On 2015/03/19 16:50:47, lizeb wrote:
> On 2015/03/16 14:41:23, dsinclair wrote:
> > Can this be removed and we just use thread_time?
> 
> I'm not sure that I understand what you mean here. This is not part of this
CL,
> it was added for something else.


Right, but we've added another TimeTicks called thread_time which you use below.
It seems like this TimeTicks is redundant?

[dsinclair, 2015-08-05 18:09:24]

Is this CL still in the works?

[dsinclair, 2015-08-14 14:35:09]

dsinclair@chromium.org changed reviewers:
- dsinclair@chromium.org

[Benoit L, 2015-08-18 11:34:34]

On 2015/08/14 14:35:09, dsinclair wrote:

Currently not, but we may want to revisit later.