189384 - Composer window slow to load

Status: VERIFIED FIXED

(SeaMonkey :: Composer, defect)

Description

[Simon Fraser [no longer active]]

I see a 4-5 second lockup when bringing up the composer window in a debug build,
every time (not just the first time).

Some sampling shows that we're updating commands, which is calling
GetElemenetsByTagName a lot (which is known to be slow). Sampler data coming

Comment 1

[Simon Fraser [no longer active]]

Attachment: Part of sampler trace

Comment 2

[Simon Fraser [no longer active]]

This looks like the culprit:

http://lxr.mozilla.org/seamonkey/source/editor/ui/composer/content/editorUtilities.js#224

230     var editorList = tmpWindow.document.getElementsByTagName("editor");
231 
232     // This will change if we support > 1 editor element
233     if (editorList && editorList.length > 0)
234       return editorList[0];

Comment 3

[[e-mail]]

Attachment: Proposed patch

Comment 4

[Kathleen :Brade]

Comment on attachment 112292 [details] [diff] [review]
Proposed patch

r=brade

In this scenario we have a huge win:
 open new composer window
 as soon as you see window appear start counting
 stop counting when you see the caret blink

Before this patch, I could count to 5 or 6.

After this patch I rarely get to 2.

Comment 5

[Kathleen :Brade]

-->Neil

Comment 6

[[e-mail]]

Comment on attachment 112292 [details] [diff] [review]
Proposed patch

Looking for this perf win for 1.3

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 112292 [details] [diff] [review]
Proposed patch

a=asa (on behalf of drivers) for chechin to 1.3beta.

Comment 8

[[e-mail]]

Fix checked in.

Comment 9

[Boris Zbarsky [:bzbarsky]]

A quick question... if you remove that .length check on the list, how does that
affect tje perf?  getElementsByTagName should be pretty fast as long as you
don't ask it how many there are total (since that requires walking the entire
DOM tree)....

Just curious.

Comment 10

[Kathleen :Brade]

bzbarsky--if you look at the sampler trace, most of the time seems to be spent
in the getElementsByTagName call.  Not knowing much about the code, I'd guess
it's more expensive to build the list of elements than to count the list (in
this case we expect it to be a small number and probably only 1 at the present
time).  I would guess that it wouldn't make much difference.

Comment 11

[Boris Zbarsky [:bzbarsky]]

brade, the list is built lazily.  It is not built until you get .count or try to
get an actual element... If you get an element, we only walk far enough to find
that element, not through the whole DOM.  So .count should be by far the most
expensive part of that.... (I see the sampler trace, and that's why I asked my
question; I'm confused by that trace.)

Comment 12

[Simon Fraser [no longer active]]

Fixed stack trace MIME type to be plaintext. bz: the sampler trace shows hit
frequencies for functions on the stack. Many of the samples hit
nsXULDocument::GetElementsByTagName.

Comment 13

[Suresh]

Reopening this bug. This checkin caused commercial tree blocker bug
(http://bugscape.mcom.com/show_bug.cgi?id=22116)

Commercial tree uses
<editor id="ComposeWnd".....> and a bunch of places that access this editor using
document.getElementById("ComposeWnd")

I'm gonna try replacing "ComposeWnd" with "content-frame" (once my build completes)

I've backed out this checkin for now to open the tree. 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Um... you backed out a checkin from the _Mozilla_ tree because it caused
problems in _commercial_ due to the fact that commercial and Mozilla have
diverged in this area of the code?  That seems a little odd to me...

Comment 15

[Kathleen :Brade]

bz--I change the code to be like this:
230     var editorList = tmpWindow.document.getElementsByTagName("editor");
231 
232     // This will change if we support > 1 editor element
233     if (editorList && editorList[0])
234       return editorList[0];

The result was not noticeable at all; it takes about 5-6 seconds for me to get
to the point of being able to type *after* the window has already appeared. 
With Neil's fix it was down to 1-2 second range.

Comment 16

[Boris Zbarsky [:bzbarsky]]

brade, thank you!  The XUL impl of getElementsByTagName didn't get upgraded when
the HTML/XML one did...  I'll try to post a patch fixing that tonight.

(in the meantime brade is testing a quick hack that will show how worthwhile
that approach is).

Comment 17

[Kathleen :Brade]

In hindsight, getting the editor by id is not the correct solution.  <sigh>
All <editors> should not have to have the same id, etc.

I did test what bz suggested and it seems to work fine everywhere (not that I
have tested everything).  Composer window opening is back to 1-2 seconds.  I
have replied to e-mails, removed people from cc list on messages, browsed, done
form submits.  I am sure there is more to test but it seems to work fine for me
so far.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: how about we make comment 11 true?

Summary:

1)  Make nsGenericElement and nsDocument have basically the same code (minor
    tweaks)
2)  Copy/paste this code into nsXULElement/nsXULDocument (obligatory cursing
    about using inheritance here)
