Bug 39958 – [Qt] TextBreakIteratorQt performance

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[David Leong]

Clicking on a large section of text (20,000) characters stored in a div takes several minutes to process a mouse down event on a S60 phone. The app is totally non-responsive and crashes most of the time when this happens.

After some investigations, we found the problem to be located in WebCore' htmlediting.cpp. In the function Position previousCandidate(const Position& position ) the while loop keeps looking for a text position and continually re-initializes QTextBoundaryFinder further down the stack. QTextBoundaryFinder initializes the text buffer to harfbuzz to calculate some line/word break data.

Initializing Harfbuzz is extremely expensive it seems. While looking for the text position in this use case it is re-initializing QTextBoundaryFinder with the same text data in the render text node. 

An optimization will be to cache the text ptr and length to not recreate QTextBoundaryFinder when the same data is presented to harfbuzz.

[Kenneth Rohde Christiansen]

cc'ing Simon, as he is the Harfbuzz expert ;-)

[David Leong]

Created attachment 57499 [details]
Proposed fixes to TextBreakIteratorQt

Proposed fixes, speeds up mouse down event handling for 20k character text from >1min to 2s on S60 device.

[WebKit Review Bot]

Attachment 57499 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
espace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:52:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:54:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:58:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:60:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:61:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:62:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:73:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:75:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:77:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:81:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:83:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:84:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:85:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:102:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:104:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:107:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:111:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:113:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:114:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:115:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:126:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:128:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:131:  Missing space before ( in if(  [whitespace/parens] [5]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:135:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:137:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:138:  Missing spaces around =  [whitespace/operators] [4]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:139:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 28 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 57499 [details]
Proposed fixes to TextBreakIteratorQt

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:40
 +      static const UChar* tempBuf;
write out the names, 'buffer

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:41
 +      static int tempLen;
length

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:50
 +              tempBuf=0;
you need space between tempBuf and the 0

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:51
 +              tempLen=-1;
same here

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:52
 +              type=QTextBoundaryFinder::Word;
and here

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:54
 +          if( tempBuf == string && tempLen == length && type == QTextBoundaryFinder::Word ) {
space after the if and no space before the tempBuf, same at the end

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:60
 +              tempBuf=string;
same

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:61
 +              tempLen=length;
same

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:62
 +              type=QTextBoundaryFinder::Word;
same

Apart form this the code seems quite similar, maybe it makes sense creating an inline method instead (using a template)?

[David Leong]

Created attachment 57500 [details]
patch v2

Thanks for the quick review Kenneth!

I've reworked the patch as you suggested and also ran the style checker to make sure there are no failures.

[Kenneth Rohde Christiansen]

Comment on attachment 57500 [details]
patch v2

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:44
 +      TextBreakIterator* createBreakIterator(const UChar* string, int length, QTextBoundaryFinder::BoundaryType type ) 
There is a space at the end before the ), please remove.

Btw, maybe we should consider making this inline? but that might be unmeasurable anyway.

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:51
 +              prevlength = -1;
Wouldnt 0 do?

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:55
 +          // Try to reuse the iterator if it was previously inited with the same string
initialized, plus a punctuation mark at the end

[Kenneth Rohde Christiansen]

Comment on attachment 57500 [details]
patch v2

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:61
 +              *iterator = QTextBoundaryFinder(type, (const QChar *)string, length, buffer, sizeof(buffer));
Can we use a C++ type cast? That is preferred

Apart from these nits, I'm fine with it. Let's see if Simon has some comments.

[Kenneth Rohde Christiansen]

Btw, remove the line with only //

Please write previousLength instead of prevlength, etc/

57         if (prevstring == string && prevlength == length && prevtype == type) {

Here is it probably better to first then type, then length and then string

 58             // We are the same!

// Same as last time, reuse iterator. 

 59             iterator->toStart();
 60         } else {

 // As the boundaries is often recalculated for the same string due to ... store the iterator for reuse.

 61             *iterator = QTextBoundaryFinder(type, (const QChar *)string, length, buffer, sizeof(buffer));
 62             prevstring = string;
 63             prevlength = length;
 64             prevtype = type;
 65         }

[David Leong]

Created attachment 57508 [details]
Fixed based on Kenneth's latest comments.

[Kenneth Rohde Christiansen]

Comment on attachment 57508 [details]
Fixed based on Kenneth's latest comments.

I would personally have used previousString and not previousstring, as it fits with how we normally name variables, but that is a nit. I'm r+, but please let this sit for at least a day so that others have the change to have a look at it as well.

WebCore/ChangeLog:5
 +          [Qt] TextBreakIterator QT performance
It is called Qt, not QT. QT is short for QuickTime, which can cause confusion as it is used other places in WebKit.

Did you measure if the inlining made any difference?

[David Leong]

Thanks for the review Kenneth! Always good to be perfect. I will be glad to make the changes in the morning w/ the variable names too :)

I tried it on device and didn't notice any difference with inlining, i guess 1s vs 1.1s wasn't noticeable.

[Simon Hausmann]

Comment on attachment 57508 [details]
Fixed based on Kenneth's latest comments.

WebCore/platform/text/qt/TextBreakIteratorQt.cpp:40
 +      static const UChar* previousstring;
The main problem I see with this patch is that this variable becomes a dangling pointer. There is _no_ guarantee at all that the caller of the text iterator functions isn't going to free the memory later. I'm convinced that this is going to cause random crashes at run-time.

One solution I think would be to "fix" the caller side in WebKit to re-use iterator objects - as one way to avoid repeated text analysis. An alternative would be to improve QTextBoundaryFinder to perform the analysis lazily and from within the string and not always from the start.

[David Leong]

Created attachment 57586 [details]
Patch, lets try this again.

Thanks for the review again Kenneth, Simon :)

