Currently there is a 16 bit line break iterator provided by the function acquireLineBreakIterator() provided in TextBreakIteratorICU.cpp. To support 8-bit rendering, a 8 bit line break iterator should be added.
Created attachment 164430 [details] Draft patch without build changes for non-Mac platforms Will post an updated patch with changes to other platform that use TextBreakIteratorICU.cpp. The change is neutral in performance on the Layout tests.
I assume by LayoutTests you mean PLT? or PerformanceTests? Do we have a complex-text line-breaking perf test? PerformanceTests/Layout/line-breaking.html only does the ascii case.
We probably should add a complex line-breaking microbenchmark to PerformanceTests. I'm sure there is a lot there we could work through yet. :)
Comment on attachment 164430 [details] Draft patch without build changes for non-Mac platforms View in context: https://bugs.webkit.org/attachment.cgi?id=164430&action=review > Source/WebCore/platform/text/TextBreakIterator.h:62 > + : m_string(String()) > + , m_locale(AtomicString()) You can remove those. > Source/WebCore/rendering/break_lines.cpp:151 > +template<typename CharacterType, bool treatNoBreakSpaceAsBreak> > +static inline int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, const CharacterType* str, unsigned length, int pos) Chances are we should fully specialize that function instead. I can do that after your patch if you want.
(In reply to comment #2) > I assume by LayoutTests you mean PLT? or PerformanceTests? > > Do we have a complex-text line-breaking perf test? PerformanceTests/Layout/line-breaking.html only does the ascii case. I mean PerformanceTests/Layout. I don't know if we have a complex text test for layout. This patch doesn't enable using the 8 bit code path as Text nodes and thus RenderText objects are 16 bit until we enable the 8 bit path in the HTML parser.
(In reply to comment #4) > (From update of attachment 164430 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164430&action=review > > > Source/WebCore/platform/text/TextBreakIterator.h:62 > > + : m_string(String()) > > + , m_locale(AtomicString()) > > You can remove those. Done. > > Source/WebCore/rendering/break_lines.cpp:151 > > +template<typename CharacterType, bool treatNoBreakSpaceAsBreak> > > +static inline int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, const CharacterType* str, unsigned length, int pos) > > Chances are we should fully specialize that function instead. > > I can do that after your patch if you want. Please elaborate.
(In reply to comment #5) > (In reply to comment #2) > > I assume by LayoutTests you mean PLT? or PerformanceTests? > > > > Do we have a complex-text line-breaking perf test? PerformanceTests/Layout/line-breaking.html only does the ascii case. > > I mean PerformanceTests/Layout. I don't know if we have a complex text test for layout. > > This patch doesn't enable using the 8 bit code path as Text nodes and thus RenderText objects are 16 bit until we enable the 8 bit path in the HTML parser. I see. So this path is as of yet unused. Thanks. We should probably catch mitz or jungshik's ear about adding some complex-text Layout PerformanceTests as thats an area we could micro-benchmark more and probably eek more performance out of.
> > > Source/WebCore/rendering/break_lines.cpp:151 > > > +template<typename CharacterType, bool treatNoBreakSpaceAsBreak> > > > +static inline int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, const CharacterType* str, unsigned length, int pos) > > > > Chances are we should fully specialize that function instead. > > > > I can do that after your patch if you want. > > Please elaborate. If you look at nextBreakablePosition(), there is fast path (first if()), and a slower patch for non-ASCII characters (second if()). Chances are we could generalize the fast path and get rid of the second if() for Latin1. What takes time in that particular loop is the number of branches needed for each character. If we could get rid of the second if(), it would likely be a big win.
(In reply to comment #8) > > > > Source/WebCore/rendering/break_lines.cpp:151 > > > > +template<typename CharacterType, bool treatNoBreakSpaceAsBreak> > > > > +static inline int nextBreakablePosition(LazyLineBreakIterator& lazyBreakIterator, const CharacterType* str, unsigned length, int pos) > > > > > > Chances are we should fully specialize that function instead. > > > > > > I can do that after your patch if you want. > > > > Please elaborate. > > If you look at nextBreakablePosition(), there is fast path (first if()), and a slower patch for non-ASCII characters (second if()). > > Chances are we could generalize the fast path and get rid of the second if() for Latin1. > > What takes time in that particular loop is the number of branches needed for each character. If we could get rid of the second if(), it would likely be a big win. We could do that in this patch or as a follow on. We would have to generate the new entries for the line break table.
Created attachment 165218 [details] Updated Patch Removed the unneeded explicit initializations.
Comment on attachment 165218 [details] Updated Patch Attachment 165218 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13957821
Comment on attachment 165218 [details] Updated Patch Attachment 165218 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13954780
Comment on attachment 165218 [details] Updated Patch Attachment 165218 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13949796
Created attachment 165221 [details] Patch again
Comment on attachment 165221 [details] Patch again r=me
Committed r129662: <http://trac.webkit.org/changeset/129662>