base::Time::FromExploded is very slow on Mac and accounts for 80ms during startup |
||||||
Issue descriptionbase::Time::FromExploded is very slow on Mac and accounts for 80ms during startup. We can see this from new UMA Sampling Profiler data: https://uma.googleplex.com/callstacks?sid=a1ef74e9ee27492b22f7046543ce6f53 In particular, 22ms of that is part of Field Trial creation (where I noticed it first), but the rest is also elsewhere. For field trial creation, I can certainly work around this - since a lot of time we use a dummy expiration date anyway - so I can detect that and avoid doing this work for every field trial (22ms is the aggregate amount). However, it would be better if the base code could simply be fixed to not be so slow. cc'ing folks who worked on the implementation and also some Mark, Robert and Erik who may have ideas on how to speed it up.
,
Nov 5 2017
,
Nov 6 2017
Would it be faster to use the POSIX library for this instead? Or, are the necessary functions not available on Mac (or iOS)?
,
Nov 6 2017
I don't know if the POSIX version of the function suffers from these same problems or not. We don't yet have UMA Sampling Profiler running on platforms that use that code. I think it's worth checking if it works as-is and if so enabling it and seeing what the next day's Canary data looks like. Though we should probably check the history of why the Mac code uses a different implementation. Does anyone cc'd know why we have different code for Mac here?
,
Nov 6 2017
I think this comment in time_mac.cc explains why: // The Time routines in this file use Mach and CoreFoundation APIs, since the // POSIX definition of time_t in Mac OS X wraps around after 2038--and // there are already cookie expiration dates, etc., past that time out in // the field. Using CFDate prevents that problem, and using mach_absolute_time // for TimeTicks gives us nice high-resolution interval timing. That was written in 2008, though, so there may be better alternatives now.
,
Nov 6 2017
The 2038 time_t limitation was only true of the 32-bit build, but we’re 64-bit-only on macOS now.
,
Nov 6 2017
Thanks for the context. Let me see if just using time_exploded_posix.cc Just Works on Mac. If so, can make that change and see how the performance data looks like on Canary.
,
Nov 6 2017
Looks like the time_exploded_posix.cc compiles and works (all unit tests pass) on Mac. Will send a CL!
,
Nov 6 2017
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dcb7897c8ab8914a67a8eb4467772432875685b1 commit dcb7897c8ab8914a67a8eb4467772432875685b1 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Thu Nov 09 02:30:19 2017 Switch Mac to use time_exploded_posix.cc. Previously, we would use a CoreFoundation-based version of these functions implemented in time_mac.cc because time_t on Mac was capped at year 2038. However, with 64-bit that is no longer the case and we can just use time_exploded_posix.cc. From UMA sampling profiler data (see bug), we know that the CoreFoundation versions of these functions are slow and currently account for around 80ms cost during startup. After this change is submitted and makes it to Canary, we can see how the POSIX versions of these functions perform - hopefully much faster than the CoreFoundation ones. Some code in net/ had assumptions around the min and max years that are supported on different platforms by these functions. To make that logic a bit cleaner, this CL introduces two constants in time.h specifying the platform-specific limits - so that downstream code like net can use them instead of baking their own assumptions. Some net error codes are changing as a result of this clean up - which can be seen in the updated net tests. BUG= 781601 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I2cf030fd4d3ed8da87e41bc8ba3dc3b1b99e8f6d Reviewed-on: https://chromium-review.googlesource.com/754079 Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Matt Mueller <mattm@chromium.org> Reviewed-by: Yuri Wiitala <miu@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Cr-Commit-Position: refs/heads/master@{#515062} [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/base/BUILD.gn [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/base/time/time.h [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/base/time/time_exploded_posix.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/base/time/time_mac.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/base/time/time_unittest.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/net/cert/x509_certificate.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/net/cookies/cookie_util.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/net/cookies/cookie_util_unittest.cc [modify] https://crrev.com/dcb7897c8ab8914a67a8eb4467772432875685b1/net/socket/ssl_client_socket_unittest.cc
,
Nov 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0 commit 2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0 Author: Yuta Kitamura <yutak@chromium.org> Date: Thu Nov 09 06:13:34 2017 Revert "Switch Mac to use time_exploded_posix.cc." This reverts commit dcb7897c8ab8914a67a8eb4467772432875685b1. Reason for revert: Caused compile errors on iso-device-xcode-clang. https://build.chromium.org/p/chromium.mac/builders/ios-device-xcode-clang/builds/43807 ../../net/cert/x509_certificate.cc:100:24: error: comparison of constant 2147483647 with expression of type 'const uint16_t' (aka 'const unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare] if (generalized.year > base::Time::kExplodedMaxYear) { ~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../net/cert/x509_certificate.cc:104:24: error: comparison of constant -2147483648 with expression of type 'const uint16_t' (aka 'const unsigned short') is always false [-Werror,-Wtautological-constant-out-of-range-compare] if (generalized.year < base::Time::kExplodedMinYear) { ~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2 errors generated. Original change's description: > Switch Mac to use time_exploded_posix.cc. > > Previously, we would use a CoreFoundation-based version of these > functions implemented in time_mac.cc because time_t on Mac was > capped at year 2038. However, with 64-bit that is no longer the > case and we can just use time_exploded_posix.cc. > > From UMA sampling profiler data (see bug), we know that the > CoreFoundation versions of these functions are slow and currently > account for around 80ms cost during startup. After this change > is submitted and makes it to Canary, we can see how the POSIX > versions of these functions perform - hopefully much faster than > the CoreFoundation ones. > > Some code in net/ had assumptions around the min and max years > that are supported on different platforms by these functions. To > make that logic a bit cleaner, this CL introduces two constants > in time.h specifying the platform-specific limits - so that > downstream code like net can use them instead of baking their > own assumptions. Some net error codes are changing as a result > of this clean up - which can be seen in the updated net tests. > > BUG= 781601 > > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > Change-Id: I2cf030fd4d3ed8da87e41bc8ba3dc3b1b99e8f6d > Reviewed-on: https://chromium-review.googlesource.com/754079 > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Matt Mueller <mattm@chromium.org> > Reviewed-by: Yuri Wiitala <miu@chromium.org> > Reviewed-by: Mark Mentovai <mark@chromium.org> > Cr-Commit-Position: refs/heads/master@{#515062} TBR=miu@chromium.org,mattm@chromium.org,asvitkine@chromium.org,mmenke@chromium.org,mark@chromium.org Change-Id: I260bd4e8e0ab98580ba22646e5d66d56e8933d63 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 781601 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/760116 Reviewed-by: Yuta Kitamura <yutak@chromium.org> Commit-Queue: Yuta Kitamura <yutak@chromium.org> Cr-Commit-Position: refs/heads/master@{#515112} [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/base/BUILD.gn [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/base/time/time.h [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/base/time/time_exploded_posix.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/base/time/time_mac.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/base/time/time_unittest.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/net/cert/x509_certificate.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/net/cookies/cookie_util.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/net/cookies/cookie_util_unittest.cc [modify] https://crrev.com/2ed145b2ac488fa8fe7cb3e6ac362386afbbd7e0/net/socket/ssl_client_socket_unittest.cc
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/af2ab97d5f8900c802fe6a20e726c4f4b8392228 commit af2ab97d5f8900c802fe6a20e726c4f4b8392228 Author: Alexei Svitkine <asvitkine@chromium.org> Date: Fri Nov 10 00:38:46 2017 Re-land "Switch Mac to use time_exploded_posix.cc." Original review: https://chromium-review.googlesource.com/c/chromium/src/+/754079 Reverted here: https://chromium-review.googlesource.com/c/chromium/src/+/760116 Previously, we would use a CoreFoundation-based version of these functions implemented in time_mac.cc because time_t on Mac was capped at year 2038. However, with 64-bit that is no longer the case and we can just use time_exploded_posix.cc. From UMA sampling profiler data (see bug), we know that the CoreFoundation versions of these functions are slow and currently account for around 80ms cost during startup. After this change is submitted and makes it to Canary, we can see how the POSIX versions of these functions perform - hopefully much faster than the CoreFoundation ones. Some code in net/ had assumptions around the min and max years that are supported on different platforms by these functions. To make that logic a bit cleaner, this CL introduces two constants in time.h specifying the platform-specific limits - so that downstream code like net can use them instead of baking their own assumptions. Some net error codes are changing as a result of this clean up - which can be seen in the updated net tests. BUG= 781601 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Change-Id: I5d10a06dd5b0073bfd943f58af08f67980227d4a Reviewed-on: https://chromium-review.googlesource.com/760443 Reviewed-by: Matt Mueller <mattm@chromium.org> Reviewed-by: Yuri Wiitala <miu@chromium.org> Reviewed-by: Mark Mentovai <mark@chromium.org> Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> Cr-Commit-Position: refs/heads/master@{#515370} [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/base/BUILD.gn [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/base/time/time.h [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/base/time/time_exploded_posix.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/base/time/time_mac.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/base/time/time_unittest.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/net/cert/x509_certificate.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/net/cookies/cookie_util.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/net/cookies/cookie_util_unittest.cc [modify] https://crrev.com/af2ab97d5f8900c802fe6a20e726c4f4b8392228/net/socket/ssl_client_socket_unittest.cc
,
Nov 10 2017
Looks like this made it into 64.0.3264.0 canary. So next step is to check how the data looks like from UMA sampling profiler to see what performance looks like with posix version. Will check Monday.
,
Nov 10 2017
This seems to be failing on trybots. https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337212 has TimeTest.TimeT/0 failing: TimeTest.TimeT: ../../base/time/time_unittest.cc:134: Failure Expected: tms.tm_hour Which is: 14 To be equal to: exploded.hour Which is: 22
,
Nov 10 2017
,
Nov 10 2017
This is failing on "ios-simulator" which seems to be a weird combination of Mac and iOS.
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ac4c8a2547bcde57a757991f2efef247e9454455 commit ac4c8a2547bcde57a757991f2efef247e9454455 Author: Avi Drissman <avi@chromium.org> Date: Fri Nov 10 23:06:07 2017 Revert "Re-land "Switch Mac to use time_exploded_posix.cc."" This reverts commit af2ab97d5f8900c802fe6a20e726c4f4b8392228. Reason for revert: This breaks the "ios-simulator" trybot on my CL. See https://chromium-review.googlesource.com/c/chromium/src/+/590273 and failed trybots runs at https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337162 https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337212 which has TimeTest.TimeT/0 failing: TimeTest.TimeT: ../../base/time/time_unittest.cc:134: Failure Expected: tms.tm_hour Which is: 14 To be equal to: exploded.hour Which is: 22 Original change's description: > Re-land "Switch Mac to use time_exploded_posix.cc." > > Original review: > https://chromium-review.googlesource.com/c/chromium/src/+/754079 > Reverted here: > https://chromium-review.googlesource.com/c/chromium/src/+/760116 > > Previously, we would use a CoreFoundation-based version of these > functions implemented in time_mac.cc because time_t on Mac was > capped at year 2038. However, with 64-bit that is no longer the > case and we can just use time_exploded_posix.cc. > > From UMA sampling profiler data (see bug), we know that the > CoreFoundation versions of these functions are slow and currently > account for around 80ms cost during startup. After this change > is submitted and makes it to Canary, we can see how the POSIX > versions of these functions perform - hopefully much faster than > the CoreFoundation ones. > > Some code in net/ had assumptions around the min and max years > that are supported on different platforms by these functions. To > make that logic a bit cleaner, this CL introduces two constants > in time.h specifying the platform-specific limits - so that > downstream code like net can use them instead of baking their > own assumptions. Some net error codes are changing as a result > of this clean up - which can be seen in the updated net tests. > > BUG= 781601 > > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > Change-Id: I5d10a06dd5b0073bfd943f58af08f67980227d4a > Reviewed-on: https://chromium-review.googlesource.com/760443 > Reviewed-by: Matt Mueller <mattm@chromium.org> > Reviewed-by: Yuri Wiitala <miu@chromium.org> > Reviewed-by: Mark Mentovai <mark@chromium.org> > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > Cr-Commit-Position: refs/heads/master@{#515370} TBR=miu@chromium.org,mattm@chromium.org,asvitkine@chromium.org,mmenke@chromium.org,mark@chromium.org Change-Id: I4a582e11d4fd8490cf22cc37d4d746b4c3731957 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 781601 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/764868 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#515727} [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/base/BUILD.gn [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/base/time/time.h [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/base/time/time_exploded_posix.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/base/time/time_mac.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/base/time/time_unittest.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/net/cert/x509_certificate.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/net/cookies/cookie_util.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/net/cookies/cookie_util_unittest.cc [modify] https://crrev.com/ac4c8a2547bcde57a757991f2efef247e9454455/net/socket/ssl_client_socket_unittest.cc
,
Nov 10 2017
The simulator’s bad about timezones. That’s probably what this is. 22 - 14 = 8, the same as UTC - PST.
,
Nov 10 2017
asvitkine notes that my experience is likely to be bug 782033.
,
Nov 10 2017
Yep, I chatted with Rohit about this earlier in the week before my CL landed and we believed it should be unrelated - since it was already been seen at that point. (I was worried about this because I saw this in one of my try runs on my CL.) My CL should not change any behavior of what base time code does on iOS. It does add a constant to time.h - but that constant is not used in base. I also had asked Rohit about the code that's used on the simulator and it shouldn't be the mac codepath. So ... can you please revert your revert of my CL?
,
Nov 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfecbd146962ac8edebb4cf8ff94c8604fac935b commit cfecbd146962ac8edebb4cf8ff94c8604fac935b Author: Avi Drissman <avi@chromium.org> Date: Fri Nov 10 23:19:24 2017 Revert "Revert "Re-land "Switch Mac to use time_exploded_posix.cc.""" This reverts commit ac4c8a2547bcde57a757991f2efef247e9454455. Reason for revert: This appears to be bug 782033. Original change's description: > Revert "Re-land "Switch Mac to use time_exploded_posix.cc."" > > This reverts commit af2ab97d5f8900c802fe6a20e726c4f4b8392228. > > Reason for revert: > > This breaks the "ios-simulator" trybot on my CL. See https://chromium-review.googlesource.com/c/chromium/src/+/590273 and failed trybots runs at > > https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337162 > https://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/337212 > > which has TimeTest.TimeT/0 failing: > TimeTest.TimeT: > ../../base/time/time_unittest.cc:134: Failure > Expected: tms.tm_hour > Which is: 14 > To be equal to: exploded.hour > Which is: 22 > > > Original change's description: > > Re-land "Switch Mac to use time_exploded_posix.cc." > > > > Original review: > > https://chromium-review.googlesource.com/c/chromium/src/+/754079 > > Reverted here: > > https://chromium-review.googlesource.com/c/chromium/src/+/760116 > > > > Previously, we would use a CoreFoundation-based version of these > > functions implemented in time_mac.cc because time_t on Mac was > > capped at year 2038. However, with 64-bit that is no longer the > > case and we can just use time_exploded_posix.cc. > > > > From UMA sampling profiler data (see bug), we know that the > > CoreFoundation versions of these functions are slow and currently > > account for around 80ms cost during startup. After this change > > is submitted and makes it to Canary, we can see how the POSIX > > versions of these functions perform - hopefully much faster than > > the CoreFoundation ones. > > > > Some code in net/ had assumptions around the min and max years > > that are supported on different platforms by these functions. To > > make that logic a bit cleaner, this CL introduces two constants > > in time.h specifying the platform-specific limits - so that > > downstream code like net can use them instead of baking their > > own assumptions. Some net error codes are changing as a result > > of this clean up - which can be seen in the updated net tests. > > > > BUG= 781601 > > > > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > > Change-Id: I5d10a06dd5b0073bfd943f58af08f67980227d4a > > Reviewed-on: https://chromium-review.googlesource.com/760443 > > Reviewed-by: Matt Mueller <mattm@chromium.org> > > Reviewed-by: Yuri Wiitala <miu@chromium.org> > > Reviewed-by: Mark Mentovai <mark@chromium.org> > > Commit-Queue: Alexei Svitkine <asvitkine@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#515370} > > TBR=miu@chromium.org,mattm@chromium.org,asvitkine@chromium.org,mmenke@chromium.org,mark@chromium.org > > Change-Id: I4a582e11d4fd8490cf22cc37d4d746b4c3731957 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 781601 > Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet > Reviewed-on: https://chromium-review.googlesource.com/764868 > Reviewed-by: Avi Drissman <avi@chromium.org> > Commit-Queue: Avi Drissman <avi@chromium.org> > Cr-Commit-Position: refs/heads/master@{#515727} TBR=avi@chromium.org,miu@chromium.org,mattm@chromium.org,asvitkine@chromium.org,mmenke@chromium.org,mark@chromium.org Change-Id: Ieaf3bab463fb46f65054111d26c8bef3d0b8292e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 781601 Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet Reviewed-on: https://chromium-review.googlesource.com/764581 Reviewed-by: Avi Drissman <avi@chromium.org> Commit-Queue: Avi Drissman <avi@chromium.org> Cr-Commit-Position: refs/heads/master@{#515737} [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/base/BUILD.gn [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/base/time/time.h [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/base/time/time_exploded_posix.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/base/time/time_mac.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/base/time/time_unittest.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/net/cert/x509_certificate.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/net/cookies/cookie_util.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/net/cookies/cookie_util_unittest.cc [modify] https://crrev.com/cfecbd146962ac8edebb4cf8ff94c8604fac935b/net/socket/ssl_client_socket_unittest.cc
,
Nov 12 2017
Here are the sampling profiler results for 64.0.3264.0 Canary: https://uma.googleplex.com/p/chrome/callstacks/?sid=3c00e74b97207cdac60fd9060bd3d44d And when comparing between previous canary: https://uma.googleplex.com/p/chrome/callstacks/?sid=b0b2e1343beb8e87fff1d1524b3fb76f So FromExploded is now taking 5.8ms aggregate during first 30s of startup compared to 81.8ms aggregate from before the change. The Variations::CreateTrialsFromSeed portion changed from 22ms to 1.3ms So seems like we can call this fixed.
,
Nov 12 2017
,
Nov 22 2017
Note: Besides the improvements to FromExploded linked in comment 22, it also improved Time::Explode: https://uma.googleplex.com/p/chrome/callstacks/?sid=6b0887e18246e9f45a597d9eb6917fde vs. https://uma.googleplex.com/p/chrome/callstacks/?sid=7db5365ce8176ba70fbd8a82f8dd119f So another improvement of 98.6ms (before) to 7.67ms (after). So UMA Sampling Profiler data looks good. However, perf waterfall reported a CPU time regression on loading.desktop which I'm investigating here: https://bugs.chromium.org/p/chromium/issues/detail?id=784381
,
Nov 22 2017
Also, did a sanity test locally and the perf wins are pretty obvious. I just added a "for (int i = 0; i < 10000; i++)" loop around TimeTestOutOfBounds.FromExplodedOutOfBoundsTime. Before the change, this runs in 22920 ms. After the change, this runs in 1832 ms.
,
Jan 5 2018
Bulk adding Performance-Browser label for bugs identified with the UMA Sampling Profiler. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by asvitk...@chromium.org
, Nov 5 2017