Closed Bug 597627 Opened 14 years ago Closed 13 years ago

Adding and removing lines in big textareas is really slow

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0b11
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: sicking, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug)

Details

(Keywords: perf, regression, Whiteboard: [hardblocker][fx4-fixed-bugday])

Attachments

(5 files, 6 obsolete files)

Don't invalidate textruns
1.42 KB, patch
Details | Diff | Splinter Review
Log showing this behavior
10.42 KB, text/plain
Details
Framedump (for the asserion in comment 99)
1.88 KB, text/html
Details
Framedump (for the assertion in comment 93)
34.47 KB, text/html
Details
Patch v5
4.87 KB, patch
roc
: review+
bzbarsky
: feedback+
Details | Diff | Splinter Review 
It seems like there is an n^2 algorithm somewhere causing adding and removing newlines to <textarea>s with lots of lines (or maybe just lots of text) to very slow.

Steps to reproduce:
1. Go to https://bugzilla.mozilla.org/attachment.cgi?id=476443&action=edit
2. Click "Edit Attachment As Comment"
3. Click in the textarea
4. Press enter

Step 4 is very noticeably slow.

Similarly

5. Select a few lines of text
6. Press delete

is similarly slow.

Normal editing is fast, however any time newlines are added or removed there is a significant pause.

This is a recent regression so I'm guessing it's a regression from bug 240933. I've only tested on mac so not sure if it happens on other platforms or not.
Profile (on Mac) says 95% of the time is spent in the reflow that we trigger from EndUpdateViewBatch.  Even more interestingly, 93% is under nsLineLayout::TrimTrailingWhiteSpace and almost all of this is under EnsureTextRun.  Sounds like we're recreating textruns for the whole thing, presumably because inserting some text marked them all dirty?

Oh, and we're spending most of our time here on line-breaking in the textrun code.

Are we trying to trim trailing whitespace all down the line, or are we just creating huge textrun?  I thought we limited textrun length or something...  and we shouldn't have to trim trailing whitespace all down the line (and in fact, should be able to stop the reflow at the next line that ends in a newline char, yes?)
blocking2.0: --- → ?
Actually, it appears adding enough text on one line that it wraps also slows things down. So it's not the newline character in itself that's the culprit.
Right; it's the fact that we end up having to reflow over to the next line at all.  We should still be terminating that reflow at the _next_ newline we hit.  So if you have:

  AAAAAAA
  CCCCCCC

on two adjacent lines and you type a bunch of Bs after the As until you line-wrap, we shouldn't reflow past the end of the Cs if we're doing it right.  Imo.
One problem is that we can't insert a frame into the middle of the continuation chain. So if you need to split one line into two, the continuation frame for each later line N+1 becomes the continuation for line N (which requires reflow of course).

Should only be O(N) per split though.
You seem to be right in that it's not n^2. If I duplicate the size of a 100KB text area the time it takes to add a newline early in the area goes from 3 seconds to 6 seconds on a reasonably new macbook pro.

