New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 619850 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jul 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

74.7% regression in blink_perf.canvas at 399453:399464

Project Member Reported by oth@chromium.org, Jun 14 2016

Issue description

See the link to graphs below.
 

Comment 1 by oth@chromium.org, Jun 14 2016

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

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICg_Ke8qAoM


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

chromium-rel-mac-retina
Project Member

Comment 2 by 42576172...@developer.gserviceaccount.com, Jun 14 2016

Cc: msar...@google.com
Owner: msar...@google.com

=== Auto-CCing suspected CL author msarett@google.com ===

Hi msarett@google.com, the bisect results pointed to your CL below as possibly
causing a regression. Please have a look at this info and see whether
your CL be related.


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Update libpng to 1.6.22
Author  : msarett
Commit description:
  
BUG= 599917 
BUG= 618061 

Review-Url: https://codereview.chromium.org/2021403002
Cr-Commit-Position: refs/heads/master@{#399464}
Commit  : 8c0ac8d882561cca77d5184248708b0ce9c851d4
Date    : Mon Jun 13 16:19:43 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev  N  Good?
chromium@399452  212.893  1.28144  5  good
chromium@399458  212.105  3.13468  5  good
chromium@399461  212.178  2.3473   5  good
chromium@399463  215.781  2.14336  5  good
chromium@399464  364.292  9.57979  5  bad    <--

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 619850

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.canvas
Test Metric: toBlob_duration/toBlob_duration
Relative Change: 71.12%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1332
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009894376284668672


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5772423689928704

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Jun 14 2016


===== BISECT JOB RESULTS =====
Status: completed


===== SUSPECTED CL(s) =====
Subject : Update libpng to 1.6.22
Author  : msarett
Commit description:
  
BUG= 599917 
BUG= 618061 

Review-Url: https://codereview.chromium.org/2021403002
Cr-Commit-Position: refs/heads/master@{#399464}
Commit  : 8c0ac8d882561cca77d5184248708b0ce9c851d4
Date    : Mon Jun 13 16:19:43 2016


===== TESTED REVISIONS =====
Revision         Mean     Std Dev   N  Good?
chromium@399452  212.9    2.80482   5  good
chromium@399458  210.237  2.15102   5  good
chromium@399461  211.876  0.823958  5  good
chromium@399463  214.138  1.93407   5  good
chromium@399464  366.748  3.8223    5  bad    <--

Bisect job ran on: mac_retina_perf_bisect
Bug ID: 619850

Test Command: src/tools/perf/run_benchmark -v --browser=release --output-format=chartjson --upload-results --also-run-disabled-tests blink_perf.canvas
Test Metric: toBlob_duration/toBlob_duration
Relative Change: 72.26%
Score: 99.9

Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/mac_retina_perf_bisect/builds/1333
Job details: https://chromeperf.appspot.com/buildbucket_job_status/9009894371754975904


Not what you expected? We'll investigate and get back to you!
  https://chromeperf.appspot.com/bad_bisect?try_job_id=5874606330609664

| O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq
|  X  | for more information addressing perf regression bugs. For feedback,
| / \ | file a bug with component Tests>AutoBisect.  Thank you!
Cc: scroggo@chromium.org
Labels: -performance-sheriff Performance-Sheriff
This is a performance regression in png encoding.

I've identified the cause and proposed a fix to upstream.
https://github.com/glennrp/libpng/pull/116
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93

commit b43ad19eed3d9b660f2fc3e445e250a1b42b6b93
Author: msarett <msarett@google.com>
Date: Wed Jun 22 16:14:08 2016

Fix performance regression in png encoding caused by libpng update

libpng calculates "filter heuristics" to choose which png
filter to apply.  Chrome need not calculate these "filter
heuristics" because we always choose to use the SUB filter,
for speed.

libpng 1.6 refactors the SUB filter to share code, also
forcing us to calculate the filter heuristics, even though
we don't need to.  This caused a performance regression.

This CL cherry picks a fix from upstream that:
(1) Passes PNG_SIZE_MAX to the filter code.  This allows
the compiler to optimize out the heuristic calculation
code, fixing the performance regression.
(2) Fixes overflow handling in the calculation of filter
heuristics.  This won't affect Chrome.

BUG= 619850 
BUG= 599917 

Review-Url: https://codereview.chromium.org/2062423002
Cr-Commit-Position: refs/heads/master@{#401298}

[modify] https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93/third_party/WebKit/Source/platform/image-encoders/PNGImageEncoder.cpp
[modify] https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93/third_party/libpng/BUILD.gn
[modify] https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93/third_party/libpng/README.chromium
[modify] https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93/third_party/libpng/libpng.gyp
[modify] https://crrev.com/b43ad19eed3d9b660f2fc3e445e250a1b42b6b93/third_party/libpng/pngwutil.c

Comment 7 by msar...@google.com, Jun 23 2016

Mac regressions are fixed.  Expecting the same for Win/Android once results are available.

Comment 8 by msar...@google.com, Jun 27 2016

The fix isn't working on all platforms.  I suspect it is compiler related, given that the fix depends on the compiler making optimizations.

Mac/Linux clang is fine, but Nexus 5x (GCC I think?) and Windows (maybe MSVC?) are not fixed.

I've reached back out to upstream about landing a more portable fix.
Project Member

Comment 9 by sheriffbot@chromium.org, Jul 5 2016

Labels: -M-53 M-54 MovedFrom-53
Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/67058eff9c2a51e589388d7d494b1490530a1ec5

commit 67058eff9c2a51e589388d7d494b1490530a1ec5
Author: msarett <msarett@google.com>
Date: Fri Jul 08 14:29:14 2016

Fix for png encoding performance regressions - downstream diff

The original fix was:

> Fix performance regression in png encoding caused by libpng update
>
> libpng calculates "filter heuristics" to choose which png
> filter to apply.  Chrome need not calculate these "filter
> heuristics" because we always choose to use the SUB filter,
> for speed.
>
> libpng 1.6 refactors the SUB filter to share code, also
> forcing us to calculate the filter heuristics, even though
> we don't need to.  This caused a performance regression.
>
> This CL cherry picks a fix from upstream that:
> (1) Passes PNG_SIZE_MAX to the filter code.  This allows
> the compiler to optimize out the heuristic calculation
> code, fixing the performance regression.
> (2) Fixes overflow handling in the calculation of filter
> heuristics.  This won't affect Chrome.
>
> Review-Url: https://codereview.chromium.org/2062423002

This fix was effective everywhere we use clang, but did
not work on Android (GCC) or Windows (MSVS) because the
compiler was not applying the appropriate optimizations.

This CL contains a more portable fix.  This was briefly
landed by libpng, but backed out.  They fear it increases
code size and that the compiler can be forced to make
the same optimization.

FWIW, we have demonstrated that we can force the compiler
to make the same optimization.
https://codereview.chromium.org/2119813003/

But we feel that changing the compiler options is fragile
and it is better to just carry a diff.

BUG= 619850 
BUG= 599917 

Review-Url: https://codereview.chromium.org/2132783002
Cr-Commit-Position: refs/heads/master@{#404379}

[modify] https://crrev.com/67058eff9c2a51e589388d7d494b1490530a1ec5/third_party/libpng/README.chromium
[modify] https://crrev.com/67058eff9c2a51e589388d7d494b1490530a1ec5/third_party/libpng/pngwutil.c

Project Member

Comment 11 by bugdroid1@chromium.org, Jul 11 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/38bef90282b731572ca2fab9efaef54d7e3b3726

commit 38bef90282b731572ca2fab9efaef54d7e3b3726
Author: msarett <msarett@google.com>
Date: Mon Jul 11 14:53:49 2016

Update README.chromium in libpng

BUG= 599917 
BUG= 619850 

Review-Url: https://codereview.chromium.org/2138563002
Cr-Commit-Position: refs/heads/master@{#404658}

[modify] https://crrev.com/38bef90282b731572ca2fab9efaef54d7e3b3726/third_party/libpng/README.chromium

Comment 12 by msar...@google.com, Jul 12 2016

Status: Fixed (was: Assigned)

Sign in to add a comment