Last Comment Bug 398360 - speed up onSecurityChange
: speed up onSecurityChange
Status: RESOLVED FIXED
: perf
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Robert Sayre
:
:
Mentors:
Depends on: 889428
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-02 22:14 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-07-02 19:21 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
attempt #1 (1023 bytes, patch)
2007-10-02 22:16 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
attempt #1, revised (1.03 KB, patch)
2007-10-03 07:34 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review
trust onLocationChange (2.69 KB, patch)
2007-10-04 10:25 PDT, Robert Sayre
no flags Details | Diff | Splinter Review
set the host in onSecurityChange (2.71 KB, patch)
2007-10-04 11:36 PDT, Robert Sayre
no flags Details | Diff | Splinter Review
preserve the try block (2.75 KB, patch)
2007-10-04 11:41 PDT, Robert Sayre
no flags Details | Diff | Splinter Review
set the host to null if there's a failure (2.78 KB, patch)
2007-10-04 11:48 PDT, Robert Sayre
no flags Details | Diff | Splinter Review
use reportError and change boolean check for style (2.82 KB, patch)
2007-10-04 11:55 PDT, Robert Sayre
no flags Details | Diff | Splinter Review
make sure the assert doesn't fire when this._host is null (2.81 KB, patch)
2007-10-04 12:33 PDT, Robert Sayre
gavin.sharp: review+
Details | Diff | Splinter Review
with gavin's fix (2.87 KB, patch)
2007-10-04 12:44 PDT, Robert Sayre
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-02 22:14:36 PDT
 
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-02 22:16:53 PDT
Created attachment 283307 [details] [diff] [review]
attempt #1

This avoids us going through the content window to get the host, which I imagine might avoid some security-wrapper overhead, and also avoids nsLocation::GetURI's call to fixupURI which shouldn't be needed for our purposes. It maintains the special case for JAR URIs added in bug 53124.
Comment 2 Robert Sayre 2007-10-02 22:52:52 PDT
10 runs of loading cnn.com talos record (14 calls to onSecurityChange per pageload).

Doing nothing (entire method body commented):

  FUNCTION                         COUNT    AVG(us)      SUM(us)
  nsIWebProgressListener::onSecu    140        147        20625

Current Impl:

  FUNCTION                         COUNT    AVG(us)      SUM(us)
  nsIWebProgressListener::onSecu    140       3645       510320

Attachment 283307 [details] [diff]:

  FUNCTION                         COUNT    AVG(us)      SUM(us)
  nsIWebProgressListener::onSecu    140        502        70391
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-03 07:34:08 PDT
Created attachment 283358 [details] [diff] [review]
attempt #1, revised

Hmm, we might the createExposableURI after all, to deal with wyciwyg URIs (see bug 179269 where it was added to nsLocation::GetURI). This should be relatively cheap for non-wyciwyg URIs, though (an additional SchemeIs/GetUserName call+overhead of calling the method). Rob, can you do the same test with this patch applied?
Comment 4 Robert Sayre 2007-10-03 20:15:41 PDT
Comment on attachment 283358 [details] [diff] [review]
attempt #1, revised

This patch seems to lose the speed advantage. I tried a couple times with and without patch.
Comment 5 Robert Sayre 2007-10-03 21:14:38 PDT
On irc, Gavin and I dicussed the fact that we get an onLocationChange event, so we might not need to check the location so much in onSecurityChange. However, these two events are fired by different code, so we'll need to determine whether tying them together causes a regression.

It looks like nsDocShell::SetCurrentURI almost always fires it, except for one caller:

nsDocShell::CreateContentViewer
  PRBool onLocationChangeNeeded = OnLoadingSite(aOpenedChannel, PR_FALSE);
    nsDocShell::OnNewURI
      nsDocShell::SetCurrentURI
Comment 6 Robert Sayre 2007-10-04 10:25:16 PDT
Created attachment 283568 [details] [diff] [review]
trust onLocationChange

After investigation and consulting with bz, it looks like the only time the docshell URI changes without firing onLocationChange is error pages and subframes, so we should be safe.

I gave this function a name so I could see perf differences separate from xpconnect traffic. It looks like the current code is about twice as slow as the patch in the common case, but can also be pathologically slow occasionally.

Current code:

   FUNCTION                        COUNT    AVG(us)      SUM(us)
   browser_onSecChange               140       3811       533653

   browser_onSecChange                               
           value  ------------- Distribution ------------- count    
          131072 |                                         0        
          262144 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@    130      
          524288 |@@                                       8        
         1048576 |                                         0        
         2097152 |                                         0        
         4194304 |                                         0        
         8388608 |                                         0        
        16777216 |                                         0        
        33554432 |                                         0        
        67108864 |                                         1        
       134217728 |                                         0        
       268435456 |                                         1        
       536870912 |                                         0

With patch:

   FUNCTION                        COUNT    AVG(us)      SUM(us)
   browser_onSecChange               140        263        36892

   browser_onSecChange                               
           value  ------------- Distribution ------------- count    
           65536 |                                         0        
          131072 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@            103      
          262144 |@@@@@@@@@@                               36       
          524288 |                                         1        
         1048576 |                                         0
Comment 7 Robert Sayre 2007-10-04 11:10:44 PDT
If this works out, we can move this optimization into nsBrowserStatusFilter.cpp, and avoid crossing xpconnect a bunch.
Comment 8 Robert Sayre 2007-10-04 11:36:07 PDT
Created attachment 283583 [details] [diff] [review]
set the host in onSecurityChange

gavin points out that this is better because it will deal with the silly URI schemes we litter the product with. :)
Comment 9 Robert Sayre 2007-10-04 11:41:09 PDT
Created attachment 283584 [details] [diff] [review]
preserve the try block
Comment 10 Robert Sayre 2007-10-04 11:48:34 PDT
Created attachment 283586 [details] [diff] [review]
set the host to null if there's a failure
Comment 11 Robert Sayre 2007-10-04 11:55:49 PDT
Created attachment 283588 [details] [diff] [review]
use reportError and change boolean check for style
Comment 12 Robert Sayre 2007-10-04 12:33:43 PDT
Created attachment 283593 [details] [diff] [review]
make sure the assert doesn't fire when this._host is null

and change the comment a little
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-04 12:39:24 PDT
Comment on attachment 283593 [details] [diff] [review]
make sure the assert doesn't fire when this._host is null

>Index: browser/base/content/browser.js

>+      if (this._host && this._host != contentHost) {

Compare against undefined, to still catch the case where getting the host failed and it's null?
Comment 14 Robert Sayre 2007-10-04 12:44:50 PDT
Created attachment 283595 [details] [diff] [review]
with gavin's fix
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2007-10-04 12:57:54 PDT
Comment on attachment 283595 [details] [diff] [review]
with gavin's fix

a=beltzner
Comment 16 Robert Sayre 2007-10-04 17:58:05 PDT
looks good for a 1%+ Tp win on windows, at least.
Comment 17 Dão Gottwald [:dao] 2007-10-05 05:13:42 PDT
isn't gBrowser.contentWindow.location.host more expensive than gBrowser.currentURI.host? I think Gavin mentioned that once.
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2007-10-05 12:43:04 PDT
(In reply to comment #17)
> isn't gBrowser.contentWindow.location.host more expensive than
> gBrowser.currentURI.host? I think Gavin mentioned that once.

Yes, but that's only because it doesn't do fixup (bug 179269), or dig into JAR URIs (bug 53124), and those are both things we need to do. Doing the equivalent in the chrome JS (see attachment 283358 [details] [diff] [review]) cancelled out any wins from changing the way we get the URI (see comment 4).

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