Still very slow though. Slow enough that it's annoying to review big patches directly in bugzilla.
We can probably insert a text frame into the continuation chain, but it's a little tricky.
(In reply to comment #6)
> We can probably insert a text frame into the continuation chain, but it's a
> little tricky.

Hmm, aren't text frame continuations stored as a linked list?  Is there some other data structure which needs to get updated as well?  Would you elaborate please?
Actually I guess it might just work.
What's very weird to me is that when doing the steps in comment 0, step 2 is much faster than step 4. Doesn't both (re)create the full set of line frames?
I'd think so, yes....  That's odd, indeed.
Similarly, copying the full contents of the textarea, moving to the end, and pasting is also basically instant. Even though I would expect that to create/reflow just as many inline frames.

In fact, pasting a full copy the end is about 30 times faster than inserting a newline at the beginning.
Yeah, that's really weird.  roc, any idea what's going on there?
It seems to me that the majority of the problem here is textrun reconstruction for the entire textarea.

I saw this comment <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#3911>, and this code indeed is what invalidates all of the continuations' textruns when the field is edited.

I didn't understand the comment though, and I'm not sure how to do what it says, but I'm pretty sure that we'll get a huge perf benefit if we can avoid invalidating those textruns unnecessarily.
Moving to Layout: Text which is probably a better component for this bug.
Component: Editor → Layout: Text
QA Contact: editor → layout.fonts-and-text
That hypothesis doesn't really address the question from comment 9 or comment 11, as far as I can tell...

Also, that code runs when _any_ char is typed, no?  Why is typing non-newlines fast?  Just because we only reconstruct the textruns for the line that was edited?
(In reply to comment #17)
> That hypothesis doesn't really address the question from comment 9 or comment
> 11, as far as I can tell...

It doesn't.  I tried figuring it out today but couldn't.  Maybe tomorrow I'll notice something that I've missed today.

> Also, that code runs when _any_ char is typed, no?  Why is typing non-newlines
> fast?  Just because we only reconstruct the textruns for the line that was
> edited?

On second look, this code should have the same effect no matter what character is typed, which means that the textruns for the following lines are invalidated either way, which makes my theory in comment 14 invalid perhaps.
Well, textrun invalidation is (relatively) cheap.  If we then don't need to _reconstruct_ the textruns for the text outside the visible part of the textarea, things would be fast.

But give comment 9 and comment 11, it sounds to me like we reconstruct textruns multiple times when enter is pressed.  Or something.
So, here's what's happening.  We're actually reflowing three times:

1. Once from nsRefreshDriver::Notify which seems to be a usual refresh event pending in the event queue.
2. Once from nsTypedSelection::ScrollIntoViewEvent::Run which we post when the editing operation is done.
3. Once from nsViewManager::CallWillPaintOnObservers which happens when painting the invalidated area generated in steps 1 and 2.

Why we reflow three times really puzzles me...
Can you track what is requesting these reflows by seeing what is calling PresShell::FrameNeedsReflow?
Also, I tried taking out the loop in question in comment 14 out, and it indeed fixes the perf problem.
(In reply to comment #21)
> Can you track what is requesting these reflows by seeing what is calling
> PresShell::FrameNeedsReflow?

Probably...  But given comment 22, I don't think that's going to be too useful now...
So, this is what I could make out of the comment in the code, but the code is very broken, in that it seems to update the frame content offsets to invalidate values.  Does anybody have any idea what I've been missing here?
Again, typing _anything_ does the textrun invalidation.  Pasting the text in starts with invalid textruns.

Give me a bit to look into this.
So a bit of testing with a 4-line textarea....

I confirmed that typing non-newline characters reflows just the line they're typed on (as long as it doesn't wrap) while typing enter reflows all the lines.

I also confirmed that typing enter a single time clears textruns once via that loop and reflows each line exactly once.

I also confirmed that cutting and then pasting all the text in the textarea reflows each line exactly once.  Also that it's much much faster than hitting enter.

I added a printf in ClearAllTextRunReferences right before the SetTextRun call, like so:

    if (aFrame->GetContent()->GetParent() &&
        aFrame->GetContent()->GetParent()->GetParent() &&
        aFrame->GetContent()->GetParent()->GetParent()->Tag() ==
          nsGkAtoms::textarea &&
        aFrame->GetTextRun()) {
      printf("Clearing: %p\n", aFrame);
    }
and one at the beginning of nsTextFrame::ReflowText like so:

  if (mContent->GetParent() &&
      mContent->GetParent()->GetParent() &&
      mContent->GetParent()->GetParent()->Tag() == nsGkAtoms::textarea) {
    printf("Reflowing: %p\n", this);
  }

Here's what the output looks like if I paste 4 lines into a textarea (the lines contain one char each: "a", "b", "c", and "d"):

  Reflowing: 0x103bd8990
  Reflowing: 0x103bd93a8
  Reflowing: 0x103bd92f0
  Reflowing: 0x103bd9198

Here's what happens if I hit space on the first line before the "a":
  Clearing: 0x103bd8990
  Clearing: 0x103bd93a8
  Clearing: 0x103bd92f0
  Clearing: 0x103bd9198
  Reflowing: 0x103bd8990Here's what happens if I hit enter before the "d":

  Clearing: 0x103bd9198
  Reflowing: 0x103bd92f0
  Reflowing: 0x103bd9198
  Reflowing: 0x1041b06d8
Here's what happens if I hit enter before the "c":

  Clearing: 0x103bd92f0
  Clearing: 0x103bd9198
  Reflowing: 0x103bd93a8
  Reflowing: 0x103bd92f0
  Clearing: 0x103bd92f0
  Clearing: 0x103bd9198
  Reflowing: 0x103bd9198
  Reflowing: 0x1041b06d8Here's what I get if I hit enter before the "a":

  Clearing: 0x103bd8990
  Clearing: 0x103bd93a8
  Clearing: 0x103bd92f0
  Clearing: 0x103bd9198
  Reflowing: 0x103bd8990
  Clearing: 0x103bd8990
  Clearing: 0x103bd93a8
  Reflowing: 0x103bd93a8
  Clearing: 0x103bd93a8
  Clearing: 0x103bd92f0
  Reflowing: 0x103bd92f0
  Clearing: 0x103bd92f0
  Clearing: 0x103bd9198
  Reflowing: 0x103bd9198
  Reflowing: 0x1041b06d8
So it looks like we clear all the textruns for the textarea from the CharacterDataChanged notification, then reflow all the lines, and before reflowing each line we clear its textrun, which is non-null at that point.  So presumably we're constructing all those textruns twice...  That still doesn't quite explain the behavior I see, where the difference is a lot more than 2x between paste and hitting enter.
Er... some of the newlines in the previous comment disappeared  :(
nsTextFrame::SetLength called from nsTextFrame::ReflowText is what ends up doing some of that textrun clearing on next-in-flows and the like.
Wait.  When I paste that huge textarea, I see something like 3 textruns being constructed... (as measured by a breakpoint in gfxTextRun::gfxTextRun).

If I hit enter a few lines from the end I get two textruns constructed per line between me and the end.  roc, is that expected?
(In reply to comment #27)
> Er... some of the newlines in the previous comment disappeared  :(

Hmm, dbaron also told me yesterday that he's seen this as well.  Do you happen to have STRs?
(In reply to comment #26)
> So it looks like we clear all the textruns for the textarea from the
> CharacterDataChanged notification, then reflow all the lines, and before
> reflowing each line we clear its textrun, which is non-null at that point.  So
> presumably we're constructing all those textruns twice...  That still doesn't
> quite explain the behavior I see, where the difference is a lot more than 2x
> between paste and hitting enter.

Why is the textrun non-null when reflowing?  We've already cleared them all, right?

Also, from what I read in this comment, it seems to me that we don't need to clear the textruns at all in CharacterDataChanged.  But that breaks us horribly (having tried that).  Is there something missing in this picture?
(In reply to comment #28)
> nsTextFrame::SetLength called from nsTextFrame::ReflowText is what ends up
> doing some of that textrun clearing on next-in-flows and the like.

Yes, and I think we're actually relying on that: <http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#3881>
(In reply to comment #29)
> Wait.  When I paste that huge textarea, I see something like 3 textruns being
> constructed... (as measured by a breakpoint in gfxTextRun::gfxTextRun).

How many lines are you pasting?  (We should create at least one textrun per line, right?)

> If I hit enter a few lines from the end I get two textruns constructed per line
> between me and the end.  roc, is that expected?

Looks like we should be able to get away without constructing that many textruns on pressing Enter then, right?  Of course, I have no idea how... :(
> Do you happen to have STRs?

Not offhand.... there was cutting and pasting involved, though.  Past that... I'll see what I can dig up.

> Why is the textrun non-null when reflowing?  We've already cleared them all,
> right?

We cleared them.  Presumably something regenerated them!

> Yes, and I think we're actually relying on that:

Right.  The point is we end up clearing+recreating there at least twice.

And again, that doesn't account for all of the slowdown.

> How many lines are you pasting?

I'm using the testcase linked from comment 0.  So 4379 lines.

> (We should create at least one textrun per line, right?)

An excellent question!
And the real issue here is roc's on vacation.  He might know the answer to that question, and my question from comment 29.
(In reply to comment #34)
> > Do you happen to have STRs?
> 
> Not offhand.... there was cutting and pasting involved, though.  Past that...
> I'll see what I can dig up.

Great, thanks.  I did some rough testing with cutting and pasting and couldn't repro...

> > Why is the textrun non-null when reflowing?  We've already cleared them all,
> > right?
> 
> We cleared them.  Presumably something regenerated them!

That's... sad.

> > Yes, and I think we're actually relying on that:
> 
> Right.  The point is we end up clearing+recreating there at least twice.

Yep.

> And again, that doesn't account for all of the slowdown.

Hmm, I think it should.  See comment 22.

> > How many lines are you pasting?
> 
> I'm using the testcase linked from comment 0.  So 4379 lines.
> 
> > (We should create at least one textrun per line, right?)
> 
> An excellent question!

Well, that basically contradicts all that I know about textruns (which isn't that much, but still!)

(In reply to comment #35)
> And the real issue here is roc's on vacation.  He might know the answer to that
> question, and my question from comment 29.

Yeah.  Unless someone has a better idea, let's wait for roc to come back.
> Hmm, I think it should.  See comment 22.

Huh.  That's... odd.  Really odd, since we're clearing the textruns as we reflow too, right?

> Well, that basically contradicts all that I know about textruns

Note that it's _possible_ I was getting hits on the textrun cache.  Again, I was just looking at the constructor invocations.
(In reply to comment #37)
> > Hmm, I think it should.  See comment 22.
> 
> Huh.  That's... odd.  Really odd, since we're clearing the textruns as we
> reflow too, right?

Yes.  I observed things getting a lot faster when I took out that loop, but I can't really explain it.

> > Well, that basically contradicts all that I know about textruns
> 
> Note that it's _possible_ I was getting hits on the textrun cache.  Again, I
> was just looking at the constructor invocations.

Hmm, IINM we remove textruns from the cache when we're clearing them (why are we doing that?!).
Excellent question.  If you leave the clearing code but take out the cache-removal bit, are things fast?
Are you talking about ClearTextRun?

  if (!(textRun->GetFlags() & gfxTextRunWordCache::TEXT_IN_CACHE)) {
    // Remove it now because it's not doing anything useful
    gTextRuns->RemoveFromCache(textRun);
    delete textRun;

So we only remove it from gTextRuns (an nsExpirationTracker) if the textrun is NOT referenced from the gfxTextRunWordCache.

(In reply to comment #29)
> If I hit enter a few lines from the end I get two textruns constructed per line
> between me and the end.  roc, is that expected?

No, that sounds like a definite problem. What should happen is that in CharacterDataChanged we clear the textruns from the caret to the end, then when we reflow the line with the caret (or maybe the line before the caret), we reconstruct text runs for those lines, and then no more textrun construction happens.
Sorry to butt in.

I can see this bug in Windows 7 with Fx 3.6.10. Perhaps the Platform field for this bug should be changed to "x86 all" or "all all" to better reflect the breadth of affected systems.

Cheers.
Whatever you're seeing in 3.6.10 is unrelated to the issue this bug is about...
Well, using the STR in comment 0 I see exactly what is described there: utter slowness, big lag. If you say it's unrelated, so be it.
Well, see "This is a recent regression so I'm guessing it's a regression from bug 240933." in comment 0.
OK, I tried another experiment.  First, breakpoint in gfxTextRun::gfxTextRun and set it to run the commands "p aLength; cont" when hit.  Then create a textarea with rows="10" and paste this content into it:

1
2
3
4
5
6
7
8

That leads to two calls to the ctor, with lengths 16 and 15 respectively (both under ReflowText).

Then I focused the textarea, put the caret before the "1" and hit enter.  This led to 27 calls to the constructor, with the following lengths: 3, 2, 2, 2, 2, 2, 2, 2, 1, 4, 2, 4, 2, 2, 4, 2, 2, 4, 2, 2, 4, 2, 2, 4, 2, 2, 4.

While some of these (near the beginning) were from ReflowText, the stuff starting with "1" seems to be under nsTextFrame::TrimTrailingWhiteSpace.
That's a log of the paste and single enter press I described, generated by using these commands at the breakpoint:

  p aLength
  x/4c aText
  cont
Let me take this.
Assignee: ehsan → roc
OK, I still don't know why we create different textruns after pasting and pressing Enter, but what bz saw in comment 45 is not surprising at all.

ClearTextRun sets mTextRun on the frames to null.  And we invalidate all of the lines following the entered newline, which means that the textframes inside them are all reflown, at which time a new textrun is created because, well, mTextRun is null!
(In reply to comment #47)
> Let me take this.

OK, thanks!  I was kind of lost here.  :-)
> at which time a new textrun is created 

That would be fine... But we have 8 textframes, and create _27_ textruns.  Even ignoring the difference vs paste, that means we're creating and destroying as we go; that's the problem.
Ah, there's something going on here which is sort of my fault!

See this code: <http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxTextRunWordCache.cpp#747>

IINM, if there is a single numeric character in the textarea, it causes us to not cache the textrun when first the text, for example...  My tests show that adding a digit to a textarea makes things noticeably slower in it :(
That's only if we're transforming digits, no?  I dunno whether we are in your case, but we aren't in mine...
(In reply to comment #52)
> That's only if we're transforming digits, no?  I dunno whether we are in your
> case, but we aren't in mine...

Right...  It's possible that the transformation was actually happening in my case, since I was playing with the bidi.numeral pref value...
Here's what I've found.  In the insertion case we adjust the content offset
so that the remainder of the text run becomes part of the next one...
then in Reflow we have "1\n2\n" and we do a SetLength after the first \n --
now SetLength adjusts the content offset again so that the next one becomes
"2\n3\n" and so forth. This ripples through all the lines and when SetLength
does this it calls ClearTextRun() on both the current frame and the
next-in-flow...

Deleting a newline looks like a different problem.
Boris, did you investigate this case?
Attached patch WIP (obsolete) — — Splinter Review 
This fixes the insertion case, but not the delete case.
> Boris, did you investigate this case?

No, I had just looked at insertions....
Mats, are you still working on this?  This was marked as a softblocker, but I think shipping without this fixed is really bad.  In a sense, it's a regression compared to 3.6: textarea editing is now much slower on medium sized textareas...
Why is this a softblocker o_O
FWIW it's very easy to encounter this bug when replying to messages in Gmail, since it auto-quotes the whole conversation.
yeah, we may want to take another look at this one as a hardblocker. dbaron/roc, what was your rationale?
blocking2.0: final+ → ---
OS: Mac OS X → Windows 7
blocking2.0: --- → final+
OS: Windows 7 → All
How big is "big" and how slow is "slow"?

I'd note there are no duplicates or comments indicating anybody other than platform developers are seeing the bug; that made it seem like an edge case.
(In reply to comment #61)
> How big is "big" and how slow is "slow"?

Try Jonas's testcase in comment 0.  It's just 4000 lines -- not impossibly large for an e-mail thread -- and inserting a newline takes about 5s on my machine.
http://input.mozilla.com/en-US/search/?product=firefox&q=typing+slow

there's a thread in d-a-f and dev-planning where a user reported both slow URL-bar typing and slow text-entry typing (likely unrelated to each other)

http://twitter.com/bartgrefte/statuses/23461254035152896 is one I sampled randomly from twitter, not sure what version; could be the twitter-reflow bug too.
For a 140kb textarea (the testcase in comment 0), inserting or deleting a line takes about a second or two for me, on a laptop that was brand-new-and-fast in July.

The behavior is quadratic, so doing it on something like http://en.wikipedia.org/w/index.php?title=Economy_of_Poland&action=edit (22KB of text) is a bit faster but still noticeably laggy.  Doint it on something like http://en.wikipedia.org/w/index.php?title=Seven_Years%27_War&action=edit (55KB) is definitely laggy; 500ms at least.

The really long wikipedia articles seem to be locked down so I can't edit them.  ;)

As for why no one has reported this... my best guess is that people generally assume that our editing performance is sucky and so think this is normal.  :(
Mike, if the text is twitter-sized this bug is really not an issue.  It starts to be a problem once you have tens of kilobytes or more of text.
He's probably referring to bug 579488, typing in the tweet box being slow. It most definitely is slow, but for a completely different reason than this bug.
People are reporting it also for facebook comment entry, and entering a comment in for input.mozilla.org, but if there's another bug for "slow on small text fields too" then I'm happy to take it there.

(I do notice that input.m.o also has a character-counter, hmm.)
Slowness of typing into the location bar isn't this bug (it could be places).

Also note that in order to hit this bug, you have to have many lines in the textarea, and also edit something near the beginning which causes new lines to be added.  In such a case, trunk performs noticeably worse than 3.6.  bz's suspicion in comment 64 is also correct, IMO (the general suckiness of our editing performance), but this could cause Firefox 4 for many wikipedia editors.  One other set of users which may be affected by this are those running intranet web based apps.  Many of the old textarea editing perf bugs we have is coming from those people, and I suspect that those people might not be running our beta's for other compatibility concerns.
(In reply to comment #67)
> People are reporting it also for facebook comment entry, and entering a comment
> in for input.mozilla.org, but if there's another bug for "slow on small text
> fields too" then I'm happy to take it there.
> 
> (I do notice that input.m.o also has a character-counter, hmm.)

The twitter issue is somewhat specific, so if there are issues with entering text in other small text boxes then I would like to investigate each of them.

I tried a random facebook comment field, seemed fine to me.

I can't find an input box on input.m.o that has a character-counter, where do I look?
(In reply to comment #69)
> (In reply to comment #67)
> > People are reporting it also for facebook comment entry, and entering a comment
> > in for input.mozilla.org, but if there's another bug for "slow on small text
> > fields too" then I'm happy to take it there.
> > 
> > (I do notice that input.m.o also has a character-counter, hmm.)
> 
> The twitter issue is somewhat specific, so if there are issues with entering
> text in other small text boxes then I would like to investigate each of them.
> 
> I tried a random facebook comment field, seemed fine to me.

Those are textareas, so they're affected by this bug.

> I can't find an input box on input.m.o that has a character-counter, where do I
> look?

Why is a character counter important?
Because the twitter issue is caused by reflow caused by the change in width of the character counter as people type in it, IIRC.
(In reply to comment #70)
> (In reply to comment #69)
> > (In reply to comment #67)
> > > People are reporting it also for facebook comment entry, and entering a comment
> > > in for input.mozilla.org, but if there's another bug for "slow on small text
> > > fields too" then I'm happy to take it there.
> > > 
> > > (I do notice that input.m.o also has a character-counter, hmm.)
> > 
> > The twitter issue is somewhat specific, so if there are issues with entering
> > text in other small text boxes then I would like to investigate each of them.
> > 
> > I tried a random facebook comment field, seemed fine to me.
> 
> Those are textareas, so they're affected by this bug.

But people don't typically enter comments long enough in them to see the effect of this bug, do they?
(In reply to comment #60)
> yeah, we may want to take another look at this one as a hardblocker.
> dbaron/roc, what was your rationale?

I still don't see a rationale.

(In reply to comment #64)
> 
> The behavior is quadratic...

I think users would find that order of growth to be a reasonable proxy for "Firefox sucks". Anyone disagree?
The root problem with http://input.mozilla.com/en-US/sad is the same as twitter (repainting too much for inline lines), but it doesn't use as much box-shadow, border-radius, and overflow: hidden so the repaints aren't as slow.
(In reply to comment #72)
> (In reply to comment #70)
> > (In reply to comment #69)
> > > (In reply to comment #67)
> > > > People are reporting it also for facebook comment entry, and entering a comment
> > > > in for input.mozilla.org, but if there's another bug for "slow on small text
> > > > fields too" then I'm happy to take it there.
> > > > 
> > > > (I do notice that input.m.o also has a character-counter, hmm.)
> > > 
> > > The twitter issue is somewhat specific, so if there are issues with entering
> > > text in other small text boxes then I would like to investigate each of them.
> > > 
> > > I tried a random facebook comment field, seemed fine to me.
> > 
> > Those are textareas, so they're affected by this bug.
> 
> But people don't typically enter comments long enough in them to see the effect
> of this bug, do they?

Maybe...  Anyway we have legit use cases for textareas which lets them grow enough for this bug to show (web mail apps, for example).

(In reply to comment #73)
> I think users would find that order of growth to be a reasonable proxy for
> "Firefox sucks". Anyone disagree?

I second that.
Reasking, Why is this a softblocker?
Not sure if this is related but the textarea on the web page that is displayed when clicking the Minefield "Feedback" button (Firefox Input?) is also very slow to type characters from the first to last characters regardless of line feeds.

I guess it is possible that the 'characters remaining' code is slowing this down however I've seen similar code run substantially faster (though never at the same speed as normal typing, *sigh*).

Hope this feedback is not just noise.
pd, that sounds like a separate issue.  The "characters remaining" thing is very likely part of the problem in your case, but the details could depend on what styles are used, etc.  Can you please file a bug on your problem with detailed steps to reproduce, as well as details about your OS and the graphics information from about:support, and cc me on that bug?
A user was complaining that he/she had to wait 10 minutes when entering a new line in a textarea sometimes. How is that a softblocker? I mean, according to Johnath blog post [1] a softblocker is a "visual polish, strange edge cases, optional aspects of new specs, or opportunistic performance wins". This bug is none of them. However, it matches the description of a hardblocker which can be a "performance hit".

[1] http://blog.johnath.com/2011/01/13/its-almost-ready/
Yeah, I think we need to retriage this.

Mats, did you ever get anywhere on the deletion case?
Whiteboard: [softblocker]
Not really, I'm still at the debugging stage with the deletion case.
Any help with that would be appreciated.  I have a couple of hardblockers
left but they should be done in a day or two I hope...
Attached patch Possible patch (obsolete) — — Splinter Review 
I don't purport to fully understand what this code actually does, but following the same idea for the growing case as the shrinking case makes deletion fast for me.
Agreed, this sounds more like a hardblocker to me as well, given the recent comments. Roc, if you or someone else disagrees, feel free to mark otherwise.
Whiteboard: [hardblocker]
No, that patch is definitely wrong, since it'll mess with frames whose content offset is too big (and incidentally doesn't make deletion fast for me).
Comment on attachment 507129 [details] [diff] [review]
Possible patch

Yeah, it blew up in crashtests on try too.
Attachment #507129 - Attachment is obsolete: true
So what I think happens in the deletion case I'm testing is that we delete the newline char, end up with an empty text frame, reflow, and effectively move each bit of text to the previous textframe...  which means rebuilding text runs for all of them.
This seems to fix the deleting case for me too.  Mats, what do you think?
Comment on attachment 507163 [details] [diff] [review]
This might work, on the other hand

This works nicely for me.  Is there anything else remaining to be done here?  Or could we enter the review stage?

(Note: s/Whateve/Whatever/)
Comment on attachment 507163 [details] [diff] [review]
This might work, on the other hand

This is scary but it *should* work.
Attachment #507163 - Flags: review+
Mats, you want me to land this?  Or do you want to?
There was an assertion on TryServer in one of the reftests when I tested my
patch, in nsFirstLetterFrame IIRC.  Feel free to land it if that doesn't occur
anymore.  (sorry, I should have noted that on the bug at the time)
Ah.... I have no idea.  Pushing this to try right now, to see.
When I run reftests locally on Linux64 I get:
###!!! ASSERTION: null frame is not allowed: 'aFrame',
file layout/generic/nsLineLayout.cpp, line 656
followed by a crash.  The call comes from nsFirstLetterFrame::Reflow
Removing the last hunk fixes that.  I'm re-running the tests again to see
if I can catch the assertion I saw with my part...
Yeah, from the first part we get:
###!!! ASSERTION: bad overflow list: 'mFrames.IsEmpty()',
file layout/generic/nsFirstLetterFrame.cpp, line 364
when loading layout/reftests/bugs/408493-2.html
OK.  For the second hunk, we should probably only do the removal if we're not changing parent in the process.  That should fix the comment 93 assert+crash.

We can probably fix the comment 95 assert by checking whether the parent is a letter frame... but could other inline frames have similar issues, or just the weirdness that is letterframe?
Do you know what the actual issue is with the letterframe?
Not yet.
Attached patch Patch v2 (obsolete) — — Splinter Review 
This is with Boris' suggested fixes.  It still asserts though:
###!!! ASSERTION: overflow list w/o frames: 'mFrames.NotEmpty()',
file layout/generic/nsFirstLetterFrame.cpp, line 377
when loading layout/generic/crashtests/533379-1.html

I'm debugging these assertions to see what's going on...
Attachment #497875 - Attachment is obsolete: true
Attachment #507163 - Attachment is obsolete: true
The last assertion comes from removing the child of a first-letter frame
(0x7fffde659138 blue), on the next reflow we call DrainOverflowFrames
which asserts that the principal list is non-empty when there is an
non-empty overflow list.

I think this is just my fault of not implementing Boris' suggested fix
quite right...
 +        next->GetParent() == f->GetParent()) {
should have been
 +        GetParent() == f->GetParent()) {
which would avoid doing the RemoveFrame optimization in this case.
No, I'd in fact been thinking of comparing the parents of next and f.

If we remove |f|, which is the child of a first-letter frame, but next->GetParent() == f->GetParent(), then |next| is a child of that first-letter too.  So how does the principal child list get empty?  Do we end up pushing |next| for some reason?
Attached patch Patch v3 (obsolete) — — Splinter Review 
This is the fix Boris' suggested I think.
I'll submit it to the TryServer and ask for review if it pass unit tests...
Meanwhile, I'll debug the assertions in comment 93/95.
Attachment #507510 - Attachment is obsolete: true
Attachment #507585 - Flags: feedback?(bzbarsky)
You're fast ;-)
The principal list becomes empty because 'next' is on the overflow list.
I guess we could just remove that assertion; I mean the important
thing is that the principal list is non-empty after draining overflow.

Or we could just avoid doing the second optimization at all when a
first-letter frame is onvolved, just to be on the safe side...
Oh, |next| is already on the overflow list but |f| is not while we're reflowing |f|?

I think a simple solution would be to only do this optimization when f->GetNextSibling() == next.  That ensures that they're in the same child list, etc.
Yes.  Ok, that sounds good.
(Well, to be precise, "while we're reflowing |f|" isn't correct - we're
 reflowing its prev-in-flow (0x7fffde6532e0) which has a different
 first-letter parent.)
Attached patch Patch v4 (obsolete) — — Splinter Review 
This is with the "f->GetNextSibling() == next" check.
Attachment #507585 - Attachment is obsolete: true
Attachment #507599 - Flags: feedback?(bzbarsky)
Attachment #507585 - Flags: feedback?(bzbarsky)
> we're reflowing its prev-in-flow 

Aha, I see.  OK, that makes perfect sense!

We should document why the GetNextSibling() check is there.

For the other, do we know why there's a problem with first-letter?  Is it really first-letter specific?
The cause of the assertion/crash in comment 93 is the RemoveFrame we added
causes a lot of first-letter frames to become empty.  Then we reflow one
of those and it can't handle that.  (See bug 578977.)
The GetNextSibling() check we added should prevent this.

The assertion in comment 95 looks like a bogus assert.  I think it's a typo
that was really meant to be "overflowFrames->IsEmpty()":
http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/layout/generic/nsFirstLetterFrame.cpp#l364
I propose we simply remove it since StealOverflowFrames() already assert this:
http://hg.mozilla.org/mozilla-central/annotate/e98b94aa64fa/layout/generic/nsContainerFrame.h#l627
Attached patch Patch v5 — — Splinter Review 
Add comment about GetNextSibling() check.  Remove bogus assert.
(v4 looks good on Try so far...)
Attachment #507599 - Attachment is obsolete: true
Attachment #507698 - Flags: feedback?(bzbarsky)
Attachment #507599 - Flags: feedback?(bzbarsky)
I meant "!overflowFrames->IsEmpty()" of course :-)
After thinking some more about that assertion; it's probably not a typo
but intended to catch an empty first-letter frame that should have been
removed (as in bug 578977).  So let's keep it.
Comment on attachment 507698 [details] [diff] [review]
Patch v5

This looks fine if we document that parent frame type check.  Have roc look at this too?
Attachment #507698 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 507698 [details] [diff] [review]
Patch v5

Assuming you leave in the assert in nsFirstLetterFrame.
Attachment #507698 - Flags: review+
With additional comment and assert in nsFirstLetterFrame intact.
http://hg.mozilla.org/mozilla-central/rev/f704da8cce12
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
Status: RESOLVED → VERIFIED
Whiteboard: [hardblocker] → [hardblocker][fx4-fixed-bugday]
Verified with Mozilla/5.0 (X11; Linux i686; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre
Blocks: 635329
No longer blocks: 635329
Depends on: 635329
Depends on: 643918
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: