401949 - Bug 395248 caused a Txul regression on Linux

Status: RESOLVED INCOMPLETE

(Firefox :: General, defect, P2)

Description

[Reed Loden [:reed]]

The patch in bug 395248 caused a Txul regression on Linux.
Before the patch, Txul was ~235ms. After the patch, Txul is ~243ms.

Comment 1

[Dão Gottwald [:dao]]

On Linux, the patch for bug 395248 looks effectively like this:

 #identity-box {
-  border-right: 1px solid #888;
-  background-color: white;
-  opacity: 0.9;
+  cursor: help;
+  background-color: ThreeDFace;
+  border-right: 1px solid ThreeDShadow;
 }
 
-#identity-box:hover {
-  opacity: 1.0;
+#identity-box.verifiedIdentity {
+  border-color: highlight;
 }
 
-#identity-box.verifiedIdentity {
-  background-color: #BFA;
+#identity-box:hover {
+  border-color: ThreeDDarkShadow;
 }
 
 #identity-icon-label {
-    padding: 1px 2px 2px;
-    color: black;
-    vertical-align: middle;
+  padding: 0 2px;
+  cursor: inherit;
 }

So if this points to bug 395248, it's probably a strange Core bug.

Comment 2

[Reed Loden [:reed]]

(In reply to comment #1)
> So if this points to bug 395248, it's probably a strange Core bug.

Yeah, it's definitely this patch. Nothing else was checked-in around when the Txul regression happened, and it showed up immediately after this patch landed.

Ventnor / roc: Any ideas?

Comment 3

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

I would have thought removing the opacity would speed things up ... Is it true that the hover rules don't actually get hit by Talos ever?

Comment 4

[Michael Ventnor]

Maybe its the cursor? Regardless we probably shouldn't have a special cursor for Larry's box.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Could it be the addition of nsLookAndFeel-base system colors? Seems unlikely, but I suppose we might as well try removing that and/or the cursor and see whether that affects Txul. Could try this locally, too, it's relatively easy to get good Txul numbers in my experience.

Comment 6

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

Surely we take great pains to keep the mouse pointer out of the Talos window. Right?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't know, but even if we didn't the odds that it would be over the small "Larry" icon would be pretty low, I think. I was thinking perhaps there'd be a cost to just having an icon specified, even if it isn't displayed, because we had to load it or something. I'm just making wild guesses here.

Comment 8

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

I'm pretty sure nothing will be loaded until the cursor is needed.

Comment 9

[Michael Ventnor]

Since Windows doesn't show the same regression, its probably the fact that we use border rules on Linux rather than outline rules. Although I really wonder why the outline rules weren't kept for Linux...

Comment 10

[Dão Gottwald [:dao]]

(In reply to comment #9)
> Since Windows doesn't show the same regression, its probably the fact that we
> use border rules on Linux rather than outline rules.

Border isn't known to be expensive and has been used before.

> Although I really wonder why the outline rules weren't kept for Linux...

Because we're going to use the native textfield appearance, where GTK usually doesn't draw a plain 1px border.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

This doesn't block M9, I'll leave it to component leads to determine if it blocks final.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Hey, wait, I'm a component driver. Yeah, this blocks final.

Comment 13

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: Larry looks strange on non secure page

Windows 2000, {Build ID: 2007110322}

bonsai Bug 401949
mozilla/browser/themes/winstripe/browser/browser.css
1.112 ~ 1.125

mistake?

Comment 14

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #13)
You've obviously seen the commit message, so I'm not sure why you're asking...

This is caused by the right border on the identity box - which is "fun" because it was there before and reverting it to the previous color, changing it back to border-right, using a full border and reverting the patch entirely is still a ~3.5% regression.
I don't see anything added recently that might cause issues with borders, so maybe we've hit just the right combination to cause unnecessary reflows or something. I'll try and make a testcase for this and get it under a profiler :/

Comment 15

[AKIHIRO Misaki (a.k.a Kuden)]

{Build ID: 2007110323}
2007-11-04 00:06  rflint%ryanflint.com  mozilla/browser/themes/winstripe/browser/browser.css  1.126  4/4  Bug 401949

I confirmed fix of the addition. 
There is no problem. 

Comment 16

[Alfred Kayser]

May be the 'border-color' unnecessary causes a reflow and/or redraw, as the border color of the whole box is changed, and not just the right one.

Like:
#identity-box.verifiedIdentity {
  border-right-color: highlight;
}
#identity-box:hover {
  border-right-color: ThreeDDarkShadow;
}
(or using -moz-border-end)

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
:hover rules shouldn't matter for Txul, and verifiedIdentity isn't alive yet.
That said, we might want to remove said rules altogether on Linux; changing the border color on one edge only doesn't look that great. Also, feel free to remove cursor:help, which doesn't work well because of cursor:grab over the favicon. None of that got ui-review anyway.

Comment 18

[Dão Gottwald [:dao]]

Attachment: remove cursor, verifiedIdentity and :hover stuff

This doesn't necessarily fix the regression, but reduces the number of potential culprits. Note that -moz-dialog is more commonly used than ThreeDFace in our stylesheets.

Comment 19

[Dão Gottwald [:dao]]

Attachment: remove cursor, verifiedIdentity and :hover stuff... and an ifdef

Comment 20

[Dão Gottwald [:dao]]

Attachment: remove cursor, verifiedIdentity and :hover stuff (checked in)

missed cursor:inherit

Hopefully that's it now.

Comment 21

[Dão Gottwald [:dao]]

with that patch, the remaining differences on Linux are:

 #identity-box {
-  border-right: 1px solid #888;
-  background-color: white;
-  opacity: 0.9;
+  background-color: -moz-dialog;
+  -moz-border-end: 1px solid ThreeDShadow;
 }
 
-#identity-box:hover {
-  opacity: 1.0;
-}
-
-#identity-box.verifiedIdentity {
-  background-color: #BFA;
-}
 
 #identity-icon-label {
-    padding: 1px 2px 2px;
-    color: black;
-    vertical-align: middle;
+  padding: 0 2px;
 }

Comment 22

[Asaf Romano (gone)]

Comment on attachment 287302 [details] [diff] [review]
remove cursor, verifiedIdentity and :hover stuff (checked in)

r=mano

Comment 23

[Reed Loden [:reed]]

Checking in browser/themes/winstripe/browser/browser.css;
/cvsroot/mozilla/browser/themes/winstripe/browser/browser.css,v  <--  browser.css
new revision: 1.129; previous revision: 1.128
done

Comment 24

[Ryan Flint [:rflint] (ping via IRC for reviews)]

This doesn't fix the problem as noted in comments 14 and 18.

Comment 25

[Mike Connor [:mconnor]]

-> Ryan

Comment 26

[Mike Connor [:mconnor]]

Going to move this off the blocker list, since there's no good way to know four months later if the regression was otherwise fixed.  Plus, we're 35% faster on Txul on Linux as it stands.

In the future, for perf regressions, file a bug or back out the offending patch, fixing it later is damn near impossible.

Comment 27

[Shawn Wilsher :sdwilsh]

I'm going through and marking old performance regression bugs as INCOMPLETE that are likely too old to be valid or get any traction on them.

Please re-open if you have more information or can demonstrate the regression still exists.