1045864 - 8% Linux CART regression on Inbound (fx34) Jul 25 from rev bd4287c14070

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

we have linux32 and linux64 regressions for cart from bug 1039020:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd4287c14070

Here is a graph:
linux32: http://mzl.la/1rLHuew
linux64: http://mzl.la/1nHlfVc

I did some retriggers to verify this isn't related to noise or solar flares:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&fromchange=94fea719a746&tochange=a0a40196da87&jobname=Ubuntu%20HW%2012.04%20x64%20mozilla-inbound%20talos%20svgr

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Matt, can you take a look into this as you were the author of the patch?

Comment 2

[Matt Woodrow (:mattwoodrow)]

I think this is because the extra layers trigger component alpha, and then we have to flatten with BasicLayers. The flattening isn't free, and the resulting layer tree might well be worse than we had initially.

I guess we should disable this code for BasicLayers, and maybe the equivalent for canvas backgrounds.

That would cover component alpha layers created by text scrolling over a fixed background, I wonder how many other cases we care about? I wonder if we can just do that and get rid of the flattening code.

Can you think of any obviously important cases where we'd really want subpixel-AA text and wouldn't get it that way roc?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> I think this is because the extra layers trigger component alpha, and then
> we have to flatten with BasicLayers. The flattening isn't free, and the
> resulting layer tree might well be worse than we had initially.
> 
> I guess we should disable this code for BasicLayers,

OK.

> and maybe the equivalent for canvas backgrounds.

Not so sure. It's quite common to have text over opaque background over fixed background. I think we should leave that alone.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Disable for BasicLayers

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8466881 [details] [diff] [review]
Disable for BasicLayers

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

::: layout/generic/nsImageFrame.cpp
@@ +1351,5 @@
> +    nsDisplayImage* displayImage = static_cast<nsDisplayImage*>(item);
> +    container = displayImage->GetContainer(aManager, aBuilder);
> +    if (!container) {
> +      return LAYER_NONE;
> +    }

I do not understand this change.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Disable for BasicLayers v2

Sorry, accidentally qref'd some unrelated code into the previous patch.

Comment 7

[Matt Woodrow (:mattwoodrow)]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/751b5a710e04

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/751b5a710e04

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

thanks for fixing this!

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

I have confirmed this is fixed for linxu32 and linux64, thanks!

On a related note, I am seeing alerts that showed up later on osx 10.6 and 10.8 which appear to be the same changeset (it is sort of fuzzy).  Is there any chance the original patch could affect osx and the fix not work for osx?

Comment 11

[Matt Woodrow (:mattwoodrow)]

That seems unlikely, the linux issue was very specific to not having layers acceleration enabled.