Closed Bug 55413 Opened 24 years ago Closed 24 years ago

5 load listeners registered when one could do

Categories

(SeaMonkey :: UI Design, defect, P1)

Product:
SeaMonkey
Component:
UI Design
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: hjtoi-bugzilla, Assigned: matt)

Details

(Keywords: perf, Whiteboard: [rtm++]Fix in hand (fixed on branch 10/13)) 

This grew out of bug 51211.

In navigator.js, event listeners are added to respond to page load events:
contentArea.addEventListener("load", UpdateBookmarksLastVisitedDate, true);
contentArea.addEventListener("load", UpdateInternetSearchResults, true);
contentArea.addEventListener("load", checkForDirectoryListing, true);
contentArea.addEventListener("load", getContentAreaFrameCount, true);
contentArea.addEventListener("load", postURLToNativeWidget, true);

------- Additional Comments From Adam Lock 2000-09-22 11:38 -------

It strikes me as very inefficient to register 5 javascript handlers for the same
load event. That means five entries, five JS stubs, five marshalled calls.
Shouldn't navigator.js just register one handler and call these 5 methods from
there?

------- Additional Comments From Heikki Toivonen 2000-09-22 18:04 -------

The second patch is to navigator.js. It moves all the load event handlers into a
new function which is now the only registered event listener. I do not have the
stats yet how much this saves in time.

I also added the filter Tim Hill suggested. It does indeed filter out the frame
load events (I presume) as well. For example, I put a counter to see how many
events were filtered and on http://developer.netscape.com we filter two events.
However, I do not know if this is safe. Anyone know if the functions in
question would in fact be interested in frame loads as well?


------- Additional Comments From law@netscape.com 2000-09-22 18:46 -------

Might you have subtly changed the behavior if one of the previous handlers does
something like generate a JS exception?  Previously that would ripple back to
the event listener calling code which would (theoretically?) go ahead and call
the next listener.  Now, that would ripple back for the one listener call and
other pseudo listeners wouldn't get a crack at the load event.

That probably doesn't matter, but perhaps a try/catch around each handler call
would be safer?

Also, can you go ahead and pass the event into the 3 listeners that don't use
it?  That would match the semantics of the call currently being made to the load
listeners, right?


------- Additional Comments From Heikki Toivonen 2000-09-22 19:25 -------

Hmm, didn't think about exceptions... That's what you get for coding too long in
our subset of C++ features to use... And XUL isn't my area, really.