Updated the patch to use StringImpl's hash as a key to the cached TextBreakIterator. The hash is calculated only once as needed and can be used to identify the string. If the hash and search type is the same, then we re-used the iterator. 

There is no dependency on the string pointer anymore, so it should not crash in the case the string pointer is ever reused with a string that has the same length but different content.

I overloaded the TextBreakIterator functions instead of changing all of them, I am worried it may break other port's builds.

[Simon Hausmann]

(In reply to comment #13)
> Created an attachment (id=57586) [details]
> Patch, lets try this again.
> 
> Thanks for the review again Kenneth, Simon :)
> 
> Updated the patch to use StringImpl's hash as a key to the cached TextBreakIterator. The hash is calculated only once as needed and can be used to identify the string. If the hash and search type is the same, then we re-used the iterator. 
> 
> There is no dependency on the string pointer anymore, so it should not crash in the case the string pointer is ever reused with a string that has the same length but different content.

That's a great idea!

How can we protect ourselves from hash collisions?
 
> I overloaded the TextBreakIterator functions instead of changing all of them, I am worried it may break other port's builds.

The EWS can help us with that :)

[David Leong]

From StringImpl's hash function "http://www.azillionmonkeys.com/qed/hash.html"

"I was shocked to find that there were no significant impediments to this exercise, and I easily found a hash function with all these properties after a few hours or work. I then subjected all realistic sub-bit patterns of the hash output to a simple statistical test and verified that it had a distribution equivalent to a uniformly random map."

The likely hood of a collision in the hash is very unlikely unless the data is exactly the same.

If we need more guarantee we can store and check the length as well as a preventive measure for absolute comparison.

[David Leong]

Simon,

How about we create a TextBreakIterator for each RenderText and store it as a member? The side effect is a cost of 4bytes * text length + size of the QTextBoundaryFinder class. It would not be very efficient for small texts so we would also need to determine with some huristic to use the singleton.

For large texts the memory use would go up quite but that is a trade off for performance.

[Kenneth Rohde Christiansen]

Comments from Dave Hyatt:

On Jun 7, 2010, at 4:18 PM, [e-mail] wrote:
> My question is can we optimize this flow somehow? Can we re-use the 
> TextBreakIterator in the render text class when performing the 
> RenderText::positionForPoint() function?

That's what the ICU implementation does.  I think you just need to fix your implementation to do the same.  If you want to push the re-use higher you could probably make cursorMovementIterator cross-platform and then have it contain the static pointer to the reused iterator.  Then the existing cursorMovementIterator could be renamed and be called by the new cross-platform function.   However you could also just fix your implementation of cursorMovementIterator to reuse the iterator.

static bool createdCursorMovementIterator = false;
static TextBreakIterator* staticCursorMovementIterator;
return setUpIteratorWithRules(createdCursorMovementIterator, staticCursorMovementIterator, kRules, string, length);

[Kenneth Rohde Christiansen]

Comment on attachment 57508 [details]
Fixed based on Kenneth's latest comments.

r- due to Simon's comments

[Kenneth Rohde Christiansen]

Block the release so that this gets cherry-picked.

[Kenneth Rohde Christiansen]

