Bug 54912 – Minimize calls to ubrk_setText()

• Component: Text
• Status: RESOLVED FIXED

[Ned Holbrook]

Text requiring ICU line breaking involves many calls to ubrk_setText(), or one per invocation of WebCore::isBreakable(). It should be possible to only call ubrk_setText() once per RenderText.

For example, a test app drawing a paragraph of Japanese text currently requires 164 ubrk_setText() calls, but only one is strictly necessary in this case. A quick benchmark of this case showed a 3% overall speedup over 1000 iterations.

[Ned Holbrook]

Created attachment 83225 [details]
Proposed changes.

Comment on attachment 83225 [details]
Proposed changes.

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

> Source/WebCore/rendering/RenderText.h:162
> +    LazyLineBreakIterator m_lineBreakIterator;

Increasing the size of RenderText by 16 bytes (I think) is going to have significant impact on memory use.

[Ned Holbrook]

I have another patch that doesn't require adding member variables to RenderText but isn't optimal w/r/t total ubrk_setText() calls.

[Early Warning System Bot]

Attachment 83225 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7944083

[Ned Holbrook]

Created attachment 83343 [details]
Modified changes.

This patch avoids adding the RenderText member variable. At least for my test case the number of ubrk_setText() calls is unchanged relative to the previous patch; I can't guarantee it is optimal in all cases, but it it certainly better than what's there now.

Comment on attachment 83343 [details]
Modified changes.

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

Looks good! A few coding style comments below.

> Source/WebCore/platform/text/TextBreakIterator.h:63
> +    LazyLineBreakIterator(const UChar* string = 0, int length = 0)

Are you using these default parameters? If not, please remove them.

> Source/WebCore/platform/text/TextBreakIterator.h:68
> +    { }
> +

WebKit style is to do
{
}

> Source/WebCore/platform/text/TextBreakIterator.h:84
> +    }
> +    void reset(const UChar* string, int length)

Missing newline here

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:81
> +        setUpIterator(createdLineBreakIterator,
> +            staticLineBreakIterator, UBRK_LINE, string, length);

