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

Issue 591179 link

Starred by 8 users
Status: Fixed
Owner:
vmi...@chromium.org
Closed: Oct 2016
Cc:
█ aerotwist@chromium.org
pdr@chromium.org
█ aelias@chromium.org
ericrk@chromium.org
enne@chromium.org
esprehn@chromium.org
chrishtr@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug

Blocked on:
issue 472813
Blocking:
issue 646032



Sign in to add a comment

Reconsider the ganesh viewport trigger on mobile

Project Member Reported by pdr@chromium.org, Mar 1 2016 
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

Comment 1 by vmi...@chromium.org, Mar 9 2016 

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()"?

Comment 7 by vmi...@chromium.org, Apr 6 2016

Cc: pdr@chromium.org
I think we're going to revert this change for M51 due to memory regressions on WebView:  Issue 596881 .

Comment 8 by aelias@chromium.org, Apr 6 2016

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

Comment 13 by chrishtr@chromium.org, Jul 15 2016

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

Comment 17 by primiano@chromium.org, Jul 26 2016 

Issue 624645 has been merged into this issue.

Comment 18 by vmi...@chromium.org, Oct 3 2016

Status: Fixed (was: Assigned)
This seems to be sticking in M54.

Comment 19 by vmi...@chromium.org, Oct 3 2016

Blocking: 646032
►

Sign in to add a comment