Bug 39958 - [Qt] TextBreakIteratorQt performance
Summary: [Qt] TextBreakIteratorQt performance
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: S60 Hardware S60 3rd edition
: P1 Critical
Assignee: Raju Kunnath
URL:
Keywords: Qt
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-05-31 09:16 PDT by David Leong
Modified: 2011-05-10 13:44 PDT (History)
17 users (show)

See Also:


Attachments
Proposed fixes to TextBreakIteratorQt (5.25 KB, patch)
2010-05-31 16:18 PDT, David Leong
kenneth: review-
Details | Formatted Diff | Diff
patch v2 (4.43 KB, patch)
2010-05-31 17:05 PDT, David Leong
no flags Details | Formatted Diff | Diff
Fixed based on Kenneth's latest comments. (4.77 KB, patch)
2010-05-31 19:33 PDT, David Leong
kenneth: review-
kenneth: commit-queue-
Details | Formatted Diff | Diff
Patch, lets try this again. (11.17 KB, patch)
2010-06-01 13:12 PDT, David Leong
no flags Details | Formatted Diff | Diff
fix for crash based on Davids suggestion (601 bytes, patch)
2010-12-11 05:41 PST, Raju Kunnath
tonikitoo: review-
Details | Formatted Diff | Diff
Fix for the crash due to obsolete pointer reference (1.50 KB, patch)
2010-12-12 09:04 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
Fix for crash (1.46 KB, patch)
2010-12-12 09:24 PST, Raju Kunnath
akling: review-
Details | Formatted Diff | Diff
Updated the patch based on the review (1.47 KB, patch)
2010-12-13 22:31 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
Corrected style check (1.46 KB, patch)
2010-12-13 22:57 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
Corrected again the style issue (1.46 KB, patch)
2010-12-13 23:09 PST, Raju Kunnath
eric: review-
Details | Formatted Diff | Diff
uploaded the patch again (1.79 KB, patch)
2010-12-14 01:09 PST, Raju Kunnath
akling: review-
Details | Formatted Diff | Diff
Fixe based on the comments (2.90 KB, patch)
2010-12-15 05:28 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
Fix as per the commnet (2.91 KB, patch)
2010-12-15 05:36 PST, Raju Kunnath
akling: review-
Details | Formatted Diff | Diff
Review comments incorporated (2.75 KB, patch)
2010-12-15 10:10 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
uploading with review comments (2.76 KB, patch)
2010-12-15 10:24 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
patch (2.76 KB, patch)
2010-12-15 10:30 PST, Raju Kunnath
no flags Details | Formatted Diff | Diff
removed extra argument passed to the fucntion (2.74 KB, patch)
2010-12-15 17:42 PST, Raju Kunnath
zecke: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Leong 2010-05-31 09:16:56 PDT
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.
Comment 1 Kenneth Rohde Christiansen 2010-05-31 09:20:04 PDT
cc'ing Simon, as he is the Harfbuzz expert ;-)
Comment 2 David Leong 2010-05-31 16:18:01 PDT
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.
Comment 3 WebKit Review Bot 2010-05-31 16:22:57 PDT
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.
Comment 4 Kenneth Rohde Christiansen 2010-05-31 16:25:11 PDT
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)?
Comment 5 David Leong 2010-05-31 17:05:43 PDT
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.
Comment 6 Kenneth Rohde Christiansen 2010-05-31 18:23:17 PDT
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
Comment 7 Kenneth Rohde Christiansen 2010-05-31 18:24:44 PDT
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.
Comment 8 Kenneth Rohde Christiansen 2010-05-31 18:31:25 PDT
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         }
Comment 9 David Leong 2010-05-31 19:33:25 PDT
Created attachment 57508 [details]
Fixed based on Kenneth's latest comments.
Comment 10 Kenneth Rohde Christiansen 2010-05-31 19:39:32 PDT
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?
Comment 11 David Leong 2010-05-31 21:09:38 PDT
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.
Comment 12 Simon Hausmann 2010-06-01 03:22:07 PDT
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.
Comment 13 David Leong 2010-06-01 13:12:00 PDT
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.
Comment 14 Simon Hausmann 2010-06-02 03:45:05 PDT
(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 :)
Comment 15 David Leong 2010-06-02 11:18:38 PDT
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.
Comment 16 David Leong 2010-06-02 13:32:34 PDT
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.
Comment 17 Kenneth Rohde Christiansen 2010-06-08 04:53:38 PDT
Comments from Dave Hyatt:

