386741 - re-enable new location bar when Ts/Txul regression is addressed

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

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

The patch for bug 366797 (with the eTLD usage disabled) seems to have caused a Ts regression on bl-bldlnx01 (about 2-3%) and bl-bldxp01 (about 2%), and a Txul regression on xserve08 (about 5-6%), bl-bldlnx01 (about 4%) and bl-bldxp01 (about 8%).

Comment 1

[Robert Sayre]

What's the rationale for filing a follow-up bug rather than backing it out?

Comment 2

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

Attachment: attempt #1

I'm not sure that this will make a big difference, but XBL field construction has been a pain point in the past, so perhaps this will help. I've landed this to see what impact it has.

Comment 3

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

I wanted a place to put the patches I'm going to use to fix it. I'll back it out if I can't fix it in a reasonable time frame.

Comment 4

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

I had to disable the new location bar again given that our attempts at fixing it didn't make a significant difference on Txul. It looks like they might have helped Ts, but it might just be that our Ts numbers are unreliable. I also had to back out the browser.js part of bug 386746 to not regress the clickselectsall behavior.

I'm morphing this bug because I don't want to re-open the humongo bug 366797.

Comment 5

[Dão Gottwald [:dao]]

I don't know what else to do about it.

Comment 6

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

I'm not sure what to do about it either. It's possible that there's currently no way to implement this behavior without a regression of Txul, which means that we need to either make up for it in some other way, or accept the regression as necessary. I think the latter will be a hard sell, especially if the 8% number is accurate on Windows (I doubt it is, given the timer granularity, but perhaps our new performance tools can help get better numbers here).

Comment 7

[Dão Gottwald [:dao]]

Attachment: attempt #2

Comment 8

[Dão Gottwald [:dao]]

Attachment: attempt #2.5

Comment 9

[Dão Gottwald [:dao]]

Attachment: attempt #2.6

Comment 10

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

Comment on attachment 270882 [details] [diff] [review]
attempt #2.6

This looks like a nice attempt, the removal of the extra labels and the nsIURL stuff will probably be a nice win. Why are you inlining a bunch of functions, though? That shouldn't have any effect on Txul, and makes the code harder to read.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Why are you inlining a bunch of functions,
> though? That shouldn't have any effect on Txul, and makes the code harder to
> read.

You said XBL field construction could be inefficient and I thought that could somehow apply to methods as well. Also, I saw little value in a distinct _copy method that is used only once.
Anyway, let me know if you want me to bring some methods back. If you want them all, you can review the previous patch (attempt #2.5).

Comment 12

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

Comment on attachment 270882 [details] [diff] [review]
attempt #2.6

>Index: urlbarBindings.xml

>-                         onmousedown="focus();">
>+                    onmousedown="focus();" hidden="true">

Sorry I didn't catch this earlier, but what does this "onmousedown" actually do? I'm not sure I understand why it's needed.

>-            this._initPrettyView();
>+            initPrettyView();

Please undo this change.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> (From update of attachment 270882 [details] [diff] [review])
> >Index: urlbarBindings.xml
> 
> >-                         onmousedown="focus();">
> >+                    onmousedown="focus();" hidden="true">
> 
> Sorry I didn't catch this earlier, but what does this "onmousedown" actually
> do? I'm not sure I understand why it's needed.

Because of the timeout in the mouseover handler, you can theoretically click into the Location bar before it has switched to the editable mode.

Comment 14

[Dão Gottwald [:dao]]

Attachment: attempt #2

Comment 15

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

Landed that patch on the trunk, will try re-enabling the binding either later tonight or tomorrow.

mozilla/browser/base/content/urlbarBindings.xml 	1.13

Comment 16

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

I re-enabled the location bar binding earlier today. The test results look better:

The Ts regression on bl-bldxp01 now looks to be less than 1%, and the Txul regression on that machine looks like it's about 4%. It's hard to put much faith in these numbers, though, given the lack of timer precision.

Ts on bl-bldlnx01 is a bit strange - it seems to have increased steadily over the past 7 days, which includes the times this binding was disabled. The re-enabling this morning doesn't seem to have had an effect. Txul on that machine seems to have jumped about 3% after the initial landing, but now seems to have dropped down to pre-landing levels.

Txul on xserve08 seems to have jumped approximately 3% (5ms over ~177ms baseline).

Given those numbers, it looks to me like the Ts impact has been mostly eliminated, but the Txul regression is still around 3% (vs. the 4-8% on initial landing). It'd be great if someone would sanity check my reading of the performance numbers.

I'm inclined to say that we should leave it in and make any attempts to further improve performance in separate bugs (I have a few ideas that I can file and patch). I think backing it out at this point is just going to make it harder to iterate quickly and prevent us from getting the feedback we need on the new behavior. I think that if it comes down to it, we'd probably be willing to accept a small Txul regression if it's necessary to maintain this functionality (though I don't really know who's call it is to make). I also think there are other, juicier targets for helping Txul (bug 354048 comes to mind, hopefully I can resurrect the patch I had for that some time soon).

Thoughts?

Comment 17

[Robert Sayre]

(In reply to comment #16)
>
> I'm inclined to say that we should leave it in and make any attempts to further
> improve performance in separate bugs (I have a few ideas that I can file and
> patch). I think backing it out at this point is just going to make it harder to
> iterate quickly and prevent us from getting the feedback we need on the new
> behavior.

All patches are trying to accomplish something. 

> I think that if it comes down to it, we'd probably be willing to
> accept a small Txul regression if it's necessary to maintain this functionality

Maybe "we" are. I don't think this patch is worth it.

> 
> Thoughts?

There are two ways to improve Txul/Ts performance: 

1.) Do less.
2.) Make everything faster by improving low-level code.

We aren't very good at doing #1--it looks to me like we execute an absurd amount of code when we open a window. So, the only real perf wins we get here are things like Cairo improvements, image decoding, etc. Those result in 10-20% improvements when they land, but we're not faster than we used to be. We're treading water.

Comment 18

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

The location bar binding has been re-enabled for a while now. The code that's landed now is not necessarily what we're going to be shipping (it's going to be tweaked for M7 in bug 388135).

Comment 19

[Robert Sayre]

The resolution is WONTFIX. You didn't fix the regression.

Comment 20

[Dão Gottwald [:dao]]

Er, the regression is "addressed" as per comment 16 and the binding is re-enabled. According to the summary, this bug is fixed.

Comment 21

[Robert Sayre]

(In reply to comment #20)
> Er, the regression is "addressed" as per comment 16 and the binding is
> re-enabled. According to the summary, this bug is fixed.
> 

Oh, I thought this bug was about actually fixing a performance regression. But it isn't, and the location bar is definitely turned on.

Comment 22

[Mike Schroepfer]

> The Ts regression on bl-bldxp01 now looks to be less than 1%, and the Txul
> regression on that machine looks like it's about 4%. It's hard to put much
> faith in these numbers, though, given the lack of timer precision.
> 
> Txul on xserve08 seems to have jumped approximately 3% (5ms over ~177ms
> baseline).
> 
> Given those numbers, it looks to me like the Ts impact has been mostly
> eliminated, but the Txul regression is still around 3% (vs. the 4-8% on initial
> landing). It'd be great if someone would sanity check my reading of the
> performance numbers.
> 

Have we made progress on this?  IMHO 3% Txul regression is still not acceptable given TXUL/Ts for 1.9 is still way above 1.8.

Comment 23

[Dão Gottwald [:dao]]

There's another patch in bug 388030 that eliminates the scrollbox.