Fix landed in r60847

[Kenneth Rohde Christiansen]

Follow-up fix in r60851

[WebKit Review Bot]

http://trac.webkit.org/changeset/60847 might have broken Qt Linux Release

[Simon Hausmann]

<cherry-pick-for-backport: r60851>

[Simon Hausmann]

r60847 cherry-picked into qtwebkit-4.6 with commit 81a1d730cd489186c8d7eb2c801c2cc06a1dadbd
r60851 cherry-picked into qtwebkit-4.6 with commit d8a9d09376a47b92ea49f1a078c392cbfdbc0ed6

[Simon Hausmann]

Revision r60847 cherry-picked into qtwebkit-2.0 with commit 569e68557ca112761d15646a26b69d9531c78bc3

[Simon Hausmann]

Revision r60847 cherry-picked into qtwebkit-2.0 with commit 569e68557ca112761d15646a26b69d9531c78bc3
Revision r60851 cherry-picked into qtwebkit-2.0 with commit 218151d237627a57159355a2ead39080d4984885

[Raju Kunnath]

I assume after this check-in we are finding the following crash.

Program counter 804313D4 memcompare 003C cmem_.o(.emb_text)

Link register 7EE861F9 memcmp_private 000D stringfuncs_private.o(.text)


This is current stack pointer 00434CC0 7EE861F9 memcmp_private 000D stringfuncs_private.o(.text)

00434CC4 7EE862D5 memcmp 000B memcmp.o(.text)


00434CC8 7BCE2F28 WebCore::setUpIterator(WebCore::TextBreakIterator&, QTextBoundaryFinder::BoundaryType, const unsigned short*, int) 0054 TextBreakIteratorQt.o(.text)

00434CCC 7BD7083C WebCore::RenderText::calcPrefWidths(int) 0048 RenderText.o(.text)


00434CD0 7BCF78F4 WebCore::nextBreakablePosition(const unsigned short*, int, int, bool) 00D0 break_lines.o(.text)

00434CD4 7BD1A508 WebCore::RenderBlock::findNextLineBreak(WebCore::BidiResolver&, bool, bool&, bool&, WebCore::EClear*) 0FA4 RenderBlockLineLayout.o(.text)


00434CD8 7BD1C930 WebCore::RenderBlock::layoutInlineChildren(bool, int&, int&) 0B5C RenderBlockLineLayout.o(.text)

00434CDC 7BBCF250 WebCore::HTMLTokenizer::timerFired(WebCore::Timer*) 0000 HTMLTokenizer.o(.text)


