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

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

29 Branch
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 years ago
Posted 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)
Assignee

Comment 1

5 years ago
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+
Assignee

Comment 4

5 years ago
(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: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.