770564 - The more text in a textarea, the more time it takes to update the content

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

In Windows, in WordPress, for example, the larger the blog entry gets, the more time it takes to perform edits. Should also be reproducible on Blogger or any other form that has a textarea.

STR:
1. With Firefox Nightly, and NVDA running, insert at least 5 KB of text into the textarea.
2. Select one or two words, and delete them.

Result: It'll take several seconds for Firefox to become responsive again.

3. Insert the same amount again, so you now have roughly 10 K of text.
4. Again, perform an edit, like deleting just one character or word.

Result: The time it takes for Firefox to become responsive again increases.

5. Perform the steps once or twice more to see the effect grow worse and worse the more text is in the textarea.

Note that this is a simple textarea, not a rich-text one like CKEdit, as mentioned in bug 573719. So the caret always stays in the same accessible. I believe that it's text mutation events that are killing us here.

By comparison, I tried this on a nightly build on OS X, too, and there, inserting a 40 K text into the WordPress blog post composition textarea didn't produce any lag. Editing it was instantly snappy.

So it must have something to do with the events we fire on Windows (and possibly Linux) and/or NVDA's consumption of them that makes things so slow.

I went back to as far as Firefox 10 to find the same behavior. I suspect it's been there for a lot longer, so profiling is probably easier than trying to trace the history, considering all the code changes we've had.

Comment 1

[Marco Zehe (:MarcoZ)]

Testing with JAWS shows that the same perf problems exist, and even worse: Even in an unmodified textarea, navigation is getting considerably slower the more text is in there. With NVDA, pure navigation is still OK even if the textarea contains 40 KB of data.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Profiled it, here is where we block i/o:
http://is.gd/ch9udq

Click invert callstack to see the main offender: nsTextFrame::GetRenderedText

Comment 3

[James Teh [:Jamie]]

I've definitely noticed this, but never pin-pointed it or gotten around to investigating it further. One pointer, though: it might be worth trying this in a document with role="application". While it's still an issue in Gecko, NVDA's vbuf code may be what triggers this or exacerbates it.

Comment 4

[Marco Zehe (:MarcoZ)]

When I looked at the inverted stack, one thing I noticed that took considerable time was applying the correct spell checking attributes to the text.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to David Bolter [:davidb] from comment #2)
> Profiled it, here is where we block i/o:
> http://is.gd/ch9udq
> 
> Click invert callstack to see the main offender: nsTextFrame::GetRenderedText

the interesting thing we spend a time for spellcheck calculations. Did you have any misspelled words when you profiled?

Comment 6

[Marco Zehe (:MarcoZ)]

I definitely had misspellings in my text area, when I encountered this bug. I usually use the HTML multi-line text area in WordPress, and each tag usually gets flagged as a misspelling. Also, I had program code in there, so that also occasionally gets flagged as misspelled.

Comment 7

[alexander :surkov (:asurkov)]

We can do two things
1) Cache misspelled text offsets when requested, drop the cache when spelling selection is changed.
2) If rendered text can't be different from content text in textareas and inputs then we can specialcase these elements. We need to check this. Boris, perhaps you know the answer.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm not sure what you mean by "content text" in this case.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Boris Zbarsky (:bz) from comment #8)
> I'm not sure what you mean by "content text" in this case.

I guess the data stored in text nodes of native anonymous content of textareas and inputs.

Comment 10

[Boris Zbarsky [:bzbarsky]]

That should be what's rendered, yes.  At this point there should be no magic there.

Comment 11

[alexander :surkov (:asurkov)]

Andrew, would you like to take this bug? You simply can override ContentToRenderedOffset and RenderedToContentOffset at HTMLTextFieldAccessible (making them member function of course).

Comment 12

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #7)
> We can do two things
> 1) Cache misspelled text offsets when requested, drop the cache when
> spelling selection is changed.

I filed bug 786901 for that

Comment 13

[Trevor Saunders (:tbsaunde)]

I think we should be able to make GetSpellTextAttribute() algorithmically better too.

Since we have mozilla::Selection giving us inline access to the ranges of missspelled text, and it promises that the ranges are in sorted order I think we can binary search the array for the range containing our point, or the range before / after it.  Once we know the index of the range before the point or including the point its pretty easy to get if the point is missspelled and what the range is.

