Several Tick counter improvements.

Several Tick counter improvements.

Move logic into cc file
Simplify interval calculation
Remove unused QUERY_PERFORMANCE_COUNTER windows implementation
Remove double divide on each ::Now() invocation on mac

Move TickTime and TickInterval funcitons to cc file in prep for refactoring.

BUG=
R=mflodman@webrtc.org, pbos@webrtc.org

Committed: https://crrev.com/4c27e4b62da2047063d88eedfeec3e939fea7843
Cr-Commit-Position: refs/heads/master@{#10661}

[Tim H, 2015-10-28 23:04:35]

Description was changed from

==========
fix presubmit script path


Test presubmit stuff


Several Tick counter improvements.

Move logic into cc file
Simplify interval calculation
Remove unused QUERY_PERFORMANCE_COUNTER windows implementation
Remove double divide on each ::Now() invocation on mac

Move TickTime and TickInterval funcitons to cc file in prep for refactoring.

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

to

==========
Several Tick counter improvements.

Move logic into cc file
Simplify interval calculation
Remove unused QUERY_PERFORMANCE_COUNTER windows implementation
Remove double divide on each ::Now() invocation on mac

Move TickTime and TickInterval funcitons to cc file in prep for refactoring.

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

[Tim H, 2015-10-28 23:05:37]

I'll of course revert the tools change but needed that to get around some
presubmit errors.

[pbos-webrtc, 2015-11-03 10:39:31]

pbos@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-11-03 10:39:32]

