"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.
> 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.
it looks like the XMLSerializer is involved. perhaps all the performance cost can be attributed to it.
(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!
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... :(
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).
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?
Depends on: 240291
Depends on: 240292
Depends on: 240851
Depends on: 240884
Depends on: 240933
The STR is comment 0 now finishes almost in an instant!
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by bug 240933]
Target Milestone: --- → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.