Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935973002:
iOS: Cache the result of IsRunningOnIOS10OrLater() (Closed)
|
Created:
3 years, 6 months ago by kapishnikov Modified:
3 years, 6 months ago CC:
chromium-reviews, danakj+watch_chromium.org, ios-reviews_chromium.org, vmpstr+watch_chromium.org, mef Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptioniOS: Cache the result of IsRunningOnIOS10OrLater()
The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks()
invocation. Some profiling results have shown that the call may take
0.6% of the total network CPU utilization of the network thread.
BUG=724546
Review-Url: https://codereview.chromium.org/2935973002
Cr-Commit-Position: refs/heads/master@{#479692}
Committed: https://chromium.googlesource.com/chromium/src/+/71f40c3bd210c7450cbd2e171578c1d950750346
Patch Set 1 #Patch Set 2 : Moved the variable to the outer anonymous namespace #
Total comments: 5
Patch Set 3 : Added synchronization #Patch Set 4 : Reverted expiring_cache_unittest.cc changed by mistake #Patch Set 5 : Caching simplification #Messages
Total messages: 25 (8 generated)
Description was changed from ========== iOS: Cache the result of IsRunningOnIOS10OrLater() ========== to ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. ==========
kapishnikov@chromium.org changed reviewers: + eugenebut@chromium.org
PTAL
Description was changed from ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. ========== to ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. ==========
eugenebut@chromium.org changed reviewers: + rohitrao@chromium.org
+Rohit I've updated CL description to fit 72 symbols. https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm File base/ios/ios_util.mm (right): https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } g_ios_version = kUnknown; Should we cache the result of OSVersionAsArray instead? Otherwise the issue may show up again later. https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... base/ios/ios_util.mm:35: if (g_ios_version == kUnknown) { This makes the function non-thread safe, which you can fix by using dispatch_once API.
Caching this seems like a good idea if it's showing up as a hotspot. Is there a doc or a bug that contains more details about how we found this issue? I'm not sure what ComputeCurrentTicks() is, for example. https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm File base/ios/ios_util.mm (right): https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... base/ios/ios_util.mm:24: // Caches the result of IsRunningOnIOS10OrLater(). We'll be adding IsRunningOnIOS11OrLater() shortly, so it might be worth either adding this now or considering how we would add it later. For example, the ternary operation on line 37 wouldn't work. https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } g_ios_version = kUnknown; On 2017/06/14 00:59:48, Eugene But wrote: > Should we cache the result of OSVersionAsArray instead? Otherwise the issue may > show up again later. Can you clarify what issue might "show up again later"? Is the problem that some code calls IsRunningOnOrLater() directly?
https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm File base/ios/ios_util.mm (right): https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } g_ios_version = kUnknown; On 2017/06/14 01:10:13, rohitrao (ping after 24h) wrote: > On 2017/06/14 00:59:48, Eugene But wrote: > > Should we cache the result of OSVersionAsArray instead? Otherwise the issue > may > > show up again later. > > Can you clarify what issue might "show up again later"? Is the problem that > some code calls IsRunningOnOrLater() directly? Sorry, I meant IsRunningOnIOS11(12/13/etc..)OrLater may become a bottleneck in the future in some other place. So if improving performance for IsRunningOnOrLater is an option, then we should probably do that.
On 2017/06/14 00:59:48, Eugene But wrote: > +Rohit > I've updated CL description to fit 72 symbols. Thanks! > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm > File base/ios/ios_util.mm (right): > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } > g_ios_version = kUnknown; > Should we cache the result of OSVersionAsArray instead? Otherwise the issue may > show up again later. What would be the best way to do it without sacrificing the performance? I would like to avoid using a hashmap or a search tree. Since IsRunningOnIOS10OrLater() is called very often, I would prefer to keep the most optimal implementation of it. We can also implement caching of OSVersionAsArray in this or other CL. > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > base/ios/ios_util.mm:35: if (g_ios_version == kUnknown) { > This makes the function non-thread safe, which you can fix by using > dispatch_once API. What would be the worst case scenario if we don't add synchronization? My assumption was that in worst case scenario multiple threads could call IsRunningOnOrLater() and update the variable to the same value. Is there any chance that one of the threads will see a corrupted value?
On 2017/06/14 01:10:14, rohitrao (ping after 24h) wrote:
> Caching this seems like a good idea if it's showing up as a hotspot.
>
> Is there a doc or a bug that contains more details about how we found this
> issue? I'm not sure what ComputeCurrentTicks() is, for example.
We found this hotspot by profiling an app. There is a placeholder bug 724546 but
it is very generic. Here is an example of places that eventually call
IsRunningOnOrLater(). It is actually a call to IsRunningOnIOS10OrLater() in the
release version of the binary.
base::ios::IsRunningOnOrLater(int, int, int)
(anonymous namespace)::ComputeCurrentTicks()
base::TimeTicks::Now()
(anonymous namespace)::ComputeCurrentTicks()
base::TimeTicks::Now()
net::QuicChromiumClock::Now() const
net::NetworkActivityMonitor::IncrementBytesSent(unsigned long long)
net::NetworkActivityMonitor::IncrementBytesReceived(unsigned long
long)
tracked_objects::TrackedTime::Now()
net::QuicChromiumPacketWriter::WritePacketToSocket(scoped_refptr<net::StringIOBuffer>)
>
> https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm
> File base/ios/ios_util.mm (right):
>
>
https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne...
> base/ios/ios_util.mm:24: // Caches the result of IsRunningOnIOS10OrLater().
> We'll be adding IsRunningOnIOS11OrLater() shortly, so it might be worth either
> adding this now or considering how we would add it later. For example, the
> ternary operation on line 37 wouldn't work.
I agree. Maybe we should cache the result of IsRunningOnOrLater() but I would
like to keep the optimized version for IsRunningOnIOS11OrLater() unless we have
a good generic solution.
>
>
https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne...
> base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater }
> g_ios_version = kUnknown;
> On 2017/06/14 00:59:48, Eugene But wrote:
> > Should we cache the result of OSVersionAsArray instead? Otherwise the issue
> may
> > show up again later.
>
> Can you clarify what issue might "show up again later"? Is the problem that
> some code calls IsRunningOnOrLater() directly?
On 2017/06/14 02:14:41, kapishnikov wrote: > On 2017/06/14 01:10:14, rohitrao (ping after 24h) wrote: > > Caching this seems like a good idea if it's showing up as a hotspot. > > > > Is there a doc or a bug that contains more details about how we found this > > issue? I'm not sure what ComputeCurrentTicks() is, for example. > > We found this hotspot by profiling an app. There is a placeholder bug 724546 but > it is very generic. Here is an example of places that eventually call > IsRunningOnOrLater(). It is actually a call to IsRunningOnIOS10OrLater() in the > release version of the binary. > > base::ios::IsRunningOnOrLater(int, int, int) > (anonymous namespace)::ComputeCurrentTicks() > base::TimeTicks::Now() > (anonymous namespace)::ComputeCurrentTicks() > base::TimeTicks::Now() > net::QuicChromiumClock::Now() const > net::NetworkActivityMonitor::IncrementBytesSent(unsigned long long) > net::NetworkActivityMonitor::IncrementBytesReceived(unsigned long > long) > tracked_objects::TrackedTime::Now() > > net::QuicChromiumPacketWriter::WritePacketToSocket(scoped_refptr<net::StringIOBuffer>) > > > > > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm > > File base/ios/ios_util.mm (right): > > > > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > > base/ios/ios_util.mm:24: // Caches the result of IsRunningOnIOS10OrLater(). > > We'll be adding IsRunningOnIOS11OrLater() shortly, so it might be worth either > > adding this now or considering how we would add it later. For example, the > > ternary operation on line 37 wouldn't work. > > I agree. Maybe we should cache the result of IsRunningOnOrLater() but I would > like to keep the optimized version for IsRunningOnIOS11OrLater() unless we have > a good generic solution. > > > > > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > > base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } > > g_ios_version = kUnknown; > > On 2017/06/14 00:59:48, Eugene But wrote: > > > Should we cache the result of OSVersionAsArray instead? Otherwise the issue > > may > > > show up again later. > > > > Can you clarify what issue might "show up again later"? Is the problem that > > some code calls IsRunningOnOrLater() directly? Is it possible to measure the performance difference between caching IsRunningOnOrLater() and IsRunningOnIOS10OrLater()? If it is negligible, then I think caching IsRunningOnOrLater() is preferable. Regarding non thread safety. I honestly don't know about possible implications. More likely this function is only called on the UI thread, but it's also not very hard to use dispatch_once to be on the safe side.
> https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > > base/ios/ios_util.mm:35: if (g_ios_version == kUnknown) { > > This makes the function non-thread safe, which you can fix by using > > dispatch_once API. > > What would be the worst case scenario if we don't add synchronization? My > assumption was that in worst case scenario multiple threads could call > IsRunningOnOrLater() and update the variable to the same value. Is there any > chance that one of the threads will see a corrupted value? My understanding is that if a non-thread-safe variable is accessed by multiple threads, you have the potential for reading or writing garbage/corrupted data. Even in simple scenarios where the variable is always updated to the same value, the compiler is allowed to make assumptions that can cause undefined behavior. Since we know that this function will be called from multiple threads, we need to use some sort of synchronization. I've read articles in the past that explain this better. I'll try to dig up a link.
On 2017/06/14 01:10:14, rohitrao (ping after 24h) wrote: > Caching this seems like a good idea if it's showing up as a hotspot. > > Is there a doc or a bug that contains more details about how we found this > issue? I'm not sure what ComputeCurrentTicks() is, for example. > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm > File base/ios/ios_util.mm (right): > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > base/ios/ios_util.mm:24: // Caches the result of IsRunningOnIOS10OrLater(). > We'll be adding IsRunningOnIOS11OrLater() shortly, so it might be worth either > adding this now or considering how we would add it later. For example, the > ternary operation on line 37 wouldn't work. > > https://codereview.chromium.org/2935973002/diff/20001/base/ios/ios_util.mm#ne... > base/ios/ios_util.mm:25: enum { kUnknown, kIos9orEarlier, kIos10orLater } > g_ios_version = kUnknown; > On 2017/06/14 00:59:48, Eugene But wrote: > > Should we cache the result of OSVersionAsArray instead? Otherwise the issue > may > > show up again later. It looks that we are already caching the result of OSVersionAsArray(). The OSVersionAsArray() function should be called once because the |current_version| variable is declared as static: static const int32_t* current_version = OSVersionAsArray(); Still, there is high CPU utilization according to profiling. > > Can you clarify what issue might "show up again later"? Is the problem that > some code calls IsRunningOnOrLater() directly?
I have uploaded a new patch with dispatch_once invocation. PTAL. A local profiling has shown that the CPU utilization decreased from 0.6% to 0.1% of the total network thread utilization. Since we are already caching OSVersionAsArray() result, I don't think we should optimize IsRunningOnIOS11OrLater() method unless we can see that it is becoming a bottleneck. Maybe we can use a template for that. We should optimize IsRunningOnIOS10OrLater() because it is called on every base::TimeTicks::Now() invocation.
Description was changed from ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. ========== to ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. BUG=724546 ==========
On 2017/06/14 20:35:06, kapishnikov wrote: > I have uploaded a new patch with dispatch_once invocation. PTAL. A local > profiling has shown that the CPU utilization decreased from 0.6% to 0.1% of the > total network thread utilization. > > Since we are already caching OSVersionAsArray() result, I don't think we should > optimize IsRunningOnIOS11OrLater() method unless we can see that it is becoming > a bottleneck. Maybe we can use a template for that. We should optimize > IsRunningOnIOS10OrLater() because it is called on every base::TimeTicks::Now() > invocation. Honestly it will look strange to optimize IsRunningOnIOS10OrLater but leave IsRunningOnIOS11OrLater as it is. ios_util.mm API should not make any assumptions about it's usage so if IsRunningOnIOS10OrLater is fast then I feel it should be the case for all other functions as well. How about you use static keyword inside ComputeCurrentTicks()? You know that ComputeCurrentTicks() is called very often, so making optimization seems reasonable.
On 2017/06/14 23:58:48, Eugene But wrote: > On 2017/06/14 20:35:06, kapishnikov wrote: > > I have uploaded a new patch with dispatch_once invocation. PTAL. A local > > profiling has shown that the CPU utilization decreased from 0.6% to 0.1% of > the > > total network thread utilization. > > > > Since we are already caching OSVersionAsArray() result, I don't think we > should > > optimize IsRunningOnIOS11OrLater() method unless we can see that it is > becoming > > a bottleneck. Maybe we can use a template for that. We should optimize > > IsRunningOnIOS10OrLater() because it is called on every base::TimeTicks::Now() > > invocation. > Honestly it will look strange to optimize IsRunningOnIOS10OrLater but leave > IsRunningOnIOS11OrLater as it is. ios_util.mm API should not make any > assumptions about it's usage so if IsRunningOnIOS10OrLater is fast then I feel > it should be the case for all other functions as well. How about you use static > keyword inside ComputeCurrentTicks()? You know that ComputeCurrentTicks() is > called very often, so making optimization seems reasonable. Are there any thread-safety concerns with making a static variable inside ComputeCurrentTicks()? We have other places in the code where we cache the value of IsRunningOnXXOrLater(), so caching it inside ComputeCurrentTicks() wouldn't be unusual.
On 2017/06/15 00:11:47, rohitrao (ping after 24h) wrote: > On 2017/06/14 23:58:48, Eugene But wrote: > > On 2017/06/14 20:35:06, kapishnikov wrote: > > > I have uploaded a new patch with dispatch_once invocation. PTAL. A local > > > profiling has shown that the CPU utilization decreased from 0.6% to 0.1% of > > the > > > total network thread utilization. > > > > > > Since we are already caching OSVersionAsArray() result, I don't think we > > should > > > optimize IsRunningOnIOS11OrLater() method unless we can see that it is > > becoming > > > a bottleneck. Maybe we can use a template for that. We should optimize > > > IsRunningOnIOS10OrLater() because it is called on every > base::TimeTicks::Now() > > > invocation. > > Honestly it will look strange to optimize IsRunningOnIOS10OrLater but leave > > IsRunningOnIOS11OrLater as it is. ios_util.mm API should not make any > > assumptions about it's usage so if IsRunningOnIOS10OrLater is fast then I feel > > it should be the case for all other functions as well. How about you use > static > > keyword inside ComputeCurrentTicks()? You know that ComputeCurrentTicks() is > > called very often, so making optimization seems reasonable. > > Are there any thread-safety concerns with making a static variable inside > ComputeCurrentTicks()? > > We have other places in the code where we cache the value of > IsRunningOnXXOrLater(), so caching it inside ComputeCurrentTicks() wouldn't be > unusual. Since we are using c++ 11 that guarantees that a local static variable is initialized only once even if multiple threads reached the initializer, what about the caching simplification that I uploaded in PS5? I think it is better if we cache at the source rather than duplicating the logic in the caller methods.
Thanks! lgtm I guess it's ok if we'll have to add caching to IsRunningOnIOS12OrLater and every subsequent function.
lgtm The "duplicated" caching here is so small that it doesn't bother me. It'll also be substantially easier to add new varieties of these functions as needed, so I think the tradeoff is worth it. Thanks for your patience with this CL! Does the latest version fix the original performance issues?
The CQ bit was checked by kapishnikov@chromium.org
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": 80001, "attempt_start_ts": 1497535090033500,
"parent_rev": "f46f8cdc0ebf789cc6be1ee524955cef93229111", "commit_rev":
"71f40c3bd210c7450cbd2e171578c1d950750346"}
Message was sent while issue was closed.
Description was changed from ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. BUG=724546 ========== to ========== iOS: Cache the result of IsRunningOnIOS10OrLater() The IsRunningOnIOS10OrLater() is called for every ComputeCurrentTicks() invocation. Some profiling results have shown that the call may take 0.6% of the total network CPU utilization of the network thread. BUG=724546 Review-Url: https://codereview.chromium.org/2935973002 Cr-Commit-Position: refs/heads/master@{#479692} Committed: https://chromium.googlesource.com/chromium/src/+/71f40c3bd210c7450cbd2e171578... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/71f40c3bd210c7450cbd2e171578... |
