55413 - 5 load listeners registered when one could do

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P1)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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 [e-mail] 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

Comment 1

[Robert John Churchill]

Nominating for RTM as search is broken without this fix.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 3

[don]

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?

Comment 4

[Robert John Churchill]

> 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.

Comment 5

[don]

OK, rjc has nicely sent a patch.  Matt, can you review this and approve it?

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 7

[matt]

I've been running this in my tree.  Seems fine and works fine.
sr=rjc

Looking for another reviewer.

Comment 8

[Bill Law]

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.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 10

[Robert John Churchill]

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

Let's get this fix checked in(!) instead of playing bugzilla games.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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 ;)

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

a=ben. 

Comment 13

[don]

Fix in hand, reviewed and approved, so this is "rtm+".

Comment 14

[Michael La Guardia]

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

Comment 15

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 16

[Robert John Churchill]

PDT:  As Heikki alludes to with bug # 55414, internet searches are horked until 
this is fixed. 

Comment 17

[don]

Re-marking as "plus" now that the information is available.

Comment 18

[Phil Peterson]

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.

Comment 19

[matt]

fixed in branch.
monday will check into trunk.

Comment 20

[Jim Roskind]

updating status whiteboard to show that branch has been fixed

Comment 21

[Mike Shaver (:shaver emeritus)]

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.)

Comment 22

[Jason Eager]

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

Comment 23

[matt]

Oops
Got sidetracked.
I'll check this into the tree today.

Comment 24

[matt]

fixed

Comment 25

[sairuh (rarely reading bugmail)]

rubberstamp vrfy from the branch perspective...

Comment 26

[sairuh (rarely reading bugmail)]

...and a rubberstamp vrfy from the trunk pov.