Horrible performance with a small textarea element and huge lines of text

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: roc)

Tracking

(Depends on: 1 bug, {hang, perf})

Trunk
x86
Mac OS X
hang, perf
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.0.x +

Firefox Tracking Flags

(blocking2.0 -, status1.9.1 wanted)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
Created attachment 416163 [details]
Test case

Sam had this problem with his Firefox profile where restoring his old session practically hanged his browser.  He kindly sent me his sessionstore.js, and I managed to find the problem page by looking at the URLs in his session one by one.

The problem happens on http://new.heimat.de/home/bibdat/isdat.htm, which is a near 2MB HTML file containing a default-sized textarea with two extremely long lines of text.

I am attaching the file as plain text for viewing, but beware that loading it may hang your browser for a few minutes.

Sharking Firefox reveals that we're spending ~95% of our time in this memchr call:

<http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#2157>
It's only a few seconds to load that testcase for me. Are you using the HTML5 parser?
(Reporter)

Comment 2

9 years ago
(In reply to comment #1)
> It's only a few seconds to load that testcase for me. Are you using the HTML5
> parser?

No.  I just tested it again on my nightly (which is an optimized build) and it took near 1.5 minutes to load.

Are you testing this on Mac?
(Reporter)

Comment 3

9 years ago
I found a possible dupe, bug 228146, which was resolved INVALID.  I also asked Jeff to load this, and he saw a similar perf problem.

Safari on Mac loads the page just fine.
(Reporter)

Updated

9 years ago
Keywords: hang
See Also: → bug 228146
(Reporter)

Updated

9 years ago
Duplicate of this bug: 228146
I'm testing on Mac, yes. In a debug build. When I did a frame dump, your testcase has the text broken into a lot of 4096-char text nodes, which is what the old parser does. Is that what you see?
status1.9.1: --- → wanted
Flags: wanted1.9.0.x+
(Reporter)

Comment 6

9 years ago
When I did a frame dump, I got a huge frame tree containing numerous line frames for each line of text as displayed inside the textarea.  Here is a sample of the frame tree (the whole tree is too large to attach):

                      Block(div)(-1)@0x25d1a0e0 [view=0xa110f10] [content=0xa110090] {0,0,9500,2580} [state=01d06010] [overflow=0,0,9500,106043280] sc=0x25d19f70(i=126242,b=0) pst=:-moz-scrolled-content<
                        line 0x25d1a2c8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,0,9360,840} ca={59,0,9361,840} <
                          Text(0)@0x25d1a280[0,20,F]  next=0x25d1a780 next-continuation=0x25d1a780 {60,0,9360,840} [state=c0604200] SELECTED [content=0xa110030] [overflow=-1,0,9361,840] sc=0xdba2510 pst=:-moz-non-element<                            "Aaby, P; Garly, ML; "
                          >
                        >
                        line 0x25d1a7c8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,840,5616,840} ca={60,840,5640,840} <
                          Text(0)@0x25d1a780[20,12,F]  next=0x25d1a860 prev-continuation=0x25d1a280 next-continuation=0x25d1a860 {60,840,5616,840} [state=84604204] SELECTED [content=0xa110030] [overflow=0,0,5640,840] sc=0xdba2510 pst=:
-moz-non-element<                            "Bale, C, et "
                          >
                        >
                        line 0x25d1a8a8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,1680,7488,840} ca={60,1680,7500,840} <
                          Text(0)@0x25d1a860[32,16,F]  next=0x25d1a8d0 prev-continuation=0x25d1a780 next-continuation=0x25d1a8d0 {60,1680,7488,840} [state=84604204] SELECTED [content=0xa110030] [overflow=0,0,7500,840] sc=0xdba2510 pst=
:-moz-non-element<
                            "al.*Survival of "
                          >
                        >
                        line 0x25d1a918: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,2520,8892,840} <
                          Text(0)@0x25d1a8d0[48,19,F]  next=0x25d1a940 prev-continuation=0x25d1a860 next-continuation=0x25d1a940 {60,2520,8892,840} [state=80604004] [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<
                            "previously measles-"
                          >
                        >
                        line 0x25d1a988: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,3360,7020,840} <
                          Text(0)@0x25d1a940[67,15,F]  next=0x25d1a9b0 prev-continuation=0x25d1a8d0 next-continuation=0x25d1a9b0 {60,3360,7020,840} [state=80604004] [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<                            "vaccinated and "
                          >
                        >
                        line 0x25d1a9f8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,4200,3744,840} ca={59,4200,3745,840} <
                          Text(0)@0x25d1a9b0[82,8,F]  next=0x25d1aa20 prev-continuation=0x25d1a940 next-continuation=0x25d1aa20 {60,4200,3744,840} [state=80604004] [content=0xa110030] [overflow=-1,0,3745,840] sc=0xdba2510 pst=:-moz-non
-element<
                            "measles-"
                          >
                        >
                        line 0x25d1aa68: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,5040,6084,840} <
                          Text(0)@0x25d1aa20[90,13,F]  next=0x25d1aa90 prev-continuation=0x25d1a9b0 next-continuation=0x25d1aa90 {60,5040,6084,840} [state=80604204] SELECTED [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<
                            "unvaccinated "
                          >
                        >
                        line 0x25d1aad8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,5880,7020,840} <
                          Text(0)@0x25d1aa90[103,15,F]  next=0x25d1ab00 prev-continuation=0x25d1aa20 next-continuation=0x25d1ab00 {60,5880,7020,840} [state=80604004] [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<
                            "children in an "
                          >
                        >
                        line 0x25d1ab48: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,6720,4680,840} <
                          Text(0)@0x25d1ab00[118,10,F]  next=0x25d1ab70 prev-continuation=0x25d1aa90 next-continuation=0x25d1ab70 {60,6720,4680,840} [state=80604004] [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<
                            "emergency "
                          >
                        >
                        line 0x25d1abb8: count=1 state=inline,clean,prevmarginclean,not impacted,wrapped,before:nobr,after:nobr[0x8120] {60,7560,6552,840} <
                          Text(0)@0x25d1ab70[128,14,F]  next=0x2189010 prev-continuation=0x25d1ab00 next-continuation=0x2189010 {60,7560,6552,840} [state=80604004] [content=0xa110030] sc=0xdba2510 pst=:-moz-non-element<
                            "situation: an "
                          >
                        >


I tried this with the old parser, but I also tried the HTML5 parser, and I got the exact same performance problem as with the old parser.
What about in the testcase in comment #0?
(Reporter)

Comment 8

9 years ago
(In reply to comment #7)
> What about in the testcase in comment #0?

That is the same file, I just saved it locally in case the original URL becomes unavailable for some reason.
Comment on attachment 416163 [details]
Test case

Ah, I see the problem ... I was loading it as text/plain...
Attachment #416163 - Attachment mime type: text/plain → text/html
(Reporter)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 416163 [details])
> Ah, I see the problem ... I was loading it as text/plain...

Oh sorry for the confusion.

I really don't get this bug.  Is memchr _this_ slow?
(Reporter)

Comment 11

9 years ago
Hmm, couldn't we cache the position of the next newline character somewhere and not look for it over and over again?
Maybe.

All the text is in one giant text node. It's pre-wrap, so during reflow it breaks into many text frames. Each time we reflow a text frame, we look for a \n in the text to limit our reflow to stop at the first \n (but there isn't one). So this leads to O(N^2) behaviour.
Created attachment 417051 [details] [diff] [review]
fix

This caches the offset of the first newline after a given offset. It's still not fast, but it's a lot better.
Assignee: nobody → roc
Attachment #417051 - Flags: review?(smontagu)
Comment on attachment 417051 [details] [diff] [review]
fix

Looks like this is the testcase, not the patch
Created attachment 417121 [details] [diff] [review]
fix

oops
Attachment #417051 - Attachment is obsolete: true
Attachment #417121 - Flags: review?(smontagu)
Attachment #417051 - Flags: review?(smontagu)
Created attachment 417125 [details] [diff] [review]
fix v2
Attachment #417121 - Attachment is obsolete: true
Attachment #417121 - Flags: review?(smontagu)
Comment on attachment 417125 [details] [diff] [review]
fix v2


>@@ -6456,16 +6484,38 @@ nsTextFrame::Reflow(nsPresContext*      

>+        if (NS_FAILED(mContent->SetProperty(nsGkAtoms::newline, cachedNewlineOffset,
>+                      NewlineProperty::Destroy))) {

nit: the indentation on the second line should be after the next "("
Attachment #417125 - Flags: review+
Whiteboard: [needs landing]
(Reporter)

Comment 19

9 years ago
Roc, did you mean to mark this as FIXED?  Also, I think we also want this on 1.9.2.

For the record, the landed patch fixes the performance problem for me.
Flags: wanted1.9.2?
I believe this problem has been around for a long time, therefore the patch doesn't NEED to be in 1.9.2, although it might be good to have it there.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Roc et al, did this get checked for Memcheck cleanness?  I'm getting
the complainage shown below when loading eg http://www.kde.org

It complains about an uninitialised value use at 
layout/generic/nsTextFrameThebes.cpp:6501, viz:

  } else if (cachedNewlineOffset) {          <----------- here
    mContent->DeleteProperty(nsGkAtoms::newline);
  }

and hg ann points at changeset 36903:6dc53634551f, which refers to this
bug.

w/ apologies if I'm on the wrong track here ..

==32280== Conditional jump or move depends on uninitialised value(s)
==32280==    at 0x5799C2F: nsTextFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsTextFrameThebes.cpp:6501)
==32280==    by 0x57774E4: nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, int&) (nsLineLayout.cpp:852)
==32280==    by 0x5740D06: nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) (nsBlockFrame.cpp:3715)
==32280==    by 0x5741847: nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, int*, LineReflowStatus*, int) (nsBlockFrame.cpp:3510)
==32280==    by 0x5741DDF: nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, int*) (nsBlockFrame.cpp:3364)
==32280==    by 0x5741FFD: nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, int*) (nsBlockFrame.cpp:2439)
==32280==    by 0x574265E: nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) (nsBlockFrame.cpp:1885)
==32280==    by 0x5743342: nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsBlockFrame.cpp:993)
==32280==    by 0x574EC3F: nsFrame::BoxReflow(nsBoxLayoutState&, nsPresContext*, nsHTMLReflowMetrics&, nsIRenderingContext*, int, int, int, int, int) (nsFrame.cpp:6499)
==32280==    by 0x575373F: nsFrame::RefreshSizeCache(nsBoxLayoutState&) (nsFrame.cpp:6085)
==32280==    by 0x5753AF0: nsFrame::GetPrefSize(nsBoxLayoutState&) (nsFrame.cpp:6169)
==32280==    by 0x584E116: nsSprocketLayout::GetPrefSize(nsIFrame*, nsBoxLayoutState&) (nsSprocketLayout.cpp:1366)
==32280==  Uninitialised value was created by a stack allocation
==32280==    at 0x57990B2: nsTextFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) (nsTextFrameThebes.cpp:6059)
Julian, see bug 539720 (marked as a regression from this bug, etc).
blocking2.0: --- → -
(Reporter)

Updated

7 years ago
Flags: wanted1.9.2?
You need to log in before you can comment on or make changes to this bug.