Last Comment Bug 398950 - Investigate contentprefsink event registration
: Investigate contentprefsink event registration
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 3 beta1
Assigned To: Myk Melez [:myk] [@mykmelez]
: Jared Wein [:jaws] (please needinfo? me)
Depends on:
  Show dependency treegraph
Reported: 2007-10-07 18:26 PDT by Robert Sayre
Modified: 2007-10-12 17:44 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

30 loads of Google Italia (18.97 KB, text/plain)
2007-10-07 18:30 PDT, Robert Sayre
no flags Details
patch v1: moves WPL event handling to nsBrowserStatusHandler (3.18 KB, patch)
2007-10-10 18:37 PDT, Myk Melez [:myk] [@mykmelez]
sayrer: review+
mconnor: approval1.9+
Details | Diff | Splinter Review

Description Robert Sayre 2007-10-07 18:26:48 PDT
It looks like the ContentPrefSink might be registering for DOM and WPL events that browser.js already listens for, and many of the WPL implementations are empty. It might be better to make the CPS get events from browser.js.
Comment 1 Robert Sayre 2007-10-07 18:30:22 PDT
Created attachment 283959 [details]
30 loads of Google Italia

This shows the JS functions executing during 30 loads of the Italian Google page. There are no scrollbars or complicated content in the page. It looks like the ContentPrefSink might be doubling our event handling XPConnect traffic. This probably won't be significant on a complicated page, but I think profile shows a significant increase in overhead for a simple page.
Comment 2 Myk Melez [:myk] [@mykmelez] 2007-10-08 12:03:12 PDT
Hmm, indeed, it looks like we could save some cycles by integrating ContentPrefSink's functionality into the nsBrowserStatusHandler in browser.js.
Comment 3 Myk Melez [:myk] [@mykmelez] 2007-10-10 18:37:28 PDT
Created attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

Here's a patch that moves the WPL event handling over to nsBrowserStatusHandler.  I don't move the DOM event handling over, as nothing in browser.js listens for that DOM event.

I still don't have a working dtrace-enabled build, so I haven't tested this for performance impact.
Comment 4 Robert Sayre 2007-10-10 18:40:50 PDT
Comment on attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

yes, let's try this.
Comment 5 Myk Melez [:myk] [@mykmelez] 2007-10-10 18:51:48 PDT
Comment on attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

Requesting approval on this low-risk patch that simply moves location change listening from ContentPrefSink to nsBrowserStatusHandler for a possible perf win.
Comment 6 Myk Melez [:myk] [@mykmelez] 2007-10-10 23:36:07 PDT
Checked in at approximately 11:35pm PDT 2007-10-10.

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.866; previous revision: 1.865
Checking in browser/base/content/browser-contentPrefSink.js;
/cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v  <--  browser-contentPrefSink.js
new revision: 1.4; previous revision: 1.3
Comment 7 Myk Melez [:myk] [@mykmelez] 2007-10-12 17:44:12 PDT
Note: preliminary results showed some improvement, although it was within the margin of error, but an unrelated regression shortly after I checked this in makes it impossible to see if there was sustained improvement.

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