|
|
Created:
3 years, 7 months ago by kapishnikov Modified:
3 years, 7 months ago CC:
mef, chromium-reviews, net-reviews_chromium.org, danakj+watch_chromium.org, mac-reviews_chromium.org, cbentzel+watch_chromium.org, vmpstr+watch_chromium.org, Lei Zhang Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse faster clock_gettime() instead of sysctl() to get current ticks on iOS10
BUG=724546
Review-Url: https://codereview.chromium.org/2894173002
Cr-Commit-Position: refs/heads/master@{#474336}
Committed: https://chromium.googlesource.com/chromium/src/+/5dc9bafa3dcdc44f93757689ccff9629e8d085cf
Patch Set 1 #Patch Set 2 : Remove unused includes #
Total comments: 11
Patch Set 3 : Addressed Yuri's comments. #Patch Set 4 : Change CLOCK_MONOTONIC_RAW back to CLOCK_MONOTONIC. #
Total comments: 4
Patch Set 5 : Changed DCHECK to DCHECK_GE #Patch Set 6 : Small fix #Patch Set 7 : Fixed bug on 32-bit platforms #Messages
Total messages: 40 (25 generated)
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
kapishnikov@chromium.org changed reviewers: + ianswett@google.com, rch@chromium.org
This CL is the continuation of Ryan's work: https://codereview.chromium.org/2871573009/. We have observed that replacing sysctl() calls with calls to clock_gettime() results in significant reduction of CPU usage. We can use this CL to discuss the opportunity of applying it to base:: for even greater impact.
kapishnikov@chromium.org changed reviewers: + miu@chromium.org
Yuri, could you please take a look at the change in base/time/time_mac.cc? Thanks!
ianswett@chromium.org changed reviewers: + ianswett@chromium.org
lgtm https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:78: struct timeval boottime; I didn't realize we were already doing something different for iOS than other platforms. Should we consider reaching out to Apple for suggestions of monotonic clocks to use prior to iOS 10?
On 2017/05/19 19:22:58, ianswett wrote: > lgtm > > https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc > File base/time/time_mac.cc (right): > > https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... > base/time/time_mac.cc:78: struct timeval boottime; > I didn't realize we were already doing something different for iOS than other > platforms. Should we consider reaching out to Apple for suggestions of > monotonic clocks to use prior to iOS 10? That would be great. As far as I could find, the only way to do it on versions prior to iOS10 is to use sysctl() but maybe there is some other way.
On 2017/05/19 19:28:43, kapishnikov wrote: > On 2017/05/19 19:22:58, ianswett wrote: > > lgtm > > > > https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc > > File base/time/time_mac.cc (right): > > > > > https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... > > base/time/time_mac.cc:78: struct timeval boottime; > > I didn't realize we were already doing something different for iOS than other > > platforms. Should we consider reaching out to Apple for suggestions of > > monotonic clocks to use prior to iOS 10? > > That would be great. As far as I could find, the only way to do it on versions > prior to iOS10 is to use sysctl() but maybe there is some other way. According to https://david-smith.org/iosversionstats/, this change should improve performance on 86% of supported devices (assuming that only iOS 9 & iOS 10 are supported).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
[+cc thestig@, since they've been looking at ToJavaTime() abuse] Comments on PS2: https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:60: // Returns monotonically growing number of ticks in milliseconds since some s/milliseconds/microseconds/ https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:69: if (clock_gettime(CLOCK_MONOTONIC, &tp) == 0) { Hmm...Curious that the POSIX compatibility layer would be faster than a more-direct syscall interface. What interface does the POSIX library use? Maybe we should use that directly? Or, does the POSIX library use some kind of special CPU ticks + periodic reconciliation logic to avoid querying the system clock? https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:91: return MachAbsoluteTimeToTicks(mach_absolute_time()); Also curious about this: Why don't we just call mach_absolute_time() on iOS? Does this system clock have different behavior on iOS that Chromium cannot tolerate for some reason? https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... File net/quic/platform/impl/quic_chromium_clock.cc (right): https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:26: return CreateTimeFromMicroseconds(base::TimeTicks::Now().ToInternalValue()); Please fix: This is invalid use of ToInternalValue() (and ToInternalValue() is deprecated). If you want an arbitrary tick count in a microsecond timebase, you should make this: return CreateTimeFromMicroseconds( (base::TimeTicks::Now() - base::TimeTicks()).InMicroseconds()); More info: http://crbug.com/634507 https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:30: return QuicWallTime::FromUNIXMicroseconds(base::Time::Now().ToJavaTime() * Please fix: You are using a platform conversion function where there is no platform converting being done. This should be something like: const base::TimeDelta time_since_unix_epoch = base::Time::Now() - base::Time::UnixEpoch(); return QuicWallTime::FromUNIXMicroseconds( time_since_unix_epoch.InMicroseconds()); Note: The existing code here always truncates to millisecond precision, which is probably not a good idea; but I don't fully understand the use case of this function, so I leave it up to you to decide whether that's a good idea. If so, please use TimeDelta's operator%() to subtract-out the sub-millisecond component. Note 2: https://codereview.chromium.org/2892553005/ More info: Please see discussion in https://codereview.chromium.org/2870223003/. It's on me (on my TODO list) to take discussions like these and put together a "best practices" document. ;)
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Yuri, thanks for the review. Please see the answers/comments below. https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:60: // Returns monotonically growing number of ticks in milliseconds since some On 2017/05/22 22:16:14, miu wrote: > s/milliseconds/microseconds/ Done. https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:69: if (clock_gettime(CLOCK_MONOTONIC, &tp) == 0) { On 2017/05/22 22:16:14, miu wrote: > Hmm...Curious that the POSIX compatibility layer would be faster than a > more-direct syscall interface. What interface does the POSIX library use? Maybe > we should use that directly? Or, does the POSIX library use some kind of special > CPU ticks + periodic reconciliation logic to avoid querying the system clock? I couldn't find the sources but my guess is that it used CPU ticks + adjustments for the time the device was in sleep. The implementation may use some calls which are not allowed to be called directly by apps. BTW, I found that iOS 10 also supports CLOCK_MONOTONIC_RAW clock type, which I think is a better candidate than CLOCK_MONOTONIC, since CLOCK_MONOTONIC frequency may be impacted by NTP time adjustments. So, I changed it to CLOCK_MONOTONIC_RAW. https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#n... base/time/time_mac.cc:91: return MachAbsoluteTimeToTicks(mach_absolute_time()); On 2017/05/22 22:16:14, miu wrote: > Also curious about this: Why don't we just call mach_absolute_time() on iOS? > Does this system clock have different behavior on iOS that Chromium cannot > tolerate for some reason? mach_absolute_time() stops counting when an iOS device is in the sleep mode. It really depends on the contract of TimeTicks::Now() function. My assumption is that some clients of the function would like to account for the 'sleep' time. https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... File net/quic/platform/impl/quic_chromium_clock.cc (right): https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:26: return CreateTimeFromMicroseconds(base::TimeTicks::Now().ToInternalValue()); On 2017/05/22 22:16:14, miu wrote: > Please fix: This is invalid use of ToInternalValue() (and ToInternalValue() is > deprecated). If you want an arbitrary tick count in a microsecond timebase, you > should make this: > > return CreateTimeFromMicroseconds( > (base::TimeTicks::Now() - base::TimeTicks()).InMicroseconds()); > > More info: http://crbug.com/634507 Done. https://codereview.chromium.org/2894173002/diff/20001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:30: return QuicWallTime::FromUNIXMicroseconds(base::Time::Now().ToJavaTime() * On 2017/05/22 22:16:14, miu wrote: > Please fix: You are using a platform conversion function where there is no > platform converting being done. This should be something like: > > const base::TimeDelta time_since_unix_epoch = > base::Time::Now() - base::Time::UnixEpoch(); > return QuicWallTime::FromUNIXMicroseconds( > time_since_unix_epoch.InMicroseconds()); > > Note: The existing code here always truncates to millisecond precision, which is > probably not a good idea; but I don't fully understand the use case of this > function, so I leave it up to you to decide whether that's a good idea. If so, > please use TimeDelta's operator%() to subtract-out the sub-millisecond > component. > > Note 2: https://codereview.chromium.org/2892553005/ > > More info: Please see discussion in https://codereview.chromium.org/2870223003/. > It's on me (on my TODO list) to take discussions like these and put together a > "best practices" document. ;) Done. I think it was truncated to milliseconds because the wall-time in Java is measured in milliseconds since January 1, 1970 UTC. I don't think truncating makes sense.
I found this CL https://codereview.chromium.org/2172483002/ and switched CLOCK_MONOTONIC_RAW back to CLOCK_MONOTONIC.
lgtm Looks like you already found the reason we don't use CLOCK_MONOTONIC_RAW. :)
net/quic/ LGTM, mod two nits. https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... File net/quic/platform/impl/quic_chromium_clock.cc (right): https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:27: DCHECK(ticks >= 0); nit: DCHECK_LE(0, ticks); https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:34: DCHECK(time_since_unix_epoch.InMicroseconds() >= 0); nit: DCHECK_LE(0, time_since_unix_epoch.InMicroseconds()); (or perhaps save InMicroseconds() to a local variable to use in both the DCHECK and the FromUNIXMicroseconds() call.)
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Thanks! https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... File net/quic/platform/impl/quic_chromium_clock.cc (right): https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:27: DCHECK(ticks >= 0); On 2017/05/23 19:16:52, Ryan Hamilton wrote: > nit: DCHECK_LE(0, ticks); Done. Used DCHECK_GE instead of DCHECK_LE. https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/... net/quic/platform/impl/quic_chromium_clock.cc:34: DCHECK(time_since_unix_epoch.InMicroseconds() >= 0); On 2017/05/23 19:16:52, Ryan Hamilton wrote: > nit: DCHECK_LE(0, time_since_unix_epoch.InMicroseconds()); > > (or perhaps save InMicroseconds() to a local variable to use in both the DCHECK > and the FromUNIXMicroseconds() call.) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The base tests are consistently failing on 32-bit device simulators (e.g. iPhone 5) but passing on 64-bit ones (e.g. iPhone 6/7). I will investigate it.
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The tests were failing on 32-bit because the structure member "timespec.tv_spec" was declared as long. The size of the long is different on 32 & 64 bits. As the result the multiplication of tp.tv_sec by 1000000 caused the overflow on 32 bits. It is worrisome that the tests didn't detect it when the same change was made in QUIC.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ianswett@chromium.org, rch@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2894173002/#ps120001 (title: "Fixed bug on 32-bit platforms")
Description was changed from ========== Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10 BUG=724546 ========== to ========== Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10 BUG=724546 ==========
kapishnikov@chromium.org changed reviewers: - ianswett@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495646351122600, "parent_rev": "1f04178ffc2984344d254363261c8dc9a38265de", "commit_rev": "5dc9bafa3dcdc44f93757689ccff9629e8d085cf"}
Message was sent while issue was closed.
Description was changed from ========== Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10 BUG=724546 ========== to ========== Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10 BUG=724546 Review-Url: https://codereview.chromium.org/2894173002 Cr-Commit-Position: refs/heads/master@{#474336} Committed: https://chromium.googlesource.com/chromium/src/+/5dc9bafa3dcdc44f93757689ccff... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5dc9bafa3dcdc44f93757689ccff... |