Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(63)

Issue 1452843003: Several Tick counter improvements try #2." (Closed)

Created:
3 years, 10 months ago by Tim H
Modified:
3 years, 7 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, Andrew MacDonald, peah-webrtc, mflodman, henrika_webrtc
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Several Tick counter improvements try #2." This reverts commit c91d1738709b038fee84d569180cba2bbcbfe5d7. BUG= Committed: https://crrev.com/2935e0141939adc642d73da6d8048a4e918a8382 Cr-Commit-Position: refs/heads/master@{#10682}

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -143 lines) Patch
M webrtc/modules/utility/source/process_thread_impl_unittest.cc View 2 chunks +3 lines, -4 lines 0 comments Download
M webrtc/system_wrappers/include/tick_util.h View 4 chunks +10 lines, -104 lines 0 comments Download
M webrtc/system_wrappers/source/tick_util.cc View 4 chunks +85 lines, -35 lines 1 comment Download

Messages

Total messages: 13 (5 generated)
noahric
lgtm
3 years, 10 months ago (2015-11-17 19:34:59 UTC) #2
pbos-webrtc
On 2015/11/17 19:34:59, noahric wrote: > lgtm lgtm but please link to the CL you're ...
3 years, 10 months ago (2015-11-17 21:35:34 UTC) #3
mflodman
LGTM
3 years, 10 months ago (2015-11-17 21:45:59 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1452843003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1452843003/1
3 years, 10 months ago (2015-11-17 22:00:16 UTC) #8
commit-bot: I haz the power
Committed patchset #1 (id:1)
3 years, 10 months ago (2015-11-17 23:02:55 UTC) #9
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/2935e0141939adc642d73da6d8048a4e918a8382 Cr-Commit-Position: refs/heads/master@{#10682}
3 years, 10 months ago (2015-11-17 23:03:08 UTC) #10
tommi
https://codereview.webrtc.org/1452843003/diff/1/webrtc/system_wrappers/source/tick_util.cc File webrtc/system_wrappers/source/tick_util.cc (right): https://codereview.webrtc.org/1452843003/diff/1/webrtc/system_wrappers/source/tick_util.cc#newcode140 webrtc/system_wrappers/source/tick_util.cc:140: return mach_absolute_time() - timebase_start; There's a bug here which ...
3 years, 7 months ago (2016-01-23 14:09:41 UTC) #12
noahric
3 years, 7 months ago (2016-01-25 21:58:48 UTC) #13
Message was sent while issue was closed.
On 2016/01/23 14:09:41, tommi-webrtc wrote:
>
https://codereview.webrtc.org/1452843003/diff/1/webrtc/system_wrappers/source...
> File webrtc/system_wrappers/source/tick_util.cc (right):
> 
>
https://codereview.webrtc.org/1452843003/diff/1/webrtc/system_wrappers/source...
> webrtc/system_wrappers/source/tick_util.cc:140: return mach_absolute_time() -
> timebase_start;
> There's a bug here which will affect cases where Random() is initialized with
a
> seed derived from this implementation.
> 
> Since the tick count can be very low, conversion over to microseconds, can
yield
> 0.  Atm Random will dcheck in debug builds due to this, but even when it
> doesn't, I think that the randomness of the seed is reduced by this
> implementation.

Agreed, I bet that will significantly impact randomness relative to the old
implementation, though the API sorta calls out that this kind of thing is
possible (" Return a timestamp in microseconds relative to some arbitrary
source"). Do you think it's sufficient to take the three places that seed a
webrtc/base/random with the time in micros function?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

We could expose an int64_t GetTicks, though it strictly speaking would have the
same issue. Ticks relative to "some arbitrary point" could pick points the same
way this implementation did for time (first time asked).

Powered by Google App Engine
This is Rietveld 408576698