Bug 96935 – Add Latin-1 Line Break Iterator to TextBreakIteratorICU.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Michael Saboff]

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.

[Michael Saboff]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

We probably should add a complex line-breaking microbenchmark to PerformanceTests.  I'm sure there is a lot there we could work through yet. :)

[Benjamin Poulain]

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.

[Michael Saboff]

(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.

[Michael Saboff]

(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.

[Eric Seidel (no email)]

(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.

[Benjamin Poulain]

> > > 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.

[Michael Saboff]

(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.

[Michael Saboff]

Created attachment 165218 [details]
Updated Patch

Removed the unneeded explicit initializations.

[Early Warning System Bot]

Comment on attachment 165218 [details]
Updated Patch

Attachment 165218 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13957821

[Gyuyoung Kim]

Comment on attachment 165218 [details]
Updated Patch

Attachment 165218 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13954780

[Early Warning System Bot]

Comment on attachment 165218 [details]
Updated Patch

Attachment 165218 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13949796

[Michael Saboff]

Created attachment 165221 [details]
Patch again

[Geoffrey Garen]

Comment on attachment 165221 [details]
Patch again

r=me

[Michael Saboff]

Committed r129662: <http://trac.webkit.org/changeset/129662>