00434CE0 7B82FC00 ...{

00434CE4 7BD28658 WebCore::RenderBoxModelObject::paddingRight(bool) const 004C RenderBoxModelObject.o(.text)


00434CE8 7BD201EC WebCore::RenderBox::calcHorizontalMargins(const WebCore::Length&, const WebCore::Length&, int) 0078 RenderBox.o(.text)

00434CEC 7BD0AF00 WebCore::RenderBlock::layoutBlock(bool) 0554 RenderBlock.o(.text)


00434CF0 7BD10D48 WebCore::RenderBlock::layout() 002C RenderBlock.o(.text)

00434CF4 7BD07A10 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 0328 RenderBlock.o(.text)


00434CF8 7BD09328 WebCore::RenderBlock::layoutBlockChildren(bool, int&) 03B4 RenderBlock.o(.text)

00434CFC 7BD0AF14 WebCore::RenderBlock::layoutBlock(bool) 0568 RenderBlock.o(.text)


00434D00 7BD07BC4 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 04DC RenderBlock.o(.text)

00434D04 7BD10D48 WebCore::RenderBlock::layout() 002C RenderBlock.o(.text)


00434D08 7BD05128 WebCore::RenderBlock::layoutPositionedObjects(bool) 0130 RenderBlock.o(.text)

00434D0C 7BD0B388 WebCore::RenderBlock::layoutBlock(bool) 09DC RenderBlock.o(.text)


00434D14 7BD10D48 WebCore::RenderBlock::layout() 002C RenderBlock.o(.text)

00434D18 7BD07A10 WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 0328 RenderBlock.o(.text)


00434D1C 7BD09328 WebCore::RenderBlock::layoutBlockChildren(bool, int&) 03B4 RenderBlock.o(.text)

00434D20 7BD28658 WebCore::RenderBoxModelObject::paddingRight(bool) const 004C RenderBoxModelObject.o(.text)


00434D24 7BD2128C WebCore::RenderBox::calcWidthUsing(WebCore::WidthType, int) 019C RenderBox.o(.text)

00434D28 7BD0AF14 WebCore::RenderBlock::layoutBlock(bool) 0568 RenderBlock.o(.text)


00434D2C 7EE89A49 free 0007 ualloc.o(.text)

[Suresh Voruganti]

As per the comments # 27, re opening the error and assigning this to Kennth.

Fix need to be cherry picked to Qtwebkit 2.1

[Suresh Voruganti]

Changing the state to NEW

[Kenneth Rohde Christiansen]

(In reply to comment #28)
> As per the comments # 27, re opening the error and assigning this to Kennth.
> 
> Fix need to be cherry picked to Qtwebkit 2.1

I do not have time to look at this before after the christmas holidays, due to other deadlines. So you might want to find someone else to look at it.

[Suresh Voruganti]

Raju, is working on patch, so assigning the issue to Raju

[Raju Kunnath]

Kenneth,
I am planning to make a deep copy of the string that is referenced in Break Iterator. Since this include a memcpy and allocation of memeory, there could be a very little impact on the perf. Please suggest if this correct.

[Raju Kunnath]

Please ignore my previous comment.

[Raju Kunnath]

Created attachment 76304 [details]
fix for crash based on Davids suggestion

[Raju Kunnath]

Kenneth,
Please review patch

[Antonio Gomes]

Comment on attachment 76304 [details]
fix for crash based on Davids suggestion

Attachment is not viewable in your browser because its MIME type (application/octet-stream) is not one that your browser is able to display

[Antonio Gomes]

Comment on attachment 76304 [details]
fix for crash based on Davids suggestion

It needs explanation, a changelog entry and much possibly comments in the code. See http://webkit.org/coding/contributing.html

Please also set the r? flag and make your patch text/plain.

[Raju Kunnath]

Created attachment 76330 [details]
Fix for the crash due to obsolete pointer reference

[Raju Kunnath]

Created attachment 76331 [details]
Fix for crash

updated again

[Andreas Kling]

Comment on attachment 76331 [details]
Fix for crash

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

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:65
> +        if (iterator.isValid() && type == iterator.type() && length == iterator.length && string == iterator.string
>              && memcmp(string, iterator.string, length) == 0) {

This makes no sense.

If string == iterator.string, there's no point in calling memcmp(), and this won't shield against dereferencing deleted pointers anyway, but rather add the restriction that string and iterator.string must point to the same UChar array.

[David Leong]

The pointer compare does help when the same memory address gets recycled by the allocator but the actual contents are different. In this case we would need to reset all of the data in the text break iterator.

[Raju Kunnath]

Here are my understanding.
The pointer comparision helps to avoid the reference to the obsolete pointer.
Also the purpose of the original change is to avoid creation of new iterator many time for same content (where the same referance is passed, with same content in case of mouse event) the memcmp is needed to ensure the content is same.
So I assume this change is needed.

[Andreas Kling]

(In reply to comment #41)
> The pointer compare does help when the same memory address gets recycled by the allocator but the actual contents are different. In this case we would need to reset all of the data in the text break iterator.

If the pointers are equal, then memcmp()ing them will return 0. There's probably even an early return for that case.

[Laszlo Gombos]

(In reply to comment #43)
> (In reply to comment #41)
> > The pointer compare does help when the same memory address gets recycled by the allocator but the actual contents are different. In this case we would need to reset all of the data in the text break iterator.
> 
> If the pointers are equal, then memcmp()ing them will return 0. There's probably even an early return for that case.

The patch looks ok to me if the memcmp() test is removed as Andreas suggested. 

The patch does indeed makes the optimization less useful as the iterator does not get reused as aggressively.

[Raju Kunnath]

Created attachment 76501 [details]
Updated the patch based on the review

Updated the patch as per the review comment.

[WebKit Review Bot]

Attachment 76501 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/text/qt/TextBreakIteratorQt.cpp']" exit_code: 1
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:65:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raju Kunnath]

Created attachment 76504 [details]
Corrected style check

[WebKit Review Bot]

Attachment 76504 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/text/qt/TextBreakIteratorQt.cpp']" exit_code: 1
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:64:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raju Kunnath]

Created attachment 76505 [details]
Corrected again the style issue

[Eric Seidel (no email)]

Comment on attachment 76505 [details]
Corrected again the style issue

You need to mention which tests this fixes (from crashing) in your ChagneLog.

[Raju Kunnath]

Created attachment 76511 [details]
uploaded the patch again

[Laszlo Gombos]

Comment on attachment 76511 [details]
uploaded the patch again

LGTM, r+. Thanks.

[Laszlo Gombos]

Ideally - I think - https://bug-39958-attachments.webkit.org/attachment.cgi?id=76511 should be cherry-picked to qtwebkit-4.6, qtwebkit-2.0 and qtwebkit-2.1 branches.

[Andreas Kling]

Comment on attachment 76511 [details]
uploaded the patch again

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

> WebCore/ChangeLog:10
> +        The previous fix for the performance improvement had a reference to a
> +        pointer which is deleted and hence the memcmp crashed. A check is added
> +        to validate the string is alive or not.

This still makes very little sense.

Do you actually know what you are trying to fix? We need a clear explanation of what the problem is.
I admit I haven't looked at the issue closely yet but it's obvious that this is fixing a symptom and not a root cause.

> WebCore/ChangeLog:15
> +        This change will fix the crash while on panning and scrolling of a page has many div tages.
> +        Many testing are done w.r.t panning and scrolling on mobile phone and emulator. Also this
> +        is a sporadic crash hence performed manual regression tests to check whether the crash is 
> +        reproducible.

This adds no useful information and should be left out.

[Andreas Kling]

After a closer look, it seems to me that for the iterator caching optimization to work reliably, we should hold a proper reference to the string inside our TextBreakIterator.

Either we could change the TextBreakIterator API to take a String object instead of a UChar*/length pair, or we could take a deep copy of the string (as suggested in a comment above.)

[Kenneth Rohde Christiansen]

(In reply to comment #55)
> After a closer look, it seems to me that for the iterator caching optimization to work reliably, we should hold a proper reference to the string inside our TextBreakIterator.
> 
> Either we could change the TextBreakIterator API to take a String object instead of a UChar*/length pair, or we could take a deep copy of the string (as suggested in a comment above.)

The first one seems like the proper way to solve this.

[Kenneth Rohde Christiansen]

(In reply to comment #53)
> Ideally - I think - https://bug-39958-attachments.webkit.org/attachment.cgi?id=76511 should be cherry-picked to qtwebkit-4.6, qtwebkit-2.0 and qtwebkit-2.1 branches.

This patch is bad... you can be unlucky and get the same pointer and still using a different string.

[Raju Kunnath]

Created attachment 76639 [details]
Fixe based on the comments

[WebKit Review Bot]

Attachment 76639 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/text/qt/TextBreakIteratorQt.cpp']" exit_code: 1
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:59:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raju Kunnath]

Created attachment 76640 [details]
Fix as per the commnet

[Andreas Kling]

Comment on attachment 76640 [details]
Fix as per the commnet

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

This solution is OK as a temporary workaround, but we should really fix this properly to avoid deep-copying the string.

r- for using String(UChar*) instead of String(UChar*, int).

> WebCore/ChangeLog:12
> +        The previous fix for the performance improvement had a reference to a
> +        pointer which is deleted and hence the memcmp crashed. A check is added
> +        to validate the string is alive or not. The root cause of this issue is
> +        that in Symbian the Fastallocator claim the memory fast and hence the crash.
> +        Also this is a sporadic crash.

"A check is added to validate the string is alive or not." - This sentence no longer applies.

> WebCore/ChangeLog:14
> +        Tested the patch on QtTestBrowser on S60 devices and windows.

This is implied and can be left out.

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:51
> +            , string("") {} 

Is there any reason to initialize string with String("") instead of String()?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:60
> +        String localString = string;

This will cause an unnecessary loop over 'string' to determine its length. Should be:

String localString(string, length);

[Raju Kunnath]

Created attachment 76661 [details]
Review comments incorporated

Updated the patch with review commnets.

[Raju Kunnath]

Created attachment 76663 [details]
uploading with review comments

Again made mistake is styling...

[WebKit Review Bot]

Attachment 76663 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/platform/text/qt/TextBreakIteratorQt.cpp']" exit_code: 1
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:61:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:67:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/text/qt/TextBreakIteratorQt.cpp:67:  Extra space before )  [whitespace/parens] [2]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raju Kunnath]

Created attachment 76664 [details]
patch

[Raju Kunnath]

Created attachment 76718 [details]
removed extra argument passed to the fucntion

removed extra argument passed to the fucntion

[Kenneth Rohde Christiansen]

(In reply to comment #66)
> Created an attachment (id=76718) [details]
> removed extra argument passed to the fucntion
> 
> removed extra argument passed to the fucntion

It looks OK but aint we back where we started now? I mean we did the original patch as this code path was quite slow and I think this patch brings us back to the original situation.

Have you done any performance measurement

[Raju Kunnath]

Will update the performance numbers

[Raju Kunnath]

Please find the the performance measurement done on N8

FPS for OVi Application is 42.34
Reaction time is 0.128 secs

Please note that this result is based on only one scroll calculation.(To get a better accuracy we need to calculate based on 3 iterations)

[Kenneth Rohde Christiansen]

We need a description of how that was measures, and measurements for 1) without the original patch, 2) with the original patch (which causes crashes), 3) with your patch.

[Raju Kunnath]

The measurement was taken a high speed camera and analysis was done using midas player. Please not that this takes 3 to 4 Hrs. We have the lab setup for doing this.

[Raju Kunnath]

just for information. I got the reading from David with the previous patch and it is arround ~50fps.

[Kenneth Rohde Christiansen]

(In reply to comment #71)
> The measurement was taken a high speed camera and analysis was done using midas player. Please not that this takes 3 to 4 Hrs. We have the lab setup for doing this.

Don't you have easier ways of analyzing this? I guess the performance implications should be quite similar on Linux, and there you can measure these things quite easy. Similar for Mac OS, where you have tools such as Shark (DTrace front-end).

[Kenneth Rohde Christiansen]

(In reply to comment #72)
> just for information. I got the reading from David with the previous patch and it is arround ~50fps.

OK, so we've got:

a) Without patch: ?? FPS
b) With original patch: 50 FPS
c) With improved original patch: 43 FPS.

