Bug 72078 - [Chromium] Compositing with full sized fixed div causes heavy repaints
Summary: [Chromium] Compositing with full sized fixed div causes heavy repaints
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Xianzhu Wang
URL:
Keywords:
Depends on: 75018 75632 75637 75638
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-10 17:26 PST by James Simonsen
Modified: 2012-02-10 09:39 PST (History)
11 users (show)

See Also:


Attachments
Reduced test case (1.33 KB, text/html)
2011-11-10 17:26 PST, James Simonsen
no flags Details
Test case with negative z-index (1.75 KB, text/html)
2011-12-21 16:05 PST, Xianzhu Wang
no flags Details
patch (12.71 KB, patch)
2012-01-03 18:31 PST, Xianzhu Wang
no flags Details | Formatted Diff | Diff
patch v2 (fixed style issues) (12.85 KB, patch)
2012-01-03 18:52 PST, Xianzhu Wang
jamesr: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2011-11-10 17:26:40 PST
Created attachment 114609 [details]
Reduced test case

This is a reduced case from crosbug.com/21332, which is that techcrunch.com scrolls slowly on ChromeOS. There may be many problems, but at least one of them is that we're repainting every time the user scrolls. One of the troublemakers is a 100%x100% fixed div (#finalizer) that's causing a heavy repaint every time we scroll.

I've attached a simple test case that demonstrates this. If you instrument the code, you'll see that we have to repaint every time you scroll, even though the bright green div isn't visible.
Comment 1 James Robinson 2011-11-10 17:30:51 PST
The paint rects are significantly smaller if the div is partially onscreen - for example add top:95%. Very strange.
Comment 2 Xianzhu Wang 2011-12-21 16:05:02 PST
Created attachment 120232 [details]
Test case with negative z-index

In my experiments, I must add a z-index CSS property with a negative value (like 'z-index: -100) to the #finalizer, does the issue occur.

Another experiment is to add 'top: 0px' to #finalizer, then the finalizer will be just under the other contents of the body. It's reasonable to repaint in this case when scrolling. So the excessive repaint might be because WebKit thinks #finalizer is overlapping with the whole body.
Comment 3 Xianzhu Wang 2011-12-27 17:44:13 PST
I believe the patch to https://bugs.webkit.org/show_bug.cgi?id=75018 should resolve this issue. My investigation showed that before that patch, all layers are reinitialized and repainted on very scroll because the scrollbar layers are recreated and added into the layers hierarchy.
Comment 4 Xianzhu Wang 2011-12-27 18:04:38 PST
Sorry for misleading. Just verified that https://bugs.webkit.org/show_bug.cgi?id=75018 make this issue better, but doesn't totally resolve this issue.
Comment 5 Xianzhu Wang 2012-01-03 18:31:21 PST
Created attachment 121032 [details]
patch

Only verified on chromium-android, not on chromeos. Wondering if this CL could resolve the whole problem.
Comment 6 WebKit Review Bot 2012-01-03 18:33:10 PST
Attachment 121032 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:110:  The parameter name "viewportSize" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:176:  The parameter name "viewportSize" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Xianzhu Wang 2012-01-03 18:52:05 PST
Created attachment 121036 [details]
patch v2 (fixed style issues)
Comment 8 James Robinson 2012-01-04 20:54:27 PST
Comment on attachment 121036 [details]
patch v2 (fixed style issues)

View in context: https://bugs.webkit.org/attachment.cgi?id=121036&action=review

I'm guessing the issue here is texture use that's higher than the reclaim limit but lower than the absolute limit, right?  Looks like this without this patch we're reducing to the reclaim limit on every frame, regardless of what's visible.

This change seems reasonable to me, although it's effectively making the reclaim limit much weaker. I think you definitely need tests for a subtle behavior change like this.

> Source/WebKit/chromium/ChangeLog:12
> +        No new tests as it doesn't change any functionality.

you definitely are changing functionality, so a test would be nice
Comment 9 Vangelis Kokkevis 2012-01-05 10:05:35 PST
Comment on attachment 121036 [details]
patch v2 (fixed style issues)

I think this is a reasonable change and more in the spirit of how the reclaim limit should be functioning. Its role is not to actively go and delete textures over the limit but only do so if we need to allocate a new texture.  

There is still a remaining issue that we don't actually protect textures that we already have and need for the current frame from being deleted. So if we are over the reclaim limit and we get a new layer, there is a good chance we'll go and delete a texture from another layer that's still needed if that other layer comes later in the hierarchy traversal.

Maybe what we should be doing instead is to first reserve all the existing textures that we need for the frame before we try to create new ones.
Comment 10 Xianzhu Wang 2012-01-05 10:52:04 PST
(In reply to comment #8)
> (From update of attachment 121036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=121036&action=review
> 
> I'm guessing the issue here is texture use that's higher than the reclaim limit but lower than the absolute limit, right?

Yes.

> This change seems reasonable to me, although it's effectively making the reclaim limit much weaker. I think you definitely need tests for a subtle behavior change like this.
> 
> > Source/WebKit/chromium/ChangeLog:12
> > +        No new tests as it doesn't change any functionality.
> 
> you definitely are changing functionality, so a test would be nice

I'll add unit test for the patch.

As this bug has several reasons, and the patch just resolves one of them, I'll create separate bugs for each reason.
Comment 11 Xianzhu Wang 2012-01-05 10:53:44 PST
Add bug 75018 as the depended bug as it used to be the main reason of this bug.
Comment 12 Xianzhu Wang 2012-01-05 17:09:04 PST
Comment on attachment 121036 [details]
patch v2 (fixed style issues)

The updated patch will be in bug 75632.
Comment 13 Xianzhu Wang 2012-02-10 09:39:29 PST
All depending bugs fixed. Closing.