Open Bug 68074 Opened 19 years ago Updated 6 months ago

Lots of CreateInstances of nsEventListenerManagers at startup

Categories

(Core :: User events and focus handling, defect)

defect
Not set

Tracking

()

People

(Reporter: sfraser_bugs, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [nav+perf])

Attachments

(1 file, 1 obsolete file)

Some analaysis of an XPCOM log for startup and display of a simple HTML page in a 
Mozilla browser window shows that we create 1054 instances of 
nsEventListenerManagers. This seems a lot, and perhaps could be optimized.
Most of these nsEventListenerManagers seem to be created by nsXULElement's call 
to element->AddScriptEventListener(). At 80 bytes a pop, this is quite a space 
hog too.
Is our XUL using a bunch of onclick attributes, or some such?  What's further up
the stack in these calls to AddScriptEventListener?

/be
Or worse, a XUL template with event handlers inside the <template>? 
alecf/ben/mcafee, you should do a pass through navigator.xul and its overlays. 
(sspitzer, same for mail.) Search for the `datasources' attribute; make sure 
that there are no event handlers contained inside a <template>.
found a bunch of badness in the mailnews xul.

mailWindowOverlay.xul
    oncommand="MsgMoveMessage(event.target)"
    oncommand="MsgCopyMessage(event.target)"
    oncommand="MsgGetMessagesForAccount(event)"

messengercompose.xul 
    oncommand="MessageFcc(event.target)"

msgFolderPickerOverlay.xul
    oncommand="PickedMsgFolder(event.target,'msgNewFolderPicker')"
    etc.  (basically, every folder picker that I wrote is evil)

FilterListDialog.xul
    onclick="onToggleEnabled(event);"

pref-composing_messages.xul
    oncreate="dump('CREATE MENU NOW\n');"

I'll go start a new bug to track the clean up of the mailnews xul.
thanks for the heads up waterson / sfraser.
the mail xul is covered by #68174
Depends on: 68174
perf
Keywords: perf
Here's my list:
navigator.xul
 
oncommand="OpenBookmarkURL(event.target,document.getElementById('BookmarksMenu').database)"
/>
  oncommand="OpenBookmarkURL(event.target,
document.getElementById('innermostBox').database);"/>
oncommand="OpenBookmarkURL(event.target,
document.getElementById('innermostBox').database)"/>

navigatorOverlay.xul
  oncommand="selectLocale(event)" 
  oncommand="applyTheme(event.target)"
  oncommand="OpenBookmarkURL(event.target,
document.getElementById('BookmarksMenu').database)"

internetresults.xul
  oncommand="doEngineClick(event, this);" 

search-editor.xul
  oncommand="return chooseCategory(this);"
  oncommand="chooseCategory(this);" 
  oncommand="saveEngines();"

sidebarOverlays.xul
  oncommand="SidebarTogglePanel(event.target);"

charsetOverlay.xul
  oncommand="SelectDetectors( event )"

taskbarOverlay.xul
  oncommand="rdf:http://home.netscape.com/NC-rdf#content"

tasksOverlay.xul
  oncommand="ShowWindowFromResource(event.target)"

ok here's what needs to be done:
1) you need to find the most logical container for the template - i.e. the
<tree> or <menulist> - and move the onclick or oncommand handler there
2) look at the event handler code, and figure out if it was dependent on the
command handler being in the child node.. look for references to event.target,
and more importantly, test it out. 9 times out of 10, it's not dependent and you
can just move the event handler.
mcafee, I'm giving this bug to you since you've been working on this 
Assignee: saari → mcafee
slee: what I mumbled in the meeting was to replace the CreateInstance of the 
nsEventListenerManager with a call to NS_NewEventListenerManager if you can (i.e. 
if the things are being created in the content DLL. This avoids a trip through 
XPCOM.
Keywords: patch
I went through the browser files and found some cases
where we could move oncommand stuff outside of templates.
I tested each one of these by first breaking it (removing
the command handler), then adding it back into the new
place to restore the functionality.

Some of the JS functions were used in other JS, and sometimes
it looked like the JS needed the handler in the template code.
I managed to get changes for navigator.xul, navigatorOverlay.xul,
and sidebarOverlays.xul. Files I had trouble with:

internetresults.xul : ? I couldn't understand how to test this
search-editor.xul:    Other JS usages of chooseCategory(), more changes needed.
taskbarOverlay.xul:   Didn't work
tasksOverlay.xul:     ShowWindowFromResource() not working on linux, couldn't test.

these changes look great - you don't need to add the comment about the handlers
moving to the template - they shouldn't have been there in the first place, and
everywhere else they're outside the template already
sr=alecf on the latest patch if you remove those comments
ok thanks alec
OS: Mac System 8.5 → All
2nd patch checked in.
Blocks: 7251
nav triage team:

Chris, are there still any remaining issues? Why is this bug still open?
This is open per smfr's comment about avoiding xpcom/dll trip
nav triage team:

Marking nsbeta1+, mozilla0.9.2, reassigning to pchen
Assignee: mcafee → pchen
Keywords: nsbeta1+
Target Milestone: --- → mozilla0.9.2
nav triage team:

Marking p3 and mozilla0.9.3
Priority: -- → P3
Target Milestone: mozilla0.9.2 → mozilla0.9.3
QA contact updated
QA Contact: gerardok → madhur
Whiteboard: [nav+perf]
nav triage team:

Avoiding trip through xpcom not that critical. Marking nsbeta1- and future
Keywords: nsbeta1+nsbeta1-
Target Milestone: mozilla0.9.3 → Future
No longer blocks: 7251
Blocks: 7251
->default assignee
Assignee: pchen → joki
QA Contact: madhur → rakeshmishra
QA Contact: rakeshmishra → trix
.
Assignee: joki → saari
QA Contact: trix → ian
Attachment #27366 - Attachment is obsolete: true
Comment on attachment 27430 [details] [diff] [review]
2nd patch, resolves cvs conflicts
[Checkin: Comment 25]

(In reply to comment #16)
> 2nd patch checked in.

A (code and comment) modified version was checked in:
[
2006-09-13 22:46	mcafee%netscape.com 	mozilla/suite/browser/navigator.xul 	1.278
2006-09-13 22:46	mcafee%netscape.com 	mozilla/suite/browser/navigatorOverlay.xul 	1.179
2006-07-27 07:47	mcafee%netscape.com 	mozilla/suite/common/sidebar/sidebarOverlay.xul 	1.56
]

NB: The (2006) timestamps are unexpected ... There seems to be something wrong with Bonsaï (for this, and other, checkins) :-<
Attachment #27430 - Attachment description: 2nd patch, resolves cvs conflicts → 2nd patch, resolves cvs conflicts [Checkin: Comment 25]
Assignee: saari → nobody
Priority: P3 → --
QA Contact: ian → events
Target Milestone: Future → ---
blocking1.9=?:
not mandatory, but as it is marked as 'perf' and seems easy enough to do...
Flags: blocking1.9?
(In reply to comment #26)
> blocking1.9=?:
> not mandatory, but as it is marked as 'perf' and seems easy enough to do...
> 

What is left to do here?   Likely not going to block but would *happily* take a patch.
(In reply to comment #27)
> What is left to do here?

I don't know precisely yet:
see comment 12 and bug 68174 comment 19...
When we know what we want you can re-nom but not clear to me we should block on this
Flags: blocking1.9? → blocking1.9-
Currently when starting FF, ~320 ELMs are created.

This seems to be in wrong component. Firefox-generic might be a better one, if
the count of event listener attributes in UI should be reduced?
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.