Closed Bug 398360 Opened 17 years ago Closed 17 years ago

speed up onSecurityChange

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: Gavin, Assigned: sayrer)

References

Details

(Keywords: perf)

Attachments

(2 files, 7 obsolete files)

Attached patch attempt #1 (obsolete) — Splinter Review
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.
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
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?
Assignee: nobody → gavin.sharp
Attachment #283307 - Attachment is obsolete: true
Status: NEW → ASSIGNED
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.
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
Attached patch trust onLocationChange (obsolete) — Splinter Review
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
Attachment #283568 - Flags: review?(gavin.sharp)
If this works out, we can move this optimization into nsBrowserStatusFilter.cpp, and avoid crossing xpconnect a bunch.
Attached patch set the host in onSecurityChange (obsolete) — Splinter Review
gavin points out that this is better because it will deal with the silly URI schemes we litter the product with. :)
Attachment #283568 - Attachment is obsolete: true
Attachment #283583 - Flags: review?(gavin.sharp)
Attachment #283568 - Flags: review?(gavin.sharp)
Attached patch preserve the try block (obsolete) — Splinter Review
Attachment #283583 - Attachment is obsolete: true
Attachment #283584 - Flags: review?(gavin.sharp)
Attachment #283583 - Flags: review?(gavin.sharp)
Attachment #283584 - Attachment is obsolete: true
Attachment #283586 - Flags: review?(gavin.sharp)
Attachment #283584 - Flags: review?(gavin.sharp)
Attachment #283586 - Attachment is obsolete: true
Attachment #283588 - Flags: review?(gavin.sharp)
Attachment #283586 - Flags: review?(gavin.sharp)
and change the comment a little
Attachment #283588 - Attachment is obsolete: true
Attachment #283588 - Flags: review?(gavin.sharp)
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?
Attachment #283593 - Flags: review+
Attached patch with gavin's fixSplinter Review
Attachment #283593 - Attachment is obsolete: true
Attachment #283595 - Flags: approval1.9?
Comment on attachment 283595 [details] [diff] [review]
with gavin's fix

a=beltzner
Attachment #283595 - Flags: approval1.9? → approval1.9+
looks good for a 1%+ Tp win on windows, at least.
Assignee: gavin.sharp → sayrer
Status: ASSIGNED → NEW
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
isn't gBrowser.contentWindow.location.host more expensive than gBrowser.currentURI.host? I think Gavin mentioned that once.
(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).
Keywords: perf
You need to log in before you can comment on or make changes to this bug.