That should be faster since 1 it only looks at log(n) of the ranges and 2 it only needs to call ContentToRenderedOffset() twice.

This does however have a number of funny edge cases, but is a fairly interesting peace of code to write, but I'm not sure I have the time to write it myself right now.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> I think we should be able to make GetSpellTextAttribute() algorithmically
> better too.

> That should be faster since 1 it only looks at log(n) of the ranges and 2 it
> only needs to call ContentToRenderedOffset() twice.

please file a bug

> This does however have a number of funny edge cases, but is a fairly
> interesting peace of code to write, but I'm not sure I have the time to
> write it myself right now.

add yourself as a mentor, might be a good student project

Comment 15

[russellspivey]

I just tested in Linux, could not reproduce. I could reproduce it in the nightly within Windows 7 x64, simply by putting a ton of text within textarea tags and loading the page.

Comment 16

[Trevor Saunders (:tbsaunde)]

Attachment: patch

I didn't do the binary search thing because it just seems really edge casey and anoying, but I don't think its too hard to see how it would work conceptually on top of this patch.

However I expect this is a bit faster since there's a good bit less xpcom goop and we don't copy an array of refptrs to ranges around.

Comment 17

[alexander :surkov (:asurkov)]

Comment on attachment 673494 [details] [diff] [review]
patch

Review of attachment 673494 [details] [diff] [review]:
-----------------------------------------------------------------

nice, it seems events/test_textattrchange.html doesn't cover last case

