Investigate contentprefsink event registration

RESOLVED FIXED in Firefox 3 beta1

Status

()

Firefox
Preferences
P1
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Robert Sayre, Assigned: myk)

Tracking

unspecified
Firefox 3 beta1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
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.
(Assignee)

Comment 2

10 years ago
Hmm, indeed, it looks like we could save some cycles by integrating ContentPrefSink's functionality into the nsBrowserStatusHandler in browser.js.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → Firefox 3 M9
(Assignee)

Comment 3

10 years ago
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.
Attachment #284401 - Flags: review?(sayrer)
(Reporter)

Comment 4

10 years ago
Comment on attachment 284401 [details] [diff] [review]
patch v1: moves WPL event handling to nsBrowserStatusHandler

yes, let's try this.
Attachment #284401 - Flags: review?(sayrer) → review+
(Assignee)

Comment 5

10 years ago
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.
Attachment #284401 - Flags: approval1.9?

Updated

10 years ago
Attachment #284401 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 6

10 years ago
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
done
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
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

10 years ago
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.
You need to log in before you can comment on or make changes to this bug.