On Jun 7, 2010, at 4:18 PM, David.Leong@nokia.com 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);
Comment 18 Kenneth Rohde Christiansen 2010-06-08 10:03:05 PDT
Comment on attachment 57508 [details]
Fixed based on Kenneth's latest comments.

r- due to Simon's comments
Comment 19 Kenneth Rohde Christiansen 2010-06-08 10:04:47 PDT
Block the release so that this gets cherry-picked.
Comment 20 Kenneth Rohde Christiansen 2010-06-08 10:05:13 PDT
Fix landed in r60847
Comment 21 Kenneth Rohde Christiansen 2010-06-08 10:35:16 PDT
Follow-up fix in r60851
Comment 22 WebKit Review Bot 2010-06-08 10:36:24 PDT
http://trac.webkit.org/changeset/60847 might have broken Qt Linux Release
Comment 23 Simon Hausmann 2010-06-10 01:12:33 PDT
<cherry-pick-for-backport: r60851>
Comment 24 Simon Hausmann 2010-06-10 01:14:04 PDT
r60847 cherry-picked into qtwebkit-4.6 with commit 81a1d730cd489186c8d7eb2c801c2cc06a1dadbd
r60851 cherry-picked into qtwebkit-4.6 with commit d8a9d09376a47b92ea49f1a078c392cbfdbc0ed6
Comment 25 Simon Hausmann 2010-06-10 01:17:37 PDT
Revision r60847 cherry-picked into qtwebkit-2.0 with commit 569e68557ca112761d15646a26b69d9531c78bc3
Comment 26 Simon Hausmann 2010-06-10 01:19:56 PDT
Revision r60847 cherry-picked into qtwebkit-2.0 with commit 569e68557ca112761d15646a26b69d9531c78bc3
Revision r60851 cherry-picked into qtwebkit-2.0 with commit 218151d237627a57159355a2ead39080d4984885
Comment 27 Raju Kunnath 2010-12-09 00:53:49 PST
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)
Comment 28 Suresh Voruganti 2010-12-09 09:14:50 PST
As per the comments # 27, re opening the error and assigning this to Kennth.

