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

Issue 644573 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
inactive
Closed: Sep 2016
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Favicon downscale janks tabswitcher entry

Project Member Reported by aelias@chromium.org, Sep 7 2016

Issue description

(Forked off from http://crbug.com/641517 )

I've observed in tracing that favicon downscaling code, which includes PNG decode and reencode, runs on the UI thread for 4ms per (total) tab whenever the tab switcher is entered on Android, competing with the tabswitcher animation and janking it quite severely when the number of tabs is high.  This should be moved onto a background thread (or if possible streamed more gradually or avoided altogether).
 
Favicon_png_decode_encode.png
70.6 KB View Download
trace_50tabs_showtabswichter_m52.json
2.2 MB View Download
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 8 2016

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

commit 825375667cf20a127144357b2bcb24201f794e7d
Author: aelias <aelias@chromium.org>
Date: Thu Sep 08 04:26:10 2016

Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG= 644573 

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

[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon/core/favicon_service.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon_base/favicon_util.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon_base/favicon_util.h
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/history/core/browser/history_service.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-2854
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/825375667cf20a127144357b2bcb24201f794e7d

commit 825375667cf20a127144357b2bcb24201f794e7d
Author: aelias <aelias@chromium.org>
Date: Thu Sep 08 04:26:10 2016

Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG= 644573 

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

[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon/core/favicon_service.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon_base/favicon_util.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/favicon_base/favicon_util.h
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/825375667cf20a127144357b2bcb24201f794e7d/components/history/core/browser/history_service.cc

Labels: Merge-Request-54

Comment 4 by dimu@chromium.org, Sep 8 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 9 2016

Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5

commit ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5
Author: Alexandre Elias <aelias@chromium.org>
Date: Fri Sep 09 00:43:44 2016

Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG= 644573 

Review-Url: https://codereview.chromium.org/2313563002
Cr-Commit-Position: refs/heads/master@{#417190}
(cherry picked from commit 825375667cf20a127144357b2bcb24201f794e7d)

Review URL: https://codereview.chromium.org/2317423004 .

Cr-Commit-Position: refs/branch-heads/2840@{#260}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon/core/favicon_service.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon_base/favicon_util.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon_base/favicon_util.h
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/history/core/browser/history_service.cc

Status: Fixed (was: Assigned)
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 27 2016

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

commit ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5
Author: Alexandre Elias <aelias@chromium.org>
Date: Fri Sep 09 00:43:44 2016

Reduce jank from favicon downscaling.

This refactors the favicon decode+downscaling+reencode logic into a
utility method and calls it on the history thread instead of
janking the UI thread.  (I left in a call in the original site as a
fallback path, but it will typically early-return as the work was
already done.)

I also added TRACE_EVENTs to help diagnose future performance issues.

BUG= 644573 

Review-Url: https://codereview.chromium.org/2313563002
Cr-Commit-Position: refs/heads/master@{#417190}
(cherry picked from commit 825375667cf20a127144357b2bcb24201f794e7d)

Review URL: https://codereview.chromium.org/2317423004 .

Cr-Commit-Position: refs/branch-heads/2840@{#260}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon/core/favicon_service.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon_base/favicon_util.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/favicon_base/favicon_util.h
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/ada426ca9d3b55c1a578fc30674dd2ac3f7ef5f5/components/history/core/browser/history_service.cc

Sign in to add a comment