No reason to split this across two lines.

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:94
> + {

Leading whitespace

[Ned Holbrook]

(In reply to comment #6)
> > Source/WebCore/platform/text/TextBreakIterator.h:63
> > +    LazyLineBreakIterator(const UChar* string = 0, int length = 0)
> 
> Are you using these default parameters? If not, please remove them.

I am: the LineBreakIteratorInfo std::pair invokes the default constructor.

[Ned Holbrook]

Created attachment 83351 [details]
Changes per review.

Comment on attachment 83351 [details]
Changes per review.

Ok. Let’s see what the EWS bots think. Not sure how this affects non-ICU builds.

So there are no EWS results yet, but the red Qt box next to the first patch suggests that this will break the Qt build, which doesn't use ICU.

[Ned Holbrook]

Created attachment 83499 [details]
Changes for Qt.

Let's see if this fixes the Qt build.

[Ned Holbrook]

Comment on attachment 83499 [details]
Changes for Qt.

I guess I have to set review? to run this by the build bots.

[Darin Adler]

Comment on attachment 83499 [details]
Changes for Qt.

Clearing review flag now that this is in the EWS queue. The EWS bots will still build this since it had review? at one point.

[Ned Holbrook]

(In reply to comment #13)
> Clearing review flag now that this is in the EWS queue. The EWS bots will still build this since it had review? at one point.

Great, thanks!

Comment on attachment 83499 [details]
Changes for Qt.

This seems to build everywhere (I trust that Ned has built on Mac OS X). Ned, would you like to change the review flag to '?' again?

Comment on attachment 83499 [details]
Changes for Qt.

Whoops, no change log!

[Ned Holbrook]

(In reply to comment #16)
> (From update of attachment 83499 [details])
> Whoops, no change log!

Whoops indeed! Coming right up…

[Ned Holbrook]

Created attachment 83579 [details]
Final changes.

Comment on attachment 83579 [details]
Final changes.

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

> Source/WebCore/ChangeLog:34
> [phone]  Ned Holbrook  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Minimize calls to ubrk_setText()
> +        https://bugs.webkit.org/show_bug.cgi?id=54912
> +        <rdar://problem/9032774>
> +
> +        No new tests. (OOPS!)
> +
> +        * platform/text/TextBreakIterator.h:
> +        (WebCore::LazyLineBreakIterator::LazyLineBreakIterator):
> +        (WebCore::LazyLineBreakIterator::~LazyLineBreakIterator):
> +        (WebCore::LazyLineBreakIterator::string):
> +        (WebCore::LazyLineBreakIterator::length):
> +        (WebCore::LazyLineBreakIterator::get):
> +        (WebCore::LazyLineBreakIterator::reset):
> +        * platform/text/TextBreakIteratorICU.cpp:
> +        (WebCore::acquireLineBreakIterator):
> +        (WebCore::releaseLineBreakIterator):
> +        * platform/text/qt/TextBreakIteratorQt.cpp:
> +        (WebCore::acquireLineBreakIterator):
> +        (WebCore::releaseLineBreakIterator):
> +        * rendering/RenderBlock.h:
> +        * rendering/RenderBlockLineLayout.cpp:
> +        (WebCore::RenderBlock::layoutInlineChildren):
> +        (WebCore::RenderBlock::findNextLineBreak):
> +        * rendering/RenderText.cpp:
> +        (WebCore::RenderText::computePreferredLogicalWidths):
> +        * rendering/break_lines.cpp:
> +        (WebCore::nextBreakablePosition):
> +        * rendering/break_lines.h:
> +        (WebCore::isBreakable):
> +

Sorry, Ned, this is not how it works (even if some contributors sometimes get away with it) :-(.

For a change like this, at the very least you should give an overview of the change before the file and function listing. This would also be a great place to say by how much the change reduces calls to ubrk_setText(). Alternatively, or in addition to this, you should fill out the function listing with comments about the changes in each function, where it makes sense.

Finally, you should remove the “No new tests. (OOPS!)” line. The change log can’t be committed with that line, and instead you can comment that there are no new tests because this is only a performance optimization. But just removing the line is also fine, in my opinion.

Thanks!

[Ned Holbrook]

Sorry, I'm new here. ;-) Thanks for bearing with me.

[Ned Holbrook]

Created attachment 83590 [details]
Final, final changes.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 5 in 9 files


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

[Adele Peterson]

oh no! tabs!

[Ned Holbrook]

(In reply to comment #23)
> oh no! tabs!

You would not believe the profanities I just uttered. :-P

Comment on attachment 83590 [details]
Final, final changes.

I’ll fix the tabs in the change log when committing (the pre-commit hook won’t allow tabs to be checked in).

[Ned Holbrook]

Created attachment 83591 [details]
This had better be it.

The gazillionth time's the charm, right?

Committed r79510.
<http://trac.webkit.org/projects/webkit/changeset/79510>

[Csaba Osztrogonác]

Reopen, because it was rolled out http://trac.webkit.org/changeset/79518.

It made ~200 tests crash on Qt bot. I'm going to attach a crash log to help finding whar happened.

[Ned Holbrook]

(In reply to comment #28)
> Reopen, because it was rolled out http://trac.webkit.org/changeset/79518.
> 
> It made ~200 tests crash on Qt bot. I'm going to attach a crash log to help finding whar happened.

Thanks, I was hoping my untested code worked, but I guess this puts the lie to that. Is there a quicker way to test my next attempt short of waiting for tests to fail again?

[Csaba Osztrogonác]

Longer way, but you have to install once the QtWebKit builder and tester VM:
http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine

Shorter way: you upload patch to bugzilla or gist or ... and ping me on #webkit.
My nick is ossy, and I can run a quick test between ~10:00-20:00 (CET).

[Ned Holbrook]

(In reply to comment #30)

Thanks. Any further patches from me are going to have to wait until tomorrow, so I'll look for a crash log then and see what needs to be done.

[Csaba Osztrogonác]

Created attachment 83627 [details]
glibc backtrace

$ gdb WebKitBuild/Debug/bin/DumpRenderTree
(gdb) run css1/text_properties/text_transform.html
*** glibc detected *** /home/oszi/WebKit/WebKitBuild/Debug/bin/DumpRenderTree: realloc(): invalid pointer: 0xf77b4120 ***
======= Backtrace: =========
...
<attached>
...

Program received signal SIGABRT, Aborted.
(gdb) bt
...
<absolutely useless backtrace :/>
...

[Balazs Kelemen]

Hopefully the qt bug has been fixed by http://trac.webkit.org/changeset/79567 so
a rebased patch won't cause regression.

[Ned Holbrook]

(In reply to comment #33)
> Hopefully the qt bug has been fixed by http://trac.webkit.org/changeset/79567 so
> a rebased patch won't cause regression.

I'll rebase the patch and we can try this again.

[Ned Holbrook]

Created attachment 83694 [details]
Reapplied changes.

[Ned Holbrook]

Wait, the TextBreakIteratorQt.cpp needs further modification in order to compile with r79567.

Comment on attachment 83694 [details]
Reapplied changes.

Waiting

[Ned Holbrook]

Created attachment 83695 [details]
Changes for Qt.

[Early Warning System Bot]

Attachment 83695 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7982552

[Ned Holbrook]

Created attachment 83709 [details]
Changes for Qt.

Fix copy-paste error in TextBreakIteratorQt.cpp.

[Patrick R. Gansterer]

Can you please also add changes to TextBreakIteratorWinCE.cpp? The last change broke WinCE build bot.
I wanted to add the needed changes as patch here, but didn't found the time to create it today. :-(

[Early Warning System Bot]

Attachment 83709 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7980608

[Ned Holbrook]

(In reply to comment #42)
> Attachment 83709 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/7980608

The build output makes no sense to me: the line numbers don't correspond to my copy of the source, plus it's complaining about "no matching function for call to ‘WebCore::TextBreakIterator::TextBreakIterator(QTextBoundaryFinder::BoundaryType, QString)’" which I'm pretty certain is on line 46. Can someone help me figure this out?

[Ned Holbrook]

Created attachment 83775 [details]
Changes for additional platforms.

Let's see how this one goes, build-wise.

Comment on attachment 83775 [details]
Changes for additional platforms.

Setting r? to get builders’ attention.

[Csaba Osztrogonác]

(In reply to comment #45)
> (From update of attachment 83775 [details])
> Setting r? to get builders’ attention.

It works on Qt now, and all tests pass. Thanks for fixining.

[Early Warning System Bot]

Attachment 83775 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8043172

[Csaba Osztrogonác]

(In reply to comment #47)
> Attachment 83775 [details] did not build on qt:
> Build output: http://queues.webkit.org/results/8043172

OMG. :-S Qt EWS is stucked on r78989, but the ToT is r79669 now.
Eric, have you got any idea how is it possible?

Otherwise the patch works on ToT with Qt, as I said before.

[Eric Seidel (no email)]

(In reply to comment #48)
> (In reply to comment #47)
> > Attachment 83775 [details] [details] did not build on qt:
> > Build output: http://queues.webkit.org/results/8043172
> 
> OMG. :-S Qt EWS is stucked on r78989, but the ToT is r79669 now.
> Eric, have you got any idea how is it possible?
> 
> Otherwise the patch works on ToT with Qt, as I said before.

I suspect the git checkout on that bot got confused.

[Csaba Osztrogonác]

(In reply to comment #49)
> I suspect the git checkout on that bot got confused.
I got it, update-webkit is failing now. I'm going to fix it.
Is it possible to make EWS not trying build if update failed?

[Eric Seidel (no email)]

(In reply to comment #50)
> (In reply to comment #49)
> > I suspect the git checkout on that bot got confused.
> I got it, update-webkit is failing now. I'm going to fix it.
> Is it possible to make EWS not trying build if update failed?

Bug 49395.  A lot like bug 46636.

[Darin Adler]

Comment on attachment 83775 [details]
Changes for additional platforms.

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

Some comments about slight improvements. These could wait until afterward, though. I do not mean to reverse Mitz’s review+ with these.

> Source/WebCore/platform/text/TextBreakIterator.h:61
> +class LazyLineBreakIterator {

Should probably mark this non-copyable with the macro.

> Source/WebCore/platform/text/TextBreakIterator.h:79
> +    TextBreakIterator* get()

I’d prefer a noun name for this rather than a verb. Maybe iterator().

> Source/WebCore/platform/text/TextBreakIteratorICU.cpp:86
> +        bool createdNewLineBreakIterator = false;
> +        setUpIterator(createdNewLineBreakIterator, lineBreakIterator, UBRK_LINE, string, length);

I think the local variable should have a name that makes it clear it’s not the same as the global. And why.

I committed <http://trac.webkit.org/projects/webkit/changeset/79694>.

[Xianzhu Wang]

Thanks [e-mail] for fixing this.

Just FYI, I also filed a bug similar to this one: https://bugs.webkit.org/show_bug.cgi?id=52715

[Xianzhu Wang]

*** Bug 52715 has been marked as a duplicate of this bug. ***