Fix need to be cherry picked to Qtwebkit 2.1
Comment 29 Suresh Voruganti 2010-12-09 09:15:44 PST
Changing the state to NEW
Comment 30 Kenneth Rohde Christiansen 2010-12-09 10:07:10 PST
(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.
Comment 31 Suresh Voruganti 2010-12-10 12:01:57 PST
Raju, is working on patch, so assigning the issue to Raju
Comment 32 Raju Kunnath 2010-12-10 23:41:47 PST
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.
Comment 33 Raju Kunnath 2010-12-11 03:52:48 PST
Please ignore my previous comment.
Comment 34 Raju Kunnath 2010-12-11 05:41:08 PST
Created attachment 76304 [details]
fix for crash based on Davids suggestion
Comment 35 Raju Kunnath 2010-12-11 05:43:20 PST
Kenneth,
Please review patch
Comment 36 Antonio Gomes 2010-12-12 07:28:18 PST
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
Comment 37 Antonio Gomes 2010-12-12 07:30:21 PST
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.
Comment 38 Raju Kunnath 2010-12-12 09:04:36 PST
Created attachment 76330 [details]
Fix for the crash due to obsolete pointer reference
Comment 39 Raju Kunnath 2010-12-12 09:24:37 PST
Created attachment 76331 [details]
Fix for crash

updated again
Comment 40 Andreas Kling 2010-12-12 09:30:41 PST
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.
Comment 41 David Leong 2010-12-12 16:30:10 PST
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.
Comment 42 Raju Kunnath 2010-12-12 19:58:35 PST
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.
Comment 43 Andreas Kling 2010-12-13 06:51:39 PST
(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.
Comment 44 Laszlo Gombos 2010-12-13 14:27:17 PST
(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.
Comment 45 Raju Kunnath 2010-12-13 22:31:22 PST
Created attachment 76501 [details]
Updated the patch based on the review

Updated the patch as per the review comment.
Comment 46 WebKit Review Bot 2010-12-13 22:34:16 PST
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.
Comment 47 Raju Kunnath 2010-12-13 22:57:42 PST
Created attachment 76504 [details]
Corrected style check
Comment 48 WebKit Review Bot 2010-12-13 23:00:37 PST
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.
Comment 49 Raju Kunnath 2010-12-13 23:09:43 PST
Created attachment 76505 [details]
Corrected again the style issue
Comment 50 Eric Seidel 2010-12-14 00:48:23 PST
Comment on attachment 76505 [details]
Corrected again the style issue

You need to mention which tests this fixes (from crashing) in your ChagneLog.
Comment 51 Raju Kunnath 2010-12-14 01:09:08 PST
Created attachment 76511 [details]
uploaded the patch again
Comment 52 Laszlo Gombos 2010-12-14 05:54:41 PST
Comment on attachment 76511 [details]
uploaded the patch again

LGTM, r+. Thanks.
Comment 53 Laszlo Gombos 2010-12-14 06:01:26 PST
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.
Comment 54 Andreas Kling 2010-12-14 06:08:43 PST
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.
Comment 55 Andreas Kling 2010-12-14 07:10:25 PST
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.)
Comment 56 Kenneth Rohde Christiansen 2010-12-14 12:48:38 PST
(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.
Comment 57 Kenneth Rohde Christiansen 2010-12-14 12:50:43 PST
(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.
Comment 58 Raju Kunnath 2010-12-15 05:28:31 PST
Created attachment 76639 [details]
Fixe based on the comments
Comment 59 WebKit Review Bot 2010-12-15 05:30:19 PST
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.
Comment 60 Raju Kunnath 2010-12-15 05:36:20 PST
Created attachment 76640 [details]
Fix as per the commnet
Comment 61 Andreas Kling 2010-12-15 08:44:28 PST
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);
Comment 62 Raju Kunnath 2010-12-15 10:10:17 PST
Created attachment 76661 [details]
Review comments incorporated

Updated the patch with review commnets.
Comment 63 Raju Kunnath 2010-12-15 10:24:56 PST
Created attachment 76663 [details]
uploading with review comments

Again made mistake is styling...
Comment 64 WebKit Review Bot 2010-12-15 10:26:06 PST
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.
Comment 65 Raju Kunnath 2010-12-15 10:30:38 PST
Created attachment 76664 [details]
patch
Comment 66 Raju Kunnath 2010-12-15 17:42:33 PST
Created attachment 76718 [details]
removed extra argument passed to the fucntion

removed extra argument passed to the fucntion
Comment 67 Kenneth Rohde Christiansen 2010-12-16 05:58:28 PST
(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
Comment 68 Raju Kunnath 2010-12-16 20:16:53 PST
Will update the performance numbers
Comment 69 Raju Kunnath 2010-12-17 06:13:56 PST
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)
Comment 70 Kenneth Rohde Christiansen 2010-12-17 06:40:12 PST
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.
Comment 71 Raju Kunnath 2010-12-17 07:36:02 PST
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.
Comment 72 Raju Kunnath 2010-12-17 08:21:00 PST
just for information. I got the reading from David with the previous patch and it is arround ~50fps.
Comment 73 Kenneth Rohde Christiansen 2010-12-18 11:13:00 PST
(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).
Comment 74 Kenneth Rohde Christiansen 2010-12-18 11:17:27 PST
(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.
Comment 75 Ademar Reis 2010-12-20 13:05:11 PST
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)
Comment 76 Noam Rosenthal 2010-12-22 14:34:48 PST
> 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.
Comment 77 Holger Freyther 2010-12-24 02:09:46 PST
(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
Comment 78 Holger Freyther 2010-12-24 02:26:21 PST
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?
Comment 79 Noam Rosenthal 2010-12-27 13:37:12 PST
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.
Comment 80 Prasad 2011-01-06 13:57:41 PST
Related QtWRT Bugzilla error ID: http://bugs.nokia-boston.com/bugzilla/show_bug.cgi?id=7297
Comment 81 Andreas Kling 2011-03-23 07:47:36 PDT
This issue was essentially fixed in bug 55139.