Closed Bug 402668 Opened 17 years ago Closed 17 years ago

reduce Ts by initially hiding editBookmarkPanel and autocomplete panel [was: 2% Ts regression from bug #399664]

Categories

(Firefox :: Address Bar, defect, P1)

Product:
Firefox
Component:
Address Bar
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3 beta2

People

(Reporter: moco, Assigned: moco)

References

Details

(Keywords: perf, regression)

Attachments

(1 file, 4 obsolete files)

updated patch
5.36 KB, patch
moco
: review+
mtschrep
: approval1.9+
Details | Diff | Splinter Review 
2% Ts regression from bug #399664

Txul appears to have regressed slightly, but the numbers are up and down.

Linux bl-bldlnx03 Dep fx-linux-tbox perf test
Ts. 937 -> 957 (20 ms regression, approx 2%)

MacOSX Darwin 8.8.4 bm-xserve08 Dep Universal Nightly
865 - > 885 (20 ms regression, approx 2%)

WINNT 5.1 bl-bldxp01 Dep fx-win32-tbox perf test
3125 ms - 3155 ms -> 3172 ms (50 ms regression, approx 2%)
Blocks: 399664
No longer depends on: 399664
Keywords: perf, regression
No longer blocks: 399664
Depends on: 399664
Blocks: 399664
No longer depends on: 399664
Perhaps we can try making the <panel> hidden="true" (display: none) initially to see whether that helps Ts. The trick is finding out where we should unhide it (openAutocompletePopup, maybe?).
thanks for the suggestion, gavin.

I'll go try out your idea and report back with the impact on Ts (on my mac)
Assignee: nobody → sspitzer
gavin, doing:

<panel type="autocomplete-richlistbox" chromedir="&locale.dir;" id="PopupAutoCompleteRichResult" noautofocus="true" style="display: none"/>

and:

      <method name="openAutocompletePopup">
        <parameter name="aInput"/>
        <parameter name="aElement"/>
        <body>
          <![CDATA[
          // initially the PopupAutoCompleteRichResult panel has style of "display: none"
          // to avoid impacting startup / new window performance
          var panel = document.getElementById("PopupAutoCompleteRichResult");
          if (panel.style.display == "none")
            panel.style.display = "";

Works and appears to help with Ts (at least on my mac), but my local numbers aren't the most consistent.

It might be worth doing this trick for both the tree autocomplete popup and the richlistbox autocomplete popup, to help Ts.
Status: NEW → ASSIGNED
Yeah, seems like something worth trying. Use the "hidden" attribute, though (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/xul.css&rev=1.107#23 )
> Use the "hidden" attribute, though

thanks gavin, will do.

working on my mac (with an optimized build and the startup test (see
http://lxr.mozilla.org/mozilla/source/tools/performance/startup/startup-unix.pl
http://lxr.mozilla.org/mozilla/source/tools/performance/startup/startup-test.html) to get some real numbers to figure out how much this will actually save us for the existing panel and for the panel I'd be adding for bug #399664.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M10
from my mac optimized builds, here are some numbers (in ms)

I took the best Ts out of 10 runs (like tinderbox does), and then did that 3 times to get an average (to increase my confidence in the result)

what we have on the trunk right now:
1 panel, not hidden:  1443 (1444, 1442, 1442)

If we apply gavin's trick for the trunk:
1 panel, hidden: 1435 (1437, 1436, 1432)

what I checked in last night:
2 panels, not hidden: 1460 (1462, 1460, 1457)

If I apply gavin's trick to my patch:
2 panels, both hidden: 1437 (1433, 1438, 1439)

If we remove both panels:
no panels: 1429 (1427, 1429, 1432)

note, all of this was with the rest of my change for bug #399664, just tweaking browser.xul

I'm going to re-run tests 1,2,5 above on a clean, mac tree and verify the numbers.
> If we remove both panels:
> no panels: 1429 (1427, 1429, 1432)

which leads to trying to dynamically add them, to increase Ts / Txul.
Comment on attachment 287630 [details] [diff] [review]
patch (to be applied on top of bug #399664)

>+          panel.removeAttribute("hidden");

panel.hidden = false;?
> I'm going to re-run tests 1,2,5 above on a clean, mac tree and verify the
> numbers.

1 panel, not hidden, 1450 (1445, 1459, 1447)
If we apply gavin's trick for the trunk: 1442 (1444, 1446, 1436)
If we remove the panel completely (best possible case): 1438 (1437, 1436, 1442)

> panel.hidden = false;

thanks dao, I'll do that instead.

this will decrease Ts and we can use this trick to avoid Ts for bug #399664 as well.
Attachment #287630 - Attachment is obsolete: true
Attachment #287658 - Flags: review?(gavin.sharp)
(In reply to comment #11)
> Created an attachment (id=287658) [details]
> dynamically create the panel in delayedStartup()

Hmm, I kind of worry that someone this will break if someone (the user, or other code) tries to use autocomplete before delayedStartup() fires. Perhaps we could move this popup creation code to the autocomplete "popup" getter, and have create the popup, and set attributes it based on attributes on the input (textbox)?

Or maybe we should just apply this "hide initially, unhide when showing" trick to other <popup>s/<panel>s in browser.xul, to offset the cost of the new panel, and then file a bug on trying to reduce the impact of panels/popups on Ts without these front-end hacks. Neil (or someone else) might have more ideas along the lines of bug 394887.
Flags: blocking-firefox3? → blocking-firefox3+
gavin, thanks for the feedback.

based on your feedback and the numbers I got while testing (which show the Ts benefit of this trick), I agree, we should:

1)  just do the "hide initially, unhide when showing" trick for the existing autocomplete panel (like the identity popup)
2)  extend this fix to also do this trick for the editBookmarkPanel (to save more Ts / Txul)
3)  extend this fix for the other popups / panels
4)  for #399664, update the patch to do the same trick for panel I'd be adding
5)  file a bug, as you suggest (on how to do this without this trickery)

If that sounds good, I'll go do that for M10.
> panel.hidden = false;

I'll make sure to do that (like we do in handleIdentityClick, for the identity popup), thanks dao.
Yeah, sounds good.
Attached patch updated patch (obsolete) — — Splinter Review 

        Attachment #287658 -
        Attachment is obsolete: true

        Attachment #287658 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 287786 [details] [diff] [review]
updated patch

>Index: toolkit/content/widgets/autocomplete.xml

>       <method name="openAutocompletePopup">
>         <parameter name="aInput"/>
>         <parameter name="aElement"/>
>         <body><![CDATA[
>+          // initially the panel is hidden
>+          // to avoid impacting startup / new window performance
>+          var panel = document.getElementById("PopupAutoComplete");
>+          panel.hidden = false;

This is wrong - toolkit code can't assume that the panel has that specific ID. You can get the popup from the input (aInput.popup), I guess... Putting this code in the generic nsIAutocompleteInput implementation feels kind of wrong, but I suppose there isn't really anywhere else to put it.

        Attachment #287786 -
        Flags: review-

    
    

    
  
Oh, I guess we could put it in the popup binding that's specific to the URL bar, by overriding "openAutocompletePopup" from autocomplete-result-popup (autocomplete.xml) in urlbar-result-popup (urlbarBindings.xml).

    
    

    
  
Thanks for the feedback, gavin.

I was hoping there was a clean way to call an overridden method (on a base binding), but there isn't (yet)

see bug #373652

http://developer.mozilla.org/en/docs/XBL:XBL_1.0_Reference:Binding_Implementations#Inheritance_of_Implementations

But I have an approach, will attach a new patch for review.



    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #287805 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 287805 [details] [diff] [review]
patch

>Index: browser/base/content/urlbarBindings.xml

>+      <method name="openAutocompletePopup">
>+        <parameter name="aInput"/>
>+        <parameter name="aElement"/>
>+        <body><![CDATA[
>+          // initially the panel is hidden
>+          // to avoid impacting startup / new window performance
>+          var panel = document.getElementById("PopupAutoComplete");
>+          panel.hidden = false;

It would still be better to use |var panel = aInput.popup;|, I think... does that not work for some reason?

>+          this._openAutocompletePopup(aInput, aElement);

Could add a comment that that method is defined on the base binding.

        Attachment #287805 -
        Flags: review?(gavin.sharp) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        updated patch
        — 
          — Splinter Review
      

    


  
> It would still be better to use |var panel = aInput.popup;|

fixed.  That works fine, I just forgot to implement that suggestion.

I also added the comment:

+          // this method is defined on the base binding
+          this._openAutocompletePopup(aInput, aElement);

carrying over gavin's review.

        Attachment #287786 -
        Attachment is obsolete: true

        Attachment #287805 -
        Attachment is obsolete: true

        Attachment #287863 -
        Flags: review+
Summary: 2% Ts regression from bug #399664 → reduce Ts by initially hiding editBookmarkPanel and autocomplete panel [was: 2% Ts regression from bug #399664]

    
  

        Attachment #287863 -
        Flags: approval1.9? → approval1.9+

    
    

    
  
fixed.  I'll watch tbox and see if the Ts gains I saw locally also show up there.

additionally, there are some spin off bugs to be logged as well as using this approach for bug #399644.

Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.60; previous revision: 1.59
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.388; previous revision: 1.387
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.41; previous revision: 1.40
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.88; previous revision: 1.87
done

Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED

    
    

    
  
backed out, tree was closed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [has patch][ready to land]
Status: REOPENED → ASSIGNED
Priority: -- → P1

    
    

    
  
fix re-landed.  will report back with Ts/Txul numbers from tbox, as well as log the spin off bugs.

Checking in browser/base/content/browser-places.js;
/cvsroot/mozilla/browser/base/content/browser-places.js,v  <--  browser-places.js
new revision: 1.62; previous revision: 1.61
done
Checking in browser/base/content/browser.xul;
/cvsroot/mozilla/browser/base/content/browser.xul,v  <--  browser.xul
new revision: 1.390; previous revision: 1.389
done
Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.43; previous revision: 1.42
done
Checking in toolkit/content/widgets/autocomplete.xml;
/cvsroot/mozilla/toolkit/content/widgets/autocomplete.xml,v  <--  autocomplete.xml
new revision: 1.90; previous revision: 1.89
done

Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED

    
    

    
  
some numbers from tbox.  

keep in mind, getting data from tbox is hardly a science, unless something grossly goes wrong (or right), and this was a minor change.  with that said, here are a few numbers:

WINNT 5.1 bl-bldxp01 Dep fx-win32-tbox perf test
Txul: 621 -> 593 ms (5.5% improvement) 
Ts: 2093 -> 2031 ms (3% improvement)

Linux bl-bldlnx03 Dep fx-linux-tbox perf test
Txul:  243 -> 240 ms (1.3% improvement)
Ts:  941 -> 925 ms (1.7% improvement)

MacOSX Darwin 8.8.4 bm-xserve08 Dep Universal Nightly
Txul: 152 -> 152 ms (0% improvement, this number varies betwee 155 - 130 both before and after my change, so can't tell)
Ts: 883 -> 845 ms (4.4% improvement)
Whiteboard: [has patch][ready to land]

    
  
Depends on: 405235

    
    

    
  
gavin, via boris, came up with a better way to do this, see bug #405235

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: