Issue metadata
Sign in to add a comment
|
24.3% regression in media_perftests at 487039:487098 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Jul 24 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8973170092580885168
,
Jul 24 2017
=== Auto-CCing suspected CL author hans@chromium.org === Hi hans@chromium.org, the bisect results pointed to your CL, please take a look at the results. === BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : hans Commit : 3856eab55bb241fb054a878d9922e65e7b5b0931 Date : Mon Jul 17 12:03:22 2017 Subject: Make base::WeakPtr::Get() fast Bisect Details Configuration: winx64ati_perf_bisect Benchmark : media_perftests Metric : clockless_playback/sfx_f32le.wav Change : 15.51% | 6695.32454532 -> 5617.30092867 Revision Result N chromium@487038 6695.32 +- 1158.97 9 good chromium@487046 6319.26 +- 2121.93 14 good chromium@487047 6773.55 +- 1165.48 6 good chromium@487048 5428.57 +- 1470.49 9 bad <-- chromium@487050 5233.08 +- 1031.69 6 bad chromium@487053 5129.44 +- 1595.62 9 bad chromium@487068 5551.12 +- 2768.62 14 bad chromium@487098 5617.3 +- 1891.42 9 bad To Run This Test .\src\out\Release_x64\media_perftests.exe --single-process-tests More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8973170092580885168 For feedback, file a bug with component Speed>Bisection
,
Jul 25 2017
I find it really hard to believe that clockless_playback/sfx_f32le.wav's performance has much to do with WeakPtr. How reliable are these results?
,
Jul 25 2017
The clockless_playback tests are simply processing the audio (in this case) as fast as it can. There are a bunch of callbacks involved (when new buffer is available, when buffer is done), and I suspect a lot of them are done with WeakPtr's. I'll kick off another bisect and see if it comes up with the same results.
,
Jul 25 2017
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/8973058263930245344
,
Jul 26 2017
=== BISECT JOB RESULTS === Perf regression found with culprit Suspected Commit Author : hans Commit : 3856eab55bb241fb054a878d9922e65e7b5b0931 Date : Mon Jul 17 12:03:22 2017 Subject: Make base::WeakPtr::Get() fast Bisect Details Configuration: winx64ati_perf_bisect Benchmark : media_perftests Metric : clockless_playback/sfx_f32le.wav Change : 10.65% | 6350.19748305 -> 5674.08107355 Revision Result N chromium@487021 6350.2 +- 2966.52 21 good chromium@487039 6516.57 +- 843.399 6 good chromium@487044 6469.32 +- 1263.04 6 good chromium@487046 6832.09 +- 1351.86 6 good chromium@487047 6554.93 +- 1557.06 6 good chromium@487048 5051.62 +- 1593.58 6 bad <-- chromium@487058 5769.38 +- 2792.03 21 bad chromium@487093 5752.44 +- 1602.74 14 bad chromium@487165 5523.52 +- 1164.09 9 bad chromium@487308 5674.08 +- 2477.88 21 bad To Run This Test .\src\out\Release_x64\media_perftests.exe --single-process-tests More information on addressing performance regressions: http://g.co/ChromePerformanceRegressions Debug information about this bisect: https://chromeperf.appspot.com/buildbucket_job_status/8973058263930245344 For feedback, file a bug with component Speed>Bisection
,
Aug 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f9ccb2f250461aa61d42780170377df6c9248e3f commit f9ccb2f250461aa61d42780170377df6c9248e3f Author: Hans Wennborg <hans@chromium.org> Date: Mon Aug 07 19:44:09 2017 Revert "WeakPtr: Reset flag_ to NullFlag() in moved-from objects" This reverts commit 3fc9f5b033889b5f0224e62cea23c55dba96268e. Reason for revert: I'm backing out the WeakPtr changes. While the new code was objectively better, it didn't show on any perf bots, regressed size on Linux quite a bit, and seems to have introduced a new crasher on Android. To summarize: it seems it's not worth it. The WeakPtr revert will be done in two patches. This one, and the main one (https://chromium-review.googlesource.com/c/604088) that also needs to be merged to M61. Original change's description: > WeakPtr: Reset flag_ to NullFlag() in moved-from objects > > A moved-from object shouldn't be used, but in case it is, set flag_ to > NullFlag() instead of nullptr to avoid crashing. > > This is a speculative fix for a crash seen in the wild. > > BUG=748495 > > Change-Id: I0b26156161c860e64ceb566e0d03cf3137ff8196 > Reviewed-on: https://chromium-review.googlesource.com/585563 > Commit-Queue: Hans Wennborg <hans@chromium.org> > Reviewed-by: Nico Weber <thakis@chromium.org> > Cr-Commit-Position: refs/heads/master@{#489536} BUG=748495, 748075 , 744747 Change-Id: I2453f329882cd911482cc9a27c0ee99f13693d03 Reviewed-on: https://chromium-review.googlesource.com/604087 Commit-Queue: Hans Wennborg <hans@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#492385} [modify] https://crrev.com/f9ccb2f250461aa61d42780170377df6c9248e3f/base/memory/weak_ptr.cc [modify] https://crrev.com/f9ccb2f250461aa61d42780170377df6c9248e3f/base/memory/weak_ptr.h
,
Aug 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4cfa301cda2ab84a496f3c13a5e3fa2f8de94fca commit 4cfa301cda2ab84a496f3c13a5e3fa2f8de94fca Author: Hans Wennborg <hans@chromium.org> Date: Tue Aug 08 03:33:38 2017 Revert "Make base::WeakPtr::Get() fast" This reverts commit 71f161e3bd95827876f67c656168f936d88a6e3a and 3856eab55bb241fb054a878d9922e65e7b5b0931. Reason for revert: I'm backing out the WeakPtr changes. While the new code was objectively better, it didn't show on any perf bots, regressed size on Linux quite a bit because the new code was smaller and thus inlined more often, and seems to have introduced a new crasher on Android. To summarize: it seems it's not worth it. The WeakPtr revert will be done in two patches. The first part is in https://chromium-review.googlesource.com/c/604087 This second part should be merged to M61. Original change's description: > Make base::WeakPtr::Get() fast > > It was previous calling the out-of-line method WeakReference::is_valid(). > That means that code like > > if (my_weak_ptr) > my_weak_ptr->foo(); > > would do two calls to is_valid(), plus tests and branching. > > The is_valid() method showed up as one of the most frequently called non-inline > functions during browser start-up on Windows (about 1M calls). > > is_valid() was also inefficient in itself, because it had to do a null-check > flag_, as well as checking if the flag was marked valid. > > This patch removes the null-pointer check by using a sentinel Flag object instead of > a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized > bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently > setting EFLAGS so that the code above just becomes a few loads, AND, branch, > and call. (The size of Flag is unchanged; it grows into the padding only.) > > This is expected to reduce the binary size by ~48KB on Android, and increase it > by 79KB on x64 Linux -- something that's paid for by the split-out refactorings > to WeakPtr and WeakPtrFactory. > > Exposing the SequenceChecker calls in inline functions caused the > MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds, > which is why this patch also lowers the recursion depth there. > > BUG= 728324 , 738183 , 738193 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester > > Review-Url: https://codereview.chromium.org/2963623002 > Cr-Commit-Position: refs/heads/master@{#487048} BUG=748495, 748075 , 744747 Change-Id: Id85da06115ab9453267ade4d6f8efcfd1b26220c Reviewed-on: https://chromium-review.googlesource.com/604088 Commit-Queue: Hans Wennborg <hans@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#492520} [modify] https://crrev.com/4cfa301cda2ab84a496f3c13a5e3fa2f8de94fca/base/memory/weak_ptr.cc [modify] https://crrev.com/4cfa301cda2ab84a496f3c13a5e3fa2f8de94fca/base/memory/weak_ptr.h
,
Aug 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f393cc0504d4da327d541a054c404aa422470c1c commit f393cc0504d4da327d541a054c404aa422470c1c Author: Hans Wennborg <hans@chromium.org> Date: Thu Aug 10 15:45:38 2017 Revert "Make base::WeakPtr::Get() fast" This reverts commit 71f161e3bd95827876f67c656168f936d88a6e3a and 3856eab55bb241fb054a878d9922e65e7b5b0931. Reason for revert: I'm backing out the WeakPtr changes. While the new code was objectively better, it didn't show on any perf bots, regressed size on Linux quite a bit because the new code was smaller and thus inlined more often, and seems to have introduced a new crasher on Android. To summarize: it seems it's not worth it. The WeakPtr revert will be done in two patches. The first part is in https://chromium-review.googlesource.com/c/604087 This second part should be merged to M61. Original change's description: > Make base::WeakPtr::Get() fast > > It was previous calling the out-of-line method WeakReference::is_valid(). > That means that code like > > if (my_weak_ptr) > my_weak_ptr->foo(); > > would do two calls to is_valid(), plus tests and branching. > > The is_valid() method showed up as one of the most frequently called non-inline > functions during browser start-up on Windows (about 1M calls). > > is_valid() was also inefficient in itself, because it had to do a null-check > flag_, as well as checking if the flag was marked valid. > > This patch removes the null-pointer check by using a sentinel Flag object instead of > a nullptr. And instead of storing a bool in the Flag, it stores a pointer-sized > bitmask 0 or ~0, so it can be AND'ed with the pointer value and conveniently > setting EFLAGS so that the code above just becomes a few loads, AND, branch, > and call. (The size of Flag is unchanged; it grows into the padding only.) > > This is expected to reduce the binary size by ~48KB on Android, and increase it > by 79KB on x64 Linux -- something that's paid for by the split-out refactorings > to WeakPtr and WeakPtrFactory. > > Exposing the SequenceChecker calls in inline functions caused the > MessageLoopTestType*.Nesting test to overflow on Win64 dcheck release builds, > which is why this patch also lowers the recursion depth there. > > BUG= 728324 , 738183 , 738193 > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester > > Review-Url: https://codereview.chromium.org/2963623002 > Cr-Commit-Position: refs/heads/master@{#487048} BUG=748495, 748075 , 744747 TBR=hans@chromium.org (cherry picked from commit 4cfa301cda2ab84a496f3c13a5e3fa2f8de94fca) Change-Id: Id85da06115ab9453267ade4d6f8efcfd1b26220c Reviewed-on: https://chromium-review.googlesource.com/604088 Commit-Queue: Hans Wennborg <hans@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#492520} Reviewed-on: https://chromium-review.googlesource.com/610446 Reviewed-by: Hans Wennborg <hans@chromium.org> Cr-Commit-Position: refs/branch-heads/3163@{#432} Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528} [modify] https://crrev.com/f393cc0504d4da327d541a054c404aa422470c1c/base/memory/weak_ptr.cc [modify] https://crrev.com/f393cc0504d4da327d541a054c404aa422470c1c/base/memory/weak_ptr.h
,
Aug 10 2017
Looking at https://chromeperf.appspot.com/group_report?bug_id=748075, there's no significant movement of the graph around #492520 so I don't think it was related to this change after all. |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 24 2017