::: accessible/src/generic/HyperTextAccessible.cpp
@@ -2224,5 @@
>                                             int32_t* aHTEndOffset,
>                                             nsIPersistentProperties* aAttributes)
>  {
> -  nsTArray<nsRange*> ranges;
> -  GetSelectionDOMRanges(nsISelectionController::SELECTION_SPELLCHECK, &ranges);

good you get rid it but you need to change GetSelectionDOMRanges to not take selection type to make sure it's used only for methods counting selection ranges within specific hypertext accessible.

@@ +2243,5 @@
> +    nsINode* endNode = range->GetEndParent();
> +    int32_t endOffset = range->EndOffset();
> +    if (nsContentUtils::ComparePoints(aNode, aNodeOffset, endNode, endOffset)
> +        >= 0)
> +      continue;

nit: add a comment that the point is *after* the range and we need to continue to find closest range *before* the point or the range containing the point
nit: don't keep >= on new line

@@ +2252,5 @@
> +    int32_t startOffset = range->StartOffset();
> +    if (nsContentUtils::ComparePoints(startNode, startOffset, aNode,
> +                                      aNodeOffset) <= 0) {
> +
> +      // This is the case the point is with in this range.

nit: makes sense to move this comment before 'if' statement

@@ +2278,5 @@
> +    rv = RangeBoundToHypertextOffset(range, true, false, &endHTOffset);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    if (idx > 0) {
> +      rv = RangeBoundToHypertextOffset(domSel->GetRangeAt(idx - 1), false, true, &startHTOffset);

nit: exceeds 80 chars restriction

@@ +2287,5 @@
> +      *aHTStartOffset = startHTOffset;
> +
> +    if (endHTOffset < *aHTEndOffset)
> +      *aHTEndOffset = endHTOffset;
> +    

nit: whitespace on empty line

@@ +2293,3 @@
>    }
>  
> +  // we never found a range that ended after the point, therefore we know that

nit: uppsercase 'w' in we

@@ +2296,5 @@
> +  // the point is not in a range, that we do not need to compute an end offset,
> +  // and that we should use the end offset of the last range to compute the
> +  // start offset of the text attribute range.
> +  if (rangeCount <= 0)
> +    return NS_OK;

wouldn't it be reasonable to do this early before cycle?

@@ +2298,5 @@
> +  // start offset of the text attribute range.
> +  if (rangeCount <= 0)
> +    return NS_OK;
> +
> +  rv = RangeBoundToHypertextOffset(domSel->GetRangeAt(rangeCount - 1), false, true, &startHTOffset);

nit: exceeds 80 chars restriction

Comment 18

[alexander :surkov (:asurkov)]

Trev, what about approach from comment #11? File a new bug for it?

Comment 19

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #18)
> Trev, what about approach from comment #11? File a new bug for it?

yeah, we can look at that as well as the binary search stuff in another bug, lets see what Marco thinks of just this first?

(In reply to alexander :surkov from comment #17)
> Comment on attachment 673494 [details] [diff] [review]
> patch
> 
> Review of attachment 673494 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> nice, it seems events/test_textattrchange.html doesn't cover last case

yeah, it seems there are some gaps in that, why don't we file gfb to add more coverage?  Or does it seem likely enough to find brokeness we want to do it ourselves?

> ::: accessible/src/generic/HyperTextAccessible.cpp
> @@ -2224,5 @@
> >                                             int32_t* aHTEndOffset,
> >                                             nsIPersistentProperties* aAttributes)
> >  {
> > -  nsTArray<nsRange*> ranges;
> > -  GetSelectionDOMRanges(nsISelectionController::SELECTION_SPELLCHECK, &ranges);
> 
> good you get rid it but you need to change GetSelectionDOMRanges to not take
> selection type to make sure it's used only for methods counting selection
> ranges within specific hypertext accessible.

I'm not sure why we need to make sure of that here, I didn't use it here because its faster not to since you don't copy the array, but if someone wants to use that method with other selection type that's fine with me other than that method is slow and not really useful.

> @@ +2243,5 @@
> > +    nsINode* endNode = range->GetEndParent();
> > +    int32_t endOffset = range->EndOffset();
> > +    if (nsContentUtils::ComparePoints(aNode, aNodeOffset, endNode, endOffset)
> > +        >= 0)
> > +      continue;
> 
> nit: add a comment that the point is *after* the range and we need to
> continue to find closest range *before* the point or the range containing
> the point

ok

> nit: don't keep >= on new line

iirc I did that to avoid being yeld at for going over 80 chars, but I'll happily go over 80 so sure

> @@ +2296,5 @@
> > +  // the point is not in a range, that we do not need to compute an end offset,
> > +  // and that we should use the end offset of the last range to compute the
> > +  // start offset of the text attribute range.
> > +  if (rangeCount <= 0)
> > +    return NS_OK;
> 
> wouldn't it be reasonable to do this early before cycle?

I guess though I doubt it really effects much, if there's 0 ranges then you basically skip cycle anyways.

Comment 20

[Trevor Saunders (:tbsaunde)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35311e11dafb

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35311e11dafb

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #19)
> (In reply to alexander :surkov from comment #18)
> > Trev, what about approach from comment #11? File a new bug for it?
> 
> yeah, we can look at that as well as the binary search stuff in another bug,
> lets see what Marco thinks of just this first?

I'd fix all known issues since they don't seem complicated and should give a certain win. Some people can't reproduce the bug at all (see comment #15).

> > nice, it seems events/test_textattrchange.html doesn't cover last case
> 
> yeah, it seems there are some gaps in that, why don't we file gfb to add
> more coverage?  Or does it seem likely enough to find brokeness we want to
> do it ourselves?

test coverage shouldn't be optional, especially when it's nearly trivial, at least it allows us to be sure we double checked ourselves (we have a number of cases when assignee and reviewer missed things and that led to on and on regressions). But since the patch is landed anyway then choose what you like more.

> I'm not sure why we need to make sure of that here, I didn't use it here
> because its faster not to since you don't copy the array, but if someone
> wants to use that method with other selection type that's fine with me other
> than that method is slow and not really useful.

All I say we can turn GetSelectionDOMRanges to work with selection inside the hypertext, in other words, if you want to get a selection at index 1 then you use this method. But if you want to work with selection at all (not taking into account selection indexes within hypertext) then you shouldn't use this method since it's perf hint. Selection at indexes work for normal selection type only, thus it doesn't make sense to take selection type as argument. File a gfb?

> > nit: don't keep >= on new line
> 
> iirc I did that to avoid being yeld at for going over 80 chars, but I'll
> happily go over 80 so sure

you can do:
if (nsContentUtils::ComparePoints(aNode, aNodeOffset,
                                  endNode, endOffset) >= 0)

to meet both requirements (btw, you did similar below)

> > wouldn't it be reasonable to do this early before cycle?
> 
> I guess though I doubt it really effects much, if there's 0 ranges then you
> basically skip cycle anyways.

yes, just checking length after cycle seems a little bit silly? since you can return early and do not loop into empty cycle