Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639543005:
Use rtc::time for all your timing needs! (Closed)
|
Created:
4 years, 11 months ago by sprang_webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, henrika_webrtc, zhengzhonghou_agora.io, tterriberry_mozilla.com, fengyue_agora.io, peah-webrtc, mflodman, sophiechang-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse rtc::time for all your timing needs!
Initial step of unifying so that base/timeutils.h and Clock/TimeTime
from system_wrappers use the same implementation.
BUG=webrtc:5463
R=pbos@webrtc.org, tommi@webrtc.org
Committed: https://crrev.com/1c3909899d2bc1e46b8b40f6660f30a8a280f1f4
Cr-Commit-Position: refs/heads/master@{#11394}
Patch Set 1 #Patch Set 2 : Format #
Total comments: 5
Patch Set 3 : Error on unsupported platform #
Messages
Total messages: 17 (8 generated)
Description was changed from ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG= ========== to ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG=webrtc:5463 ==========
sprang@webrtc.org changed reviewers: + pbos@webrtc.org, tommi@webrtc.org
lgtm with one question https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:77: #else #else #error? Do we ever build with none of these set?
lgtm with a topic to discuss, but feel free to land. I think it's very good that you're moving this code from system_wrappers to base/ btw. https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:49: ticks = mach_absolute_time() * timebase.numer / timebase.denom; since we're here - can we do something about this so that it behaves more like the implementations on other operating systems? At the moment it always returns a very low value on first call, regardless of when it's called. It also looks like we're using static initialization but could use atomics instead. No action required in this CL but curious to hear what you think. I realized this btw when I saw that we're using the return value from this function as a seed to Random() (via the Clock class).
https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:49: ticks = mach_absolute_time() * timebase.numer / timebase.denom; On 2016/01/26 23:23:46, tommi-webrtc wrote: > since we're here - can we do something about this so that it behaves more like > the implementations on other operating systems? At the moment it always returns > a very low value on first call, regardless of when it's called. > > It also looks like we're using static initialization but could use atomics > instead. > > No action required in this CL but curious to hear what you think. I realized > this btw when I saw that we're using the return value from this function as a > seed to Random() (via the Clock class). Are your sure "low initial value" applies here? The root cause for doing this refactoring is that the mac implementation of TickTime::QueryOsForTicks() behaves differently from this one; it used a static variable as offset and initialized it so that the first call would always return 0... Agreed we should do something about static initialization here. https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:77: #else On 2016/01/26 15:00:22, pbos-webrtc wrote: > #else #error? Do we ever build with none of these set? Agreed it makes more sense to have #error here instead.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org, pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/1639543005/#ps40001 (title: "Error on unsupported platform")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639543005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639543005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Description was changed from ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG=webrtc:5463 ========== to ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG=webrtc:5463 R=pbos@webrtc.org, tommi@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/1c3909899d2bc1e46b8b40f66... ==========
Message was sent while issue was closed.
Description was changed from ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG=webrtc:5463 ========== to ========== Use rtc::time for all your timing needs! Initial step of unifying so that base/timeutils.h and Clock/TimeTime from system_wrappers use the same implementation. BUG=webrtc:5463 R=pbos@webrtc.org, tommi@webrtc.org Committed: https://crrev.com/1c3909899d2bc1e46b8b40f6660f30a8a280f1f4 Cr-Commit-Position: refs/heads/master@{#11394} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1c3909899d2bc1e46b8b40f6660f30a8a280f1f4 Cr-Commit-Position: refs/heads/master@{#11394}
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as 1c3909899d2bc1e46b8b40f6660f30a8a280f1f4 (presubmit successful).
Message was sent while issue was closed.
noahric@chromium.org changed reviewers: + noahric@chromium.org
Message was sent while issue was closed.
+sophie, as FYI. https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc File webrtc/base/timeutils.cc (right): https://codereview.webrtc.org/1639543005/diff/20001/webrtc/base/timeutils.cc#... webrtc/base/timeutils.cc:49: ticks = mach_absolute_time() * timebase.numer / timebase.denom; On 2016/01/27 08:48:47, språng wrote: > On 2016/01/26 23:23:46, tommi-webrtc wrote: > > since we're here - can we do something about this so that it behaves more like > > the implementations on other operating systems? At the moment it always > returns > > a very low value on first call, regardless of when it's called. > > > > It also looks like we're using static initialization but could use atomics > > instead. > > > > No action required in this CL but curious to hear what you think. I realized > > this btw when I saw that we're using the return value from this function as a > > seed to Random() (via the Clock class). > > Are your sure "low initial value" applies here? The root cause for doing this > refactoring is that the mac implementation of TickTime::QueryOsForTicks() > behaves differently from this one; it used a static variable as offset and > initialized it so that the first call would always return 0... > > Agreed we should do something about static initialization here. FYI, the reason it was different was: https://codereview.webrtc.org/1452843003 Which was for a 2% overall cpu usage reduction on iOS. The other issues are definitely real (it purposefully picked low values so the double would maintain precision for awhile, but that meant things that used raw tick values for random seeding were broken, as tommi discovered), so I wouldn't just put it back in without fixing that. But it'd be sad to lose the 2% cpu improvement for nothing. Not too many other things are so simple to fix :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
