237735 - "Edit Attachment As Comment" in bugzilla has severe performance problems

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Darin Fisher]

"Edit Attachment As Comment" in bugzilla has severe performance problems.

Tested with:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040315 Firefox/0.8.0+

Steps to reproduce:
1) goto http://bugzilla.mozilla.org/attachment.cgi?id=143917&action=edit
2) click "Edit Attachment As Comment"

Result:
100% CPU for a long time (long enough that one might think the browser hung). 
Firefox froze up for nearly 25 seconds on my 2 GHz laptop.  That's long enough
for most users to get annoyed and likely force quit the app.

With Mozilla 1.6, the amount of time required is closer to 15 seconds.  That's a
big difference.

Even if this is not a regression, I think it is something that should be
profiled and fixed.  It's clearly indicating a significant performance penalty
somewhere.

BTW, I looked to see if this bug was already on file, but I couldn't find
anything.  Also, I could be way off on blaming this on Editor  Please help me
triage this to the right component if I'm wrong.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Even if this is not a regression

It's not.

> It's clearly indicating a significant performance penalty somewhere.

My money is on the JS the page runs, but I'll do a profile to verify.

Comment 2

[Darin Fisher]

it looks like the XMLSerializer is involved.  perhaps all the performance cost
can be attributed to it.

Comment 3

[Darin Fisher]

(In reply to comment #1)
> > Even if this is not a regression
> 
> It's not.

right, but can you explain why it appears to have gotten even slower?  that
smells like a regression.  of course this has always been slow :-/


> > It's clearly indicating a significant performance penalty somewhere.
> 
> My money is on the JS the page runs, but I'll do a profile to verify.

cool.. thanks!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Hmm.. So apparently I was wrong.  Of the 616367 total hits on that testcase,
535930 are under nsEditor::DoTransaction.  The main callers of this are
nsEditor::CreateNode and nsEditor::InsertNode.  Part of the problem, it seems,
is that editor has to break up the input into umpteen bazillion textnodes and
stick in <br>s in between them....

Tracing down from DoTransaction, I end up with 240788 hits under
InsertElementTxn::DoTransaction, which almost all go through
nsHTMLDivElement::InsertBefore.  Under that, most of the time is actually spent
under nsCSSFrameConstructor::ContentAppended, which ends up calling down into
nsBlockFrame::AddFrames, which calls nsLineBox::RFindLineContaining which calls
 nsLineBox::IndexOf, which is where a good chunk of the time is spent.  The
other big chunk is in nsLineBox::LastChild.  65% of the total time on this
testcase is spent in (not under, in) those two functions.

Looking under CreateElementTxn::DoTransaction, it also calls through
nsHTMLDivElement::InsertBefore and the rest is as above.  CreateElement is
creating the <br>s, while InsertElement is inserting the textnodes, looks like
(based on the callers of CreateTxnForInsertElement and CreateTxnForCreateElement.

So the problem seems to be that we're ending up with a really long line in the
block.  I wonder why that is.  Do <br>s not lead to new lines until we reflow? 
Because that would certainly do it....

I really wish Editor had a way to use the batching apis nsIDocument and
nsIContent provide here... :(

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Profile (about 1.5megs uncompressed)

Comment 6

[Boris Zbarsky [:bzbarsky]]

If, as a test (because it's not the right thing to do), I make
nsLineLayout::TreatFrameAsBlock return true for br frames, the time on the
testcase drops by a factor of 3.  LastChild() and RFindLineContaining()
disappear from the profile for all intents and purposes.  There are still O(N^2)
algorithms hanging about (eg the calls to nsEditor::GetChildOffset that <br>
creation in editor makes), though....

So maybe the right thing to is to push a new line when appending content after a
<br> frame instead of waiting till reflow to do it?

Of course the _really_ right thing to do, imo, is to make the plaintext editor
actually edit _plaintext_ (textnodes, with white-space: pre or pre-wrap or
whatever, and newlines being newlines, instead of <br> nodes).  Would be much
ligher-weight than what we do now, if nothing else.

> but can you explain why it appears to have gotten even slower? 

Actually, yes.  jst recently changed DOM to report ContentAppended for
insertBefore() calls when the insertion is at the end of the DOM node.  So
before we only used to spend lots of time in RFindLineContaining, now we do that
_and_ do the LastChild() thing first.

It's funny that we were being saved by GetPrimaryFrameFor (on the prevsibling)
being fast compared to LastChild() (because there was a frame map entry for that
prevsibling frame, presumably).

Comment 7

[Boris Zbarsky [:bzbarsky]]

So just to make it clear what is and what isn't a regression, the algorithm was
and is O(N^2).  The constant doubled recently.  For small N, other parts of the
algorithm, O(N) ones, predominate, and doubling the constant here let us reduce
constants there.  

On a separate note, I wonder whether editor could implement interruptible
batching... coming up for air every so often to do things like handle events and
let reflows happen (sorta like the way the parser does during incremental page
loading).  Or would that break people who expect .value setting to be atomic?

Comment 8

[(no longer active)]

The STR is comment 0 now finishes almost in an instant!