Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 591179 Reconsider the ganesh viewport trigger on mobile
Starred by 8 users Project Member Reported by pdr@chromium.org, Mar 1 2016 Back to list
Status: Fixed
Owner:
Closed: Oct 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 472813



Sign in to add a comment
Version: 50/dev
OS: Android

I don't remember all of the original reasons for using a very specific set of triggers for Ganesh on Android. The current trigger logic[1] is a poor experience for developers. I helped implement the original trigger and just this weekend I wasted a full day on a personal project because I had used user-scalable=no instead of minimum-scale=1.0... If I'm messing this up in my demos, so are sites that actually matter.

Can we remove the site triggers so Ganesh runs on all pages as long as the phone has the requisite hardware?

[1] Ganesh triggers are listed here:
https://www.chromium.org/developers/design-documents/chromium-graphics/how-to-get-gpu-rasterization
 
I'd like to remove the trigger once we resolve some remaining smoothness regressions.

Examples are Issue 509665, although almost any complex site with Ganesh forced on will regress smoothness while pinch-zooming.  The main blocker for this is Issue 525259 - enabling the GPU scheduler for Ganesh.

We'll also have to decide if we can remove the trigger requirement for all GPUs.  The Galaxy S6 for example is especially challenged with Ganesh on complex sites (Issue 493854).  It may be harder to expand the content there, and may be undesirable to entirely blacklist it with Ganesh already enabled for sites with the trigger.
Comment 2 by pdr@chromium.org, Mar 10 2016
Cc: aerotwist@chromium.org chrishtr@chromium.org enne@chromium.org
Summary: Reconsider the ganesh viewport trigger on mobile (was: Reconsider the ganesh trigger on mobile)
This same issue just bit performance expert Paul Lewis (aerotwist) who was working with a partner on an existing large site. Are multi-quarter issues like 509665 worse than the cost of not removing the finicky viewport trigger sooner?

For example, could we relax the viewport trigger so a page with any meta viewport (typically indicative of a "mobile designed" page) has the option of ganesh provided the hardware is capable? If not, why don't we remove the viewport trigger entirely so ganesh isn't used at all until it's ready?
Comment 3 by vmi...@chromium.org, Mar 10 2016
Cc: aelias@chromium.org
I was just discussing this with chrishtr@ yesterday.  Relaxing the trigger to be any meta viewport sooner may be a good step.

I think we clearly don't want to lose the ability to opt into Ganesh if your main thread is blocked by raster.  We also don't want to regress your site if you're not main thread driven, which Ganesh may do.  I'm very supportive of ways to improve this delineation.
Comment 4 by aelias@chromium.org, Mar 10 2016
Sounds fine, it's probably time.  We can change ViewportDescription::matchesHeuristicsForGpuRasterization() to "return isSpecifiedByAuthor();"
Comment 5 by vmi...@chromium.org, Mar 10 2016
Should we restrict to "isMetaViewportType()"?
Cc: pdr@chromium.org
I think we're going to revert this change for M51 due to memory regressions on WebView: Issue 596881.
Blockedon: 472813
Project Member Comment 9 by bugdroid1@chromium.org, Apr 6 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e707452627a8ace7951e49421be443fe544aac71

commit e707452627a8ace7951e49421be443fe544aac71
Author: vmiura <vmiura@chromium.org>
Date: Wed Apr 06 20:26:11 2016

