Bug 98711 – DOM/textarea-edit.html spends 35% of time in numGraphemeClusters

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Ryosuke Niwa]

It appears that the bottleneck is numGraphemeClusters. It's very slow :(

Running Time	Self		Symbol Name
17515.0ms   36.0%	17515.0	 	icu::RuleBasedBreakIterator::handleNext(icu::RBBIStateTable const*)
17460.0ms   35.9%	0.0	 	 icu::RuleBasedBreakIterator::next()
17031.0ms   35.0%	0.0	 	  WebCore::numGraphemeClusters(WTF::String const&)
8731.0ms   17.9%	0.0	 	   WebCore::HTMLTextAreaElement::tooLong() const
8731.0ms   17.9%	0.0	 	    non-virtual thunk to WebCore::HTMLTextAreaElement::tooLong() const
8300.0ms   17.0%	0.0	 	   WebCore::HTMLTextAreaElement::handleBeforeTextInsertedEvent(WebCore::BeforeTextInsertedEvent*) const
8300.0ms   17.0%	0.0	 	    WebCore::HTMLTextAreaElement::defaultEventHandler(WebCore::Event*)

[Ryosuke Niwa]

Created attachment 167690 [details]
Mitigates the bug

[Ryosuke Niwa]

Comment on attachment 167690 [details]
Mitigates the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=167690&action=review

> Source/WebCore/ChangeLog:8
> +        Since numGraphemeClusters(text) is always smaller than text.length(),

Ugh... this isn't true because of LF -> CRLF conversion :(

[Ryosuke Niwa]

Comment on attachment 167690 [details]
Mitigates the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=167690&action=review

> Source/WebCore/html/HTMLTextAreaElement.cpp:284
> +    if (currentValue.length() + event->text().length() < unsignedMaxLength)

I guess I can multiply this number by 2... so hacky :(

[Kent Tamura]

Comment on attachment 167690 [details]
Mitigates the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=167690&action=review

>> Source/WebCore/html/HTMLTextAreaElement.cpp:284
>> +    if (currentValue.length() + event->text().length() < unsignedMaxLength)
> 
> I guess I can multiply this number by 2... so hacky :(

Why don't you introduce countNumberOfLF() or something?

  if (currentValue.length() + countNumberOfLF(currentValue) + event->text().length() + countNumberOfLF(event->text()) < ...

[Darin Adler]

Comment on attachment 167690 [details]
Mitigates the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=167690&action=review

>>> Source/WebCore/html/HTMLTextAreaElement.cpp:284
>>> +    if (currentValue.length() + event->text().length() < unsignedMaxLength)
>> 
>> I guess I can multiply this number by 2... so hacky :(
> 
> Why don't you introduce countNumberOfLF() or something?
> 
>   if (currentValue.length() + countNumberOfLF(currentValue) + event->text().length() + countNumberOfLF(event->text()) < ...

We can have two early exits. One where you can tell it fits without even counting LFs. Another where you count LFs, and then the grapheme cluster counting full version.

[Darin Adler]

It also seems really unfortunate that the numGraphemeClusters function is so slow. I wonder if we could get ICU to speed it up, or do something in our ICU interface to speed up the many common cases. For example, grapheme clusters are == characters for all ASCII characters.

[Ryosuke Niwa]

(In reply to comment #6)
> It also seems really unfortunate that the numGraphemeClusters function is so slow. I wonder if we could get ICU to speed it up, or do something in our ICU interface to speed up the many common cases. For example, grapheme clusters are == characters for all ASCII characters.

Not sure special-casing ASCII characters is such a good idea. It'll certainly make it faster for English but it won't improve other languages.

[Ryosuke Niwa]

My original approach improved the score to ~0.5, and new approach (adding the number of LFs) improves the score to ~.43 whereas the original score is ~0.3. At least this approach removes either caller from the profiler so let's make this change for now.

[Ryosuke Niwa]

Created attachment 167801 [details]
Fixes the bug

[Kent Tamura]

Comment on attachment 167801 [details]
Fixes the bug

ok

[WebKit Review Bot]

Comment on attachment 167801 [details]
Fixes the bug

Clearing flags on attachment: 167801

Committed r130818: <http://trac.webkit.org/changeset/130818>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This improved the score by 54.4% on Mac Lion and 229.9% on Chromium Mac:

http://webkit-perf.appspot.com/graph.html#tests=[[2965621,2001,32196],[2965621,2001,3001]]&sel=1349804856909.961,1349864710655.502&displayrange=30&datatype=running