New issue
Advanced search Search tips

Issue 748075 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

24.3% regression in media_perftests at 487039:487098

Project Member Reported by jrumm...@chromium.org, Jul 24 2017

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Jul 24 2017

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=748075

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=c13ba79f6628ca3e2c2ed88192eb87c19eca38643f599da7ff388d14bd34f292


Bot(s) for this bug's original alert(s):

chromium-rel-win7-gpu-ati
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jul 24 2017

Cc: h...@chromium.org
Owner: h...@chromium.org

=== 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

Comment 4 by h...@chromium.org, 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?
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.
Project Member

Comment 7 by 42576172...@developer.gserviceaccount.com, 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
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, Aug 10 2017

Labels: merge-merged-3163
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

Comment 11 by h...@chromium.org, Aug 10 2017

Status: WontFix (was: Untriaged)
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