Revert of Enable GPU Rasterization for content with any author defined viewport. (patchset #2 id:20001 of https://codereview.chromium.org/1817583003/ )

Reason for revert:
Reverting until we can debug memory health regressions on WebView.

BUG= 596881 

Original issue's description:
> Enable GPU Rasterization for content with any author defined viewport.
>
> BUG= 591179 
>
> Committed: https://crrev.com/b338ee0b3125007b94854e44ed7b6df820c72a75
> Cr-Commit-Position: refs/heads/master@{#382394}

TBR=aelias@chromium.org,pdr@chromium.org,chrishtr@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 591179 

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

Cr-Commit-Position: refs/heads/master@{#385547}

[modify] https://crrev.com/e707452627a8ace7951e49421be443fe544aac71/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/e707452627a8ace7951e49421be443fe544aac71/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[delete] https://crrev.com/b3e75571785c6e135b184806c7fd19aadf7540c7/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[add] https://crrev.com/e707452627a8ace7951e49421be443fe544aac71/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Project Member Comment 11 by bugdroid1@chromium.org, Jul 14 2016
Labels: merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c1bd20736209c83cf1f1bddd7e09f5a3b4ff1c65

commit c1bd20736209c83cf1f1bddd7e09f5a3b4ff1c65
Author: Alex Mineer <amineer@chromium.org>
Date: Thu Jul 14 22:58:19 2016

Revert "Re-enable GPU Rasterization for content with any author defined viewport."

This reverts commit a017b667a53b3ee5f8bc630be98c37ebf53a2339.

BUG=624645, 591179 

Appears to be causing larger performance regressions than anticipated,
reverting on release branch until this can be investigated.

Cr-Commit-Position: refs/branch-heads/2785@{#128}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/c1bd20736209c83cf1f1bddd7e09f5a3b4ff1c65/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/c1bd20736209c83cf1f1bddd7e09f5a3b4ff1c65/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[delete] https://crrev.com/4467b42153c1fbec6e3dcfe30727e6ef2e2e615f/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[add] https://crrev.com/c1bd20736209c83cf1f1bddd7e09f5a3b4ff1c65/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Project Member Comment 12 by bugdroid1@chromium.org, Jul 15 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/10a77702aa0490dfaaa6e354151b24b738474fb4

commit 10a77702aa0490dfaaa6e354151b24b738474fb4
Author: picksi <picksi@chromium.org>
Date: Fri Jul 15 13:11:53 2016

Revert of Re-enable GPU Rasterization for content with any author defined viewport. (patchset #2 id:20001 of https://codereview.chromium.org/2097413003/ )

Reason for revert:
This has caused an unexpectedly large regression in overall PSS (about 5MB, graph here: https://chromeperf.appspot.com/report?sid=0b072725c25637efb0d3a44383da1e24e42bbb3740a00505e4040af2173423b0&start_rev=402061&end_rev=402790).
The owners are both OOO and this is blocking Android release. This has already been reverted in the release branch but our infrastructure cannot gather data from the branch to confirm that the revert has had the intended result.

This revert will allow us to confirm that this CL was the cause of the regression. Once confirmed (or otherwise) via telemetry dashboards this revert will be re-reverted.

Original issue's description:
> Re-enable GPU Rasterization for content with any author defined viewport.
>
> BUG= 591179 
>
> Committed: https://crrev.com/a017b667a53b3ee5f8bc630be98c37ebf53a2339
> Cr-Commit-Position: refs/heads/master@{#402702}

TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 591179 

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

[modify] https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[delete] https://crrev.com/6f484bd579dd71f6a30c37348b7cfb317dfcd462/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[add] https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Cc: ericrk@chromium.org
Project Member Comment 14 by bugdroid1@chromium.org, Jul 18 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f19720ded2369857bee408ab48e11e7b5d28f7b

commit 5f19720ded2369857bee408ab48e11e7b5d28f7b
Author: picksi <picksi@chromium.org>
Date: Mon Jul 18 11:04:30 2016

Reland of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2156553002/ )

Reason for revert:
Reverting change now that we have collected data from telemetry.

Original issue's description:
> Revert of Re-enable GPU Rasterization for content with any author defined viewport. (patchset #2 id:20001 of https://codereview.chromium.org/2097413003/ )
>
> Reason for revert:
> This has caused an unexpectedly large regression in overall PSS (about 5MB, graph here: https://chromeperf.appspot.com/report?sid=0b072725c25637efb0d3a44383da1e24e42bbb3740a00505e4040af2173423b0&start_rev=402061&end_rev=402790).
> The owners are both OOO and this is blocking Android release. This has already been reverted in the release branch but our infrastructure cannot gather data from the branch to confirm that the revert has had the intended result.
>
> This revert will allow us to confirm that this CL was the cause of the regression. Once confirmed (or otherwise) via telemetry dashboards this revert will be re-reverted.
>
> Original issue's description:
> > Re-enable GPU Rasterization for content with any author defined viewport.
> >
> > BUG= 591179 
> >
> > Committed: https://crrev.com/a017b667a53b3ee5f8bc630be98c37ebf53a2339
> > Cr-Commit-Position: refs/heads/master@{#402702}
>
> TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 591179 
>
> Committed: https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4
> Cr-Commit-Position: refs/heads/master@{#405750}

TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 591179 

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

[modify] https://crrev.com/5f19720ded2369857bee408ab48e11e7b5d28f7b/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/5f19720ded2369857bee408ab48e11e7b5d28f7b/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[add] https://crrev.com/5f19720ded2369857bee408ab48e11e7b5d28f7b/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[delete] https://crrev.com/ea6225bf8fb44062eabe9e9c99527408053180dd/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Project Member Comment 15 by bugdroid1@chromium.org, Jul 18 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1ce44d25cbafd74f034c30ddfd4b0f3caed9978d

commit 1ce44d25cbafd74f034c30ddfd4b0f3caed9978d
Author: picksi <picksi@chromium.org>
Date: Mon Jul 18 14:16:49 2016

Revert of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2154193002/ )

Reason for revert:
Relanding this has potentially caused a layout test to fail (https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/8452). The build sheriff has asked me to revert it.

Adding aelias & vmiura so they can do a more refined revert on their return from vacation.

Original issue's description:
> Reland of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2156553002/ )
>
> Reason for revert:
> Reverting change now that we have collected data from telemetry.
>
> Original issue's description:
> > Revert of Re-enable GPU Rasterization for content with any author defined viewport. (patchset #2 id:20001 of https://codereview.chromium.org/2097413003/ )
> >
> > Reason for revert:
> > This has caused an unexpectedly large regression in overall PSS (about 5MB, graph here: https://chromeperf.appspot.com/report?sid=0b072725c25637efb0d3a44383da1e24e42bbb3740a00505e4040af2173423b0&start_rev=402061&end_rev=402790).
> > The owners are both OOO and this is blocking Android release. This has already been reverted in the release branch but our infrastructure cannot gather data from the branch to confirm that the revert has had the intended result.
> >
> > This revert will allow us to confirm that this CL was the cause of the regression. Once confirmed (or otherwise) via telemetry dashboards this revert will be re-reverted.
> >
> > Original issue's description:
> > > Re-enable GPU Rasterization for content with any author defined viewport.
> > >
> > > BUG= 591179 
> > >
> > > Committed: https://crrev.com/a017b667a53b3ee5f8bc630be98c37ebf53a2339
> > > Cr-Commit-Position: refs/heads/master@{#402702}
> >
> > TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG= 591179 
> >
> > Committed: https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4
> > Cr-Commit-Position: refs/heads/master@{#405750}
>
> TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG= 591179 
>
> Committed: https://crrev.com/5f19720ded2369857bee408ab48e11e7b5d28f7b
> Cr-Commit-Position: refs/heads/master@{#405974}

TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 591179 

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

[modify] https://crrev.com/1ce44d25cbafd74f034c30ddfd4b0f3caed9978d/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/1ce44d25cbafd74f034c30ddfd4b0f3caed9978d/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[delete] https://crrev.com/2b36a509e01ac8858170a255d9c0780bdf3bde16/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[add] https://crrev.com/1ce44d25cbafd74f034c30ddfd4b0f3caed9978d/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Project Member Comment 16 by bugdroid1@chromium.org, Jul 18 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c67c6db688d379d0a5d2eb7cdd6853e7ce8da281

commit c67c6db688d379d0a5d2eb7cdd6853e7ce8da281
Author: picksi <picksi@chromium.org>
Date: Mon Jul 18 15:55:39 2016

Reland of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2155193002/ )

Reason for revert:
CL falsely blamed for red layout test. Relanding it.

Original issue's description:
> Revert of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2154193002/ )
>
> Reason for revert:
> Relanding this has potentially caused a layout test to fail (https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/8452). The build sheriff has asked me to revert it.
>
> Adding aelias & vmiura so they can do a more refined revert on their return from vacation.
>
> Original issue's description:
> > Reland of -enable GPU Rasterization for content with any author defined viewport. (patchset #1 id:1 of https://codereview.chromium.org/2156553002/ )
> >
> > Reason for revert:
> > Reverting change now that we have collected data from telemetry.
> >
> > Original issue's description:
> > > Revert of Re-enable GPU Rasterization for content with any author defined viewport. (patchset #2 id:20001 of https://codereview.chromium.org/2097413003/ )
> > >
> > > Reason for revert:
> > > This has caused an unexpectedly large regression in overall PSS (about 5MB, graph here: https://chromeperf.appspot.com/report?sid=0b072725c25637efb0d3a44383da1e24e42bbb3740a00505e4040af2173423b0&start_rev=402061&end_rev=402790).
> > > The owners are both OOO and this is blocking Android release. This has already been reverted in the release branch but our infrastructure cannot gather data from the branch to confirm that the revert has had the intended result.
> > >
> > > This revert will allow us to confirm that this CL was the cause of the regression. Once confirmed (or otherwise) via telemetry dashboards this revert will be re-reverted.
> > >
> > > Original issue's description:
> > > > Re-enable GPU Rasterization for content with any author defined viewport.
> > > >
> > > > BUG= 591179 
> > > >
> > > > Committed: https://crrev.com/a017b667a53b3ee5f8bc630be98c37ebf53a2339
> > > > Cr-Commit-Position: refs/heads/master@{#402702}
> > >
> > > TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org
> > > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > > BUG= 591179 
> > >
> > > Committed: https://crrev.com/10a77702aa0490dfaaa6e354151b24b738474fb4
> > > Cr-Commit-Position: refs/heads/master@{#405750}
> >
> > TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org
> > # Not skipping CQ checks because original CL landed more than 1 days ago.
> > BUG= 591179 
> >
> > Committed: https://crrev.com/5f19720ded2369857bee408ab48e11e7b5d28f7b
> > Cr-Commit-Position: refs/heads/master@{#405974}
>
> TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 591179 
>
> Committed: https://crrev.com/1ce44d25cbafd74f034c30ddfd4b0f3caed9978d
> Cr-Commit-Position: refs/heads/master@{#405983}

TBR=aelias@chromium.org,chrishtr@chromium.org,vmiura@chromium.org,primiano@chromium.org,alexclarke@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 591179 

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

[modify] https://crrev.com/c67c6db688d379d0a5d2eb7cdd6853e7ce8da281/third_party/WebKit/Source/core/dom/ViewportDescription.cpp
[modify] https://crrev.com/c67c6db688d379d0a5d2eb7cdd6853e7ce8da281/third_party/WebKit/Source/web/tests/ViewportTest.cpp
[add] https://crrev.com/c67c6db688d379d0a5d2eb7cdd6853e7ce8da281/third_party/WebKit/Source/web/tests/data/viewport/viewport-gpu-rasterization-disabled-without-viewport.html
[delete] https://crrev.com/ea63fff365730c62da696327aef771d74d01c78a/third_party/WebKit/Source/web/tests/data/viewport/viewport-inferred-values-do-not-trigger-gpu-rasterization.html

Issue 624645 has been merged into this issue.
Status: Fixed
This seems to be sticking in M54.
Blocking: 646032
Sign in to add a comment