It would be really interesting in seeing how a) compares with c). Also, No'am made a patch for WebKit half a year ago, making it possible to use ICU. Thought we cannot make Qt depend on ICU, we could use ICU if the platform comes with it, and I believe that Symbian does so.

d) With ICU: ?? FPS.

[Ademar Reis]

Added the last patch (https://bugs.webkit.org/attachment.cgi?id=76718) to the qtwebkit-2.1 branch as 4bed859438727a82f1e9bc56a8a26e2c0082770b (http://gitorious.org/webkit/qtwebkit/commit/4bed859)

[Noam Rosenthal]

> d) With ICU: ?? FPS.

It's really easy to test, just build current webkit with --qmakearg=CONFIG+=text_breaking_with_icu (see https://bugs.webkit.org/show_bug.cgi?id=40332)

But I don't really have the means to measure its FPS in a comparable way. Valgrinding it before showed significant scrolling improvements.

[Holger Freyther]

(In reply to comment #76)
> > d) With ICU: ?? FPS.
> 
> It's really easy to test, just build current webkit with --qmakearg=CONFIG+=text_breaking_with_icu (see https://bugs.webkit.org/show_bug.cgi?id=40332)
> 
> But I don't really have the means to measure its FPS in a comparable way. Valgrinding it before showed significant scrolling improvements.