(Didn't check tools ofc.)

Looks good overall, this code has some issues w/r/t thread safety. Can you
elaborate more on what your refactoring plans are?

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
File webrtc/system_wrappers/source/tick_util.cc (right):

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:35: if
(timebase_from_millisecond_fract == 0.0) {
Racy, put a TODO please.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:46: int64_t
TickTime::TicksToMilliseconds(const int64_t ticks) {
Return TicksToMicroseconds() / 1000; maybe? I don't think that is that-huge a
performance loss.

Or refactor into TicksToFractionsOfSecond(ticks, 1000) and (ticks, 1000000) if
you want to avoid the double divide, but that sounds more confusing.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:52: static double
timebase_millisecond_fract = 0.0;
Racy, put a TODO please.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:70: static double
timebase_microsecond_fract = 0.0;
Racy, put a TODO please.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:112: num_wrap_time_get_time++;
This is racy, >1 thread can enter here. Can you put a TODO and file a bug for me
to investigate/fix? Or are you aiming to remove TickTime and only use Clock or
smth?

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:132: if (timebase_start == 0) {
This is racy, can you put a TODO here (since it was also racy before)? This
should possibly be a CAS and atomic read.

[Tim H, 2015-11-03 23:32:36]

Thanks for the feedback!

On 2015/11/03 10:39:32, pbos-webrtc wrote:
> (Didn't check tools ofc.)
> 
> Looks good overall, this code has some issues w/r/t thread safety. Can you
> elaborate more on what your refactoring plans are?
> 

I should have mentioned, this is an upstream of a local change we made in
another app.  We noticed from profiling that we were spending an unexpectedly
large amount of time in the timekeeping functions, on the order of 2% total cpu
spend.  Which is pretty crazy when you consider we're doing all the a/v encode
and decode in software at the moment.  I don't plan on doing any further
refactoring of the area.

I'll look at adding the todos and potentially the tickstomillis change later
this week.


>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> File webrtc/system_wrappers/source/tick_util.cc (right):
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:35: if
> (timebase_from_millisecond_fract == 0.0) {
> Racy, put a TODO please.
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:46: int64_t
> TickTime::TicksToMilliseconds(const int64_t ticks) {
> Return TicksToMicroseconds() / 1000; maybe? I don't think that is that-huge a
> performance loss.
> 
> Or refactor into TicksToFractionsOfSecond(ticks, 1000) and (ticks, 1000000) if
> you want to avoid the double divide, but that sounds more confusing.
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:52: static double
> timebase_millisecond_fract = 0.0;
> Racy, put a TODO please.
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:70: static double
> timebase_microsecond_fract = 0.0;
> Racy, put a TODO please.
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:112: num_wrap_time_get_time++;
> This is racy, >1 thread can enter here. Can you put a TODO and file a bug for
me
> to investigate/fix? Or are you aiming to remove TickTime and only use Clock or
> smth?
> 
>
https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:132: if (timebase_start == 0) {
> This is racy, can you put a TODO here (since it was also racy before)? This
> should possibly be a CAS and atomic read.

[Tim H, 2015-11-04 22:20:52]

In talking with Noah the issue with the millis function actually is with integer
divide too.  It's the fact that it's a 64 bit divide not that it's a double
specifically that was causing the slowness, so I'll put that back shortly.  I
think two similar explicit functions are superior to a TicksToFractions just on
readability grounds.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
File webrtc/system_wrappers/source/tick_util.cc (right):

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:35: if
(timebase_from_millisecond_fract == 0.0) {
On 2015/11/03 10:39:31, pbos-webrtc wrote:
> Racy, put a TODO please.

Done.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:46: int64_t
TickTime::TicksToMilliseconds(const int64_t ticks) {
On 2015/11/03 10:39:31, pbos-webrtc wrote:
> Return TicksToMicroseconds() / 1000; maybe? I don't think that is that-huge a
> performance loss.
> 
> Or refactor into TicksToFractionsOfSecond(ticks, 1000) and (ticks, 1000000) if
> you want to avoid the double divide, but that sounds more confusing.

Your issue is with the similarity of the two functions?  I think an integer
divide is probably ok too, but I haven't tested.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:52: static double
timebase_millisecond_fract = 0.0;
On 2015/11/03 10:39:31, pbos-webrtc wrote:
> Racy, put a TODO please.

Done.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:112: num_wrap_time_get_time++;
On 2015/11/03 10:39:31, pbos-webrtc wrote:
> This is racy, >1 thread can enter here. Can you put a TODO and file a bug for
me
> to investigate/fix? Or are you aiming to remove TickTime and only use Clock or
> smth?

Done.

https://codereview.webrtc.org/1415923010/diff/1/webrtc/system_wrappers/source...
webrtc/system_wrappers/source/tick_util.cc:132: if (timebase_start == 0) {
On 2015/11/03 10:39:31, pbos-webrtc wrote:
> This is racy, can you put a TODO here (since it was also racy before)? This
> should possibly be a CAS and atomic read.

Done.

[pbos-webrtc, 2015-11-05 17:30:00]

let me know when that's ready, please put this TODO at all spots where the CAS
statement was.

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/tick_util.cc (right):

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/tick_util.cc:51: return ticks/ 1000000LL;
style: spaces around "/"

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/tick_util.cc:53: // TODO(pbos): Static local vars
are not thread safe, consider atomic CAS.
They are in C++, just not this use of them (= 0, if (==0, set)

Put // TODO(pbos): Fix unsafe use of static locals.

That should be generic enough.

[Tim H, 2015-11-05 19:08:24]

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/tick_util.cc (right):

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/tick_util.cc:51: return ticks/ 1000000LL;
On 2015/11/05 17:30:00, pbos-webrtc wrote:
> style: spaces around "/"

Done.

https://codereview.webrtc.org/1415923010/diff/40001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/tick_util.cc:53: // TODO(pbos): Static local vars
are not thread safe, consider atomic CAS.
On 2015/11/05 17:30:00, pbos-webrtc wrote:
> They are in C++, just not this use of them (= 0, if (==0, set)
> 
> Put // TODO(pbos): Fix unsafe use of static locals.
> 
> That should be generic enough.

Acknowledged.

[pbos-webrtc, 2015-11-05 19:20:40]

lgtm assuming tools/ revert, no idea why you have to do that one

[mflodman, 2015-11-10 13:10:10]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2015-11-10 13:10:11]

LGTM

[Tim H, 2015-11-11 22:39:19]

On 2015/11/10 13:10:11, mflodman wrote:
> LGTM

Made a small unittest fixup and confirmed it passes all trybots now.  Will
submit tomorrow.

[pbos-webrtc, 2015-11-12 11:55:41]

please fix without the sleep, the tests shouldn't assume >0 if >0 isn't the case

https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/process_thread_impl_unittest.cc (right):

https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/process_thread_impl_unittest.cc:254: int64_t
start_time = 0;
Shouldn't you initialize these to -1 and update the asserts below instead (that
it's > -1)?

[noahric, 2015-11-12 18:10:15]

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

[noahric, 2015-11-12 18:10:15]

https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
File webrtc/modules/utility/source/process_thread_impl_unittest.cc (right):

https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
webrtc/modules/utility/source/process_thread_impl_unittest.cc:254: int64_t
start_time = 0;
On 2015/11/12 11:55:41, pbos-webrtc wrote:
> Shouldn't you initialize these to -1 and update the asserts below instead
(that
> it's > -1)?

I think we should actually just the startTime != something check;
MillisecondTimestamp and family make no claims about the absolute values of the
value they return, so this could legally return anything for the first value,
including -1. That's what I exploited when writing this originally (deltas from
zero mean small values, so loss of precision is acceptable over certain
timeframes).

[Tim H, 2015-11-12 19:38:20]

On 2015/11/12 18:10:15, noahric wrote:
>
https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
> File webrtc/modules/utility/source/process_thread_impl_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1415923010/diff/120001/webrtc/modules/utility/s...
> webrtc/modules/utility/source/process_thread_impl_unittest.cc:254: int64_t
> start_time = 0;
> On 2015/11/12 11:55:41, pbos-webrtc wrote:
> > Shouldn't you initialize these to -1 and update the asserts below instead
> (that
> > it's > -1)?
> 
> I think we should actually just the startTime != something check;
> MillisecondTimestamp and family make no claims about the absolute values of
the
> value they return, so this could legally return anything for the first value,
> including -1. That's what I exploited when writing this originally (deltas
from
> zero mean small values, so loss of precision is acceptable over certain
> timeframes).

It feels like you left the word delete out from your comment Noah.  I've updated
the test again to explicitly leave the timestamps uninitialized and only check
their relative not absolute values.

[Tim H, 2015-11-16 22:38:03]

Committed patchset #8 (id:140001) manually as
4c27e4b62da2047063d88eedfeec3e939fea7843 (presubmit successful).

[commit-bot: I haz the power, 2015-11-16 22:38:08]

Patchset 8 (id:??) landed as
https://crrev.com/4c27e4b62da2047063d88eedfeec3e939fea7843
Cr-Commit-Position: refs/heads/master@{#10661}

[thaloun, 2015-11-17 00:27:07]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.webrtc.org/1450203002/ by thaloun@chromium.org.

The reason for reverting is: Potentially breaks a threading test under DrMemory.
 Rolling back while I investigate..