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.
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.
I have another patch that doesn't require adding member variables to RenderText but isn't optimal w/r/t total ubrk_setText() calls.
Attachment 83225 [details] did not build on qt: Build output: http://queues.webkit.org/results/7944083
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
(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.
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.
Created attachment 83499 [details] Changes for Qt. Let's see if this fixes the Qt build.
Comment on attachment 83499 [details] Changes for Qt. I guess I have to set review? to run this by the build bots.
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.
(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!
(In reply to comment #16) > (From update of attachment 83499 [details]) > Whoops, no change log! Whoops indeed! Coming right up…
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 > +2011-02-23 Ned Holbrook <nholbrook@apple.com> > + > + 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!
Sorry, I'm new here. ;-) Thanks for bearing with me.
Created attachment 83590 [details] Final, final changes.
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.
oh no! tabs!
(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).
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>
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.
(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?
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).
(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.
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 :/> ...
Hopefully the qt bug has been fixed by http://trac.webkit.org/changeset/79567 so a rebased patch won't cause regression.
(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.
Created attachment 83694 [details] Reapplied changes.
Wait, the TextBreakIteratorQt.cpp needs further modification in order to compile with r79567.
Comment on attachment 83694 [details] Reapplied changes. Waiting
Created attachment 83695 [details] Changes for Qt.
Attachment 83695 [details] did not build on qt: Build output: http://queues.webkit.org/results/7982552
Created attachment 83709 [details] Changes for Qt. Fix copy-paste error in TextBreakIteratorQt.cpp.
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. :-(
Attachment 83709 [details] did not build on qt: Build output: http://queues.webkit.org/results/7980608
(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?
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.
(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.
Attachment 83775 [details] did not build on qt: Build output: http://queues.webkit.org/results/8043172
(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.
(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.
(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?
(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.
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>.
Thanks nholbrook@apple.com for fixing this. Just FYI, I also filed a bug similar to this one: https://bugs.webkit.org/show_bug.cgi?id=52715
*** Bug 52715 has been marked as a duplicate of this bug. ***