It shouldn't be that hard to create a reduction. For the test database you have, instrument the text break iterator and let it emit code or data (qdatastream) and then feed the data to your reduction and you will get nice numbers that you can compare while you tweak your implementation...

http://gitorious.org/qtwebkit/performance/trees/master/reductions

[Holger Freyther]

Comment on attachment 76718 [details]
removed extra argument passed to the fucntion

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

I am not convinced based on the commit message. Please explain in the commit message where this speed up is coming from.


> WebCore/ChangeLog:12
> +

I don't understand this message. Are you fixing a crash introduced by a previous commit? Or is this patch improving the performance? Where is this speed up coming from?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:47
> +            : QTextBoundaryFinder(type, string)

This is just using 'QString String::operator()'. Is there any change here? Avoiding a deep copy or such?

> WebCore/platform/text/qt/TextBreakIteratorQt.cpp:-65
> -            && memcmp(string, iterator.string, length) == 0) {

Where is the speed up coming from? Avoiding the memcmp as equal(StringImpl*,StringImpl*) is more clever? Is iterator.isValid() or iterator.type() doing more work?

[Noam Rosenthal]

See https://bugs.webkit.org/show_bug.cgi?id=51653
I think it fixes the problem without touching TextBreakIterator at all, providing that the style for the paragraph has -webkit-user-select: none, which should be true for a mobile/touch list.

[Prasad]

Related QtWRT Bugzilla error ID: http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7297

[Andreas Kling]

This issue was essentially fixed in bug 55139.