Well, it seems unlikely that any of the methods would throw anything except
under extreme situations (like out of memory condition). These methods have
internal try/catch code to catch normal cases. It would be nice get some data on
the speed gain before going forward with this... My Quantify refuses to
cooperate, though :(

I don't see the benefit of passing the event into the methods that don't use it.
It is a minor optimization, but still. And does JavaScript running in strict
mode then give warnings about unused parameters?


------- Additional Comments From Heikki Toivonen 2000-09-22 19:31 -------

This is just speculation, but it seems likely that adding try/catch around each
call would emulate the current model even in low memory condition because we do
a pretty good job in forgetting to check return values from functions. I cannot
be totally sure about this, of course. But then again, how expensive is the
try/catch code, would we lose the speed gain in the extra try/catch code?


------- Additional Comments From Heikki Toivonen 2000-10-02 18:44 -------

3. We could gain some performance boost by registering only one load event
listener which would then call the real load event listeners. According to
vidur's gut feeling the gain would be insignificant, vidur thinks that 10 or so
event listeners would benefit from moving them into single event listener. But
there is not hard data on this! This has been addressed in the second patch. See
comments from me and law between 2000-09-22 18:04 and 2000-09-22 19:31. I see
some additional risk in this change.

The proposed patch (with some additional stuff) is:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15371
Keywords: perf
Summary: 5 load listeners registered when one would do → 5 load listeners registered when one could do
Nominating for RTM as search is broken without this fix.
Keywords: rtm
Uh, how is search broken? This bug is only about the performance that could be 
gained by replacing the 5 separate load listeners with one load listener that 
would then call these 5 functions.
Robert sez, "Just try doing a search in Mozilla.  Sometimes you'll get each
search result duplicated several times, sometimes the results end up
disappearing... basically, its unusable."

This would be bad.  A major loss of functionality.  Matt, can you find out
what's going on here?
Assignee: don → matt
Priority: P3 → P1
Whiteboard: [rtm need info]
> Uh, how is search broken? This patch...

The proposed patch has the following line in it:
  if (event.target == window._content.document)
which helps prevent search results from reloading endlessly.

This was all covered in bug # 51211.
OK, rjc has nicely sent a patch.  Matt, can you review this and approve it?
Whiteboard: [rtm need info] → [rtm need info]Fix in hand
Please note that 51211 was broken into SEVERAL bugs. This bug is ONLY about the 
performance that could be gained by replacing the 5 load listeners with one. 
This is actually an ENHANCEMENT.

Bug 51211 was closed, and the fix was that image load event no longer travels to 
chrome. This should have fixed most of the problems with search panel. Update 
dom/src/base/nsGlobalWindow.cpp and see if the search is better...

Bug 55414 is about filtering also the frame load events. That is what

  if (event.target == window._content.document)

would be doing, and it should be handled in bug 55414.
I've been running this in my tree.  Seems fine and works fine.
sr=rjc

Looking for another reviewer.
As I implied in my previous critique of this patch, I have concerns that this
rewrite might change the semantics if one of the called handler functions raises
an exception.

That's probably not important, and I don't even know how the current
listener-calling code behaves.  So as not to be a party-pooper, I'll go ahead
and say "r=law" anyway.

I am still saying you are discussing the wrong thing in this bug. If search
results are reloading too many times, that is bug 55414.

This bug is only about the performance gained by replacing 5 event listeners
with one.

This is silly... both bugs are the same exact patch.

Let's get this fix checked in(!) instead of playing bugzilla games.
Yeah, it is the same patch (I did explain that the patch contains other stuff).
If the patch goes in as is, the other bug should be marked fixed as well. But it
is really two different bugs, and I was specifically asked by PDT to make
separate bugs. Two decisions need to be made. If the answer is yes on both bugs
I have no reasons to complain, other than that that wrong thing is being
discussed in this bug ;)

Fix in hand, reviewed and approved, so this is "rtm+".
Whiteboard: [rtm need info]Fix in hand → [rtm+]Fix in hand, reviewed and approved
Don't understand what's going on.  Is there a patch associated with this bug?
Also, how much of a performance improvement are we talking about?  Is this user
perceptable?  Marking need info
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm+ need info]Fix in hand, reviewed and approved
Patch:

http://bugzilla.mozilla.org/showattachment.cgi?attach_id=15371

Nobody has measured performance gain.

The patch also contains the proposed fix for bug 55414. It seems that frame
loads(?) are messign up the search sidebar, maybe something else as well.

PDT:  As Heikki alludes to with bug # 55414, internet searches are horked until 
this is fixed. 
Re-marking as "plus" now that the information is available.
Whiteboard: [rtm+ need info]Fix in hand, reviewed and approved → [rtm+]Fix in hand, reviewed and approved
The unquantified performance enhancement is certainly [rtm-]. However, it's
important that internet searching work, so marking [rtm++] for the absolute
minimum set of work which fixes search.
Whiteboard: [rtm+]Fix in hand, reviewed and approved → [rtm++]Fix in hand, reviewed and approved
fixed in branch.
monday will check into trunk.

updating status whiteboard to show that branch has been fixed
Whiteboard: [rtm++]Fix in hand, reviewed and approved → [rtm++]Fix in hand (fixed on branch 10/13)
Did this make it to the trunk, ever?

(I bet you can get away with the whole fix, though I'm not sure that registering
four additional load observers has any meaningful effect.)

Umm..what are we going to do here? This bug has been sitting at rtm++ for 8 days
now.

Oops
Got sidetracked.
I'll check this into the tree today.
fixed
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
rubberstamp vrfy from the branch perspective...
Keywords: vtrunk
...and a rubberstamp vrfy from the trunk pov.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Product: Core → Mozilla Application Suite
You need to log in before you can comment on or make changes to this bug.