Closed Bug 801918 Opened 8 years ago Closed 8 years ago

Frequent unnecessary invalidation of the entire content area while scrolling

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 - ---

People

(Reporter: jwatt, Assigned: jrmuizel)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: [Snappy])

Attachments

(2 files, 1 obsolete file)

Attached file testcase
When scrolling the attached testcase, we frequently invalidate the entire content area. Try scrolling up and down while keeping the black bordered div entirely in the content area. If scrolled very, very slowly, then we only invalidate the area that is revealed. If however scrolling is sped up (again, keeping the div in the content area), we start to invalidate the entire content area every so often. It seems like the faster you scroll, the more often the entire viewport is invalidated.
This happens when we attempt to draw into multiple quadrants of our rotated GL texture here:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#537

We then throw out 3/4 quadrants of the old texture here:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/ThebesLayerOGL.cpp#597

We should be able to copy all of the old data to the new texture if wrapping works correctly. It wasn't working correctly when that code was added.

Someone with more GL knowledge should be able to fix this fairly easily, cc'ing some gfx people.
FWIW this seems to happen a lot, and as well as being a nasty perf hit in its own right it makes it more difficult to debug other invalidation bugs.
Keywords: perf
Blocks: 801949
I think this is a bug in BlitTextureImage:

http://hg.mozilla.org/mozilla-central/file/be64e9425b0b/gfx/gl/GLContext.cpp#l2144

This intersection won't take wrapped texture data into account.

If we fix BlitTextureImage such that it works correctly with rects that are intended to wrap around, then we can stop adding area to mRegionToDraw in ThebesLayerOGL.
Assignee: nobody → jmuizelaar
This patch ends up calling BlitTextureImage three extra times instead of changing BlitTextureImage to support rotated buffers.

Supporting rotated buffers in BlitTextureImage is somewhat tricky because of TiledTextureImage, and calling it three more times shouldn't be too bad (it should be better than having to repaint)

The rectangle manipulation code is a bit hairy.
Attachment #673196 - Flags: feedback?(matt.woodrow)
Comment on attachment 673196 [details] [diff] [review]
A somewhat ugly patch that accomplishes this

Review of attachment 673196 [details] [diff] [review]:
-----------------------------------------------------------------

How often are we using TiledTextureImage for a ThebesLayerOGL (if ever)? I thought it was only for mobile, and we've switched to tiling at the ThebesLayer level instead.

If we are actually hitting this path with a TiledTextureImage, then this approach seems pretty reasonable to me.

I am of course concerned by the loss of my wonderful ascii art.
I believe B2G uses TiledTextureImage with ThebesLayerOGL still.
Attachment #673196 - Flags: feedback?(matt.woodrow) → feedback+
Attachment #673196 - Attachment is obsolete: true
Attachment #677360 - Flags: review?(matt.woodrow)
Comment on attachment 677360 [details] [diff] [review]
Somewhat cleaner patch

Review of attachment 677360 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

I would still prefer to see the ascii art diagrams retained though.
Attachment #677360 - Flags: review?(matt.woodrow) → review+
Is this something tickled by DLBI landings on 18 ? Or has this been the default behavior ever since ? Looking for a compelling reason to track this on 18 and have a patch on aurora at this point.
No, it's not a regression.

It is, however, an annoying perf bug with a simple fix :)
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> No, it's not a regression.
> 
> It is, however, an annoying perf bug with a simple fix :)

Please nominate for uplift in that case. Since this isn't a new regression, no need to track for a specific release. Not clear that this has enough user impact to skip the trains though.
+1 for the FF18 uplift here. Scrolling perf goodness is always welcome :)
Whiteboard: [Snappy]
https://hg.mozilla.org/mozilla-central/rev/b5c86318a3ee
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 810521
Depends on: 810531
It sounds like this should get backed out.
Agreed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2804a15008b2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla19 → ---
No longer blocks: 810521
Depends on: 810521
https://hg.mozilla.org/integration/mozilla-inbound/rev/9707fdeafe85

Try again, this time with correct component alpha.
https://hg.mozilla.org/mozilla-central/rev/9707fdeafe85
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.