Adding support for Time and TimeTicks inside trace_event.

Adding support for Time and TimeTicks inside trace_event.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257934

[mithro-old, 2014-03-18 05:35:35]

Hi Nat,

This CL adds support for using Time/TimeTicks in a TRACE_EVENT without having to
convert it yourself.

I feel that base::Time / base::TimeTicks are fundamental enough types that they
should be able to be used with TRACE without needing to think. Getting the right
thing without thinking is awesome!

So far this was the best solution I came up with;
 * We probably don't want Time/TimeTicks to inherit from
ConvertableToTraceFormat
 * If we added a "ToConvertableTraceFormat()" to Time/TimeTicks then it is the
equivalent to using ToInternalValue() anyway (but at least it is clear what to
use).

I also wonder if it would be useful to tag the output with more information (I
can imagine clock source would be very useful) but that would need the
ToConvertableTraceFormat part.
 {'type': 'time', value: 123455, 'clock': 'system', 'base': 'microseconds',
'human': '2012-10-12 12:24:00.12345 UTC'}
 {'type': 'ticks', value: 123455, 'clock': 'monotonic', 'base': 'microseconds'}

Thoughts? I wasn't able to come up a good regex to find any current usage of
base::Time or base::TimeTicks with TRACE, so maybe nobody else has ever wanted
to do this?

Tim

[nduca, 2014-03-18 13:02:50]

Yeah nobody's ever done this. I'm okay with it landing the patch as-is [lgtm].
I'd like to avoid much more if possible except via ConvertableToInt --- wanna
keep the surface area of trace_event/impl as minimal as possible. Maybe someday
if we had a base TimeTicks that knew more about its timebase, we could improve
on the serialization...

[dsinclair, 2014-03-18 13:13:34]

On 2014/03/18 05:35:35, mithro wrote:
> Hi Nat,
> 
> This CL adds support for using Time/TimeTicks in a TRACE_EVENT without having
to
> convert it yourself.
> 
> I feel that base::Time / base::TimeTicks are fundamental enough types that
they
> should be able to be used with TRACE without needing to think. Getting the
right
> thing without thinking is awesome!
> 
> So far this was the best solution I came up with;
>  * We probably don't want Time/TimeTicks to inherit from
> ConvertableToTraceFormat
>  * If we added a "ToConvertableTraceFormat()" to Time/TimeTicks then it is the
> equivalent to using ToInternalValue() anyway (but at least it is clear what to
> use).
> 
> I also wonder if it would be useful to tag the output with more information (I
> can imagine clock source would be very useful) but that would need the
> ToConvertableTraceFormat part.
>  {'type': 'time', value: 123455, 'clock': 'system', 'base': 'microseconds',
> 'human': '2012-10-12 12:24:00.12345 UTC'}
>  {'type': 'ticks', value: 123455, 'clock': 'monotonic', 'base':
'microseconds'}
> 
> Thoughts? I wasn't able to come up a good regex to find any current usage of
> base::Time or base::TimeTicks with TRACE, so maybe nobody else has ever wanted
> to do this?
> 
> Tim

Are you planning on committing trace events that use this or is it just a
speculative addition that people may want?

If it's the first, it's fine with me, but if it's just speculative I'd prefer to
leave it out until we have trace events that want to use it.

[mithro-old, 2014-03-18 16:35:38]

Yes, definitely planning on committing trace events that use this. Felt
this was better as a separate CL (which also caused me to add tests :).

Tim


On 19 March 2014 00:13, <dsinclair@chromium.org> wrote:

> On 2014/03/18 05:35:35, mithro wrote:
>
>> Hi Nat,
>>
>
>  This CL adds support for using Time/TimeTicks in a TRACE_EVENT without
>> having
>>
> to
>
>> convert it yourself.
>>
>
>  I feel that base::Time / base::TimeTicks are fundamental enough types that
>>
> they
>
>> should be able to be used with TRACE without needing to think. Getting the
>>
> right
>
>> thing without thinking is awesome!
>>
>
>  So far this was the best solution I came up with;
>>   * We probably don't want Time/TimeTicks to inherit from
>> ConvertableToTraceFormat
>>   * If we added a "ToConvertableTraceFormat()" to Time/TimeTicks then it
>> is the
>> equivalent to using ToInternalValue() anyway (but at least it is clear
>> what to
>> use).
>>
>
>  I also wonder if it would be useful to tag the output with more
>> information (I
>> can imagine clock source would be very useful) but that would need the
>> ToConvertableTraceFormat part.
>>   {'type': 'time', value: 123455, 'clock': 'system', 'base':
>> 'microseconds',
>> 'human': '2012-10-12 12:24:00.12345 UTC'}
>>   {'type': 'ticks', value: 123455, 'clock': 'monotonic', 'base':
>>
> 'microseconds'}
>
>  Thoughts? I wasn't able to come up a good regex to find any current usage
>> of
>> base::Time or base::TimeTicks with TRACE, so maybe nobody else has ever
>> wanted
>> to do this?
>>
>
>  Tim
>>
>
> Are you planning on committing trace events that use this or is it just a
> speculative addition that people may want?
>
> If it's the first, it's fine with me, but if it's just speculative I'd
> prefer to
> leave it out until we have trace events that want to use it.
>
> https://codereview.chromium.org/203063002/
>

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

[dsinclair, 2014-03-18 16:56:50]

On 2014/03/18 16:35:38, mithro wrote:
> Yes, definitely planning on committing trace events that use this. Felt
> this was better as a separate CL (which also caused me to add tests :).
> 

Awesome, definitely better as a separate CL. LGTM.

[mithro-old, 2014-03-19 01:32:53]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-03-19 01:34:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mithro@mithis.com/203063002/1

[commit-bot: I haz the power, 2014-03-19 11:41:51]

Change committed as 257934