3)  Make the change brade mentions in comment 15

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

I've done some testing of this.  Nothing seems to be broken.  Nothing _should_
be broken -- this is a fairly safe fix.

Could people verify that this fixes the composer perf problem?

jst?  sicking?	Could you review?

Comment 20

[[e-mail]]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

>+    if (editorList && editorList[0])
You'll have to use .item(0) to avoid JS strict warnings :-(

Comment 21

[Kathleen :Brade]

I tested bz's patch on my macho debug build and it works fine.  I see no
problems and the Composer window opens in less than half the time (compared to a
build without his patch).

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

The XUL code uses 4-space indentation, thus the code you add to those files
should follow that.

sr=jst if you re-indent the nsXUL* changes.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

>+  if (!aNamespaceURI.Equals(NS_LITERAL_STRING("*"))) {
>+    nsContentUtils::GetNSManagerWeakRef()->GetNameSpaceID(aNamespaceURI,
>+                                                          nameSpaceId);
> 
>-        rv = GetElementsByTagName(root, aLocalName, nsid,
>-                                  elements);
>+    if (nameSpaceId == kNameSpaceID_Unknown) {
>+      // Unknown namespace means no matches, we create an empty list...
>+      NS_GetContentList(this, nsnull, kNameSpaceID_None, nsnull,
>+                        getter_AddRefs(list));
>+      NS_ENSURE_TRUE(list, NS_ERROR_OUT_OF_MEMORY);
>     }
>+  }
> 
>-    return NS_OK;
>+  if (!list) {


Nit: I would just put a |return CallQI(..);| at the end of the inner |if| and
remove the |if (!list)|. Feel free to leave it as is though. (same for the
function on xul-element)

>     var editorList = tmpWindow.document.getElementsByTagName("editor");
> 
>     // This will change if we support > 1 editor element
>-    if (editorList && editorList.length > 0)
>+    if (editorList && editorList[0])

Can editorList ever be null without an error being thrown? I'd just remove the
"editorList &&".

r=sicking

Comment 24

[Kathleen :Brade]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

Can we take this patch for 1.3?  I've been running with it for 2 days now with
no problems.  The benefits are huge performance increases in Composer (and
probably some other performance wins elsewhere across the product).  In my
build, this patch reduces the time between composer window open and time you
can type from 5-6 seconds to 1.5-2 seconds.  It also affects typing performance
and other areas within composer which are harder to measure precisely.	The
patch fixes the broken implementation of nsXULDocument::GetElementsByTagName to
be more like the html peers.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 112837 [details] [diff] [review]
how about we make comment 11 true?

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 26

[Boris Zbarsky [:bzbarsky]]

taking

Comment 27

[Boris Zbarsky [:bzbarsky]]

fixed.

Comment 28

[Asko Tontti]

There was increase in btek's Tp numbers. Something to do with this bug?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Possibly.  I spent some time looking through all callers of getElementsByTagName
in our codebase, and none of them should affect Tp (especially none of them that
would call it on a XUL document).

Also, none of the other tinderboxes show anything....  So I supect this is a
hiccup on btek.  But if someone wants to back this patch out to test, go for it.
 I will not be in a position to do that for at least a week.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Ah, the vagaries of life.... Backed out for now; we'll see how that goes.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Well, I backed it out.  That did nothing.  Then a cycle later btek dropped.
Looking back over the times overnight, it did some jumping around then too.

So I'm relanding this once the tree opens, assuming I still have CVS access
then. If it hasn't happened by tonight, someone please do it.

Comment 32

[Andreas Kunz]

bzbarsky relanded it at 01/31/2003 15:21.
So shouldn't this bug be resolved as "fixed" again?

Comment 33

[Boris Zbarsky [:bzbarsky]]

maybe... I'm still seeing Tp weirdness on btek, unfortunately, correlated with
the relanding.  I'm trying to decide whether to leave the patch in or to back it
out and deal later (in a few weeks at least).

Comment 34

[Boris Zbarsky [:bzbarsky]]

ok, fixed.  I've filed bug 191589 on some changes that may help the Tp issue,
assuming it's real.

Comment 35

[Chris Petersen]

Marking verified.