Closed Bug 1000266 Opened 7 years ago Closed 7 years ago

don't purge the canvas background image cache all the time, making it useless

Categories

(Core :: Layout, defect)

29 Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
Bug 818643 was a problem where we didn't purge the cache enough. The fix was to basically always purge the cache. Making it useless. We should either fix this or get rid of the cache, I couldn't tell exactly what led to it's creation based on the code history/bug.

This is super hacky.
Attachment #8411077 - Flags: review?(matt.woodrow)
This does have a measurable perf impact on the cases it affects though (otherwise there'd be no point in all the hackyness!).
This was originally added because we don't have component alpha layers with BasicLayers. Instead we have to flatten layers together as best we can to try keep subpixel AA enabled for text.

Unfortunately this means we have to repaint the whole page on scroll if we have text on top of a fixed background.

I'm surprised this regressed, since it was a big win for tscrollx when it was added.
Comment on attachment 8411077 [details] [diff] [review]
patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2687,5 @@
>    } else {
>      // Let the display item check for geometry changes and decide what needs to be
>      // repainted.
> +
> +    if (oldGeometry->ComputeInvalidationRegion() == aGeometry->ComputeInvalidationRegion() &&

A comment (similar to the one in the commit message) would be really good here.
Attachment #8411077 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> This was originally added because we don't have component alpha layers with
> BasicLayers. Instead we have to flatten layers together as best we can to
> try keep subpixel AA enabled for text.
> 
> Unfortunately this means we have to repaint the whole page on scroll if we
> have text on top of a fixed background.

That's what I figured.

> I'm surprised this regressed, since it was a big win for tscrollx when it
> was added.

This patch gets about 4% on linux tscrollx, and I'm guessing that's the only place we could see a difference because everywhere else is using non-basic layers. Maybe tscroll has changed since then.

Strangely putting a return at the start of the canvas background image paint function gets less of a win than that. Which I cannot explain.
https://hg.mozilla.org/mozilla-central/rev/671620466491
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.