Bug 96979 – Update RenderText to use String instead of UChar* for text

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

[Michael Saboff]

This change is part of adding 8 bit string handling to the rendering code.

[Michael Saboff]

Created attachment 166782 [details]
Patch

Here is the performance results of this patch on the Performance/LayoutTests:

                        Baseline    With Changes
flexbox-column-nowrap   102202.6        105725.9   runs/s
flexbox-column-nowrap   102850.4        105806.4
flexbox-column-nowrap   103790.0        107004.1
flexbox-column-nowrap   103997.7        107176.2
    Mean                103210.2        106428.2   Diff    3.12%
flexbox-column-wrap     102530.4        104567.0   runs/s
flexbox-column-wrap     105323.5        107327.2
flexbox-column-wrap     107329.5        107771.2
flexbox-column-wrap     107347.2        109178.8
    Mean                105632.7        107211.0   Diff    1.49%
flexbox-row-nowrap      138271.6        142910.7   runs/s
flexbox-row-nowrap      140718.1        143366.9
flexbox-row-nowrap      144811.8        144682.2
flexbox-row-nowrap      146147.0        150649.8
    Mean                142487.1        145402.4   Diff    2.05%
flexbox-row-wrap        103482.7        104720.6   runs/s       
flexbox-row-wrap        103778.7        106867.7                
flexbox-row-wrap        104554.4        108036.3        
flexbox-row-wrap        105866.3        108196.2        
    Mean                104420.5        106955.2   Diff    2.43%
floats_100_100             200.0           198.0   ms 
floats_100_100             201.0           200.5
floats_100_100             201.5           201.0   
floats_100_100             202.5           201.5   
    Mean                   201.3           200.3   Diff    0.50%
floats_100_100_nested      215.5           216.0   ms
floats_100_100_nested      215.5           216.5
floats_100_100_nested      217.5           218.0   
floats_100_100_nested      218.5           221.5   
    Mean                   216.8           218.0   Diff   -0.58%
floats_20_100              409.9           414.9   ms
floats_20_100              411.4           416.3
floats_20_100              413.0           416.4   
floats_20_100              416.3           419.1   
    Mean                   412.6           416.7   Diff   -0.98%
floats_20_100_nested       612.3           619.8   ms
floats_20_100_nested       618.3           622.7          
floats_20_100_nested       635.2           625.2   
floats_20_100_nested       635.7           628.7   
    Mean                   625.4           624.1   Diff    0.21%
floats_2_100               204.6           209.5   ms
floats_2_100               205.7           210.4
floats_2_100               205.9           210.4   
floats_2_100               206.0           210.9   
    Mean                   205.6           210.3   Diff   -2.31%
floats_2_100_nested        463.6           460.5   ms
floats_2_100_nested        463.9           461.5
floats_2_100_nested        464.5           462.6   
floats_2_100_nested        466.7           463.2   
    Mean                   464.7           462.0   Diff    0.59%
floats_50_100              305.6           313.4   ms     
floats_50_100              312.4           314.6
floats_50_100              313.0           314.8   
floats_50_100              314.4           317.8   
    Mean                   311.4           315.2   Diff   -1.22%
floats_50_100_nested       367.6           371.8   ms     
floats_50_100_nested       370.8           373.2
floats_50_100_nested       371.8           373.6   
floats_50_100_nested       375.6           377.0   
    Mean                   371.5           373.9   Diff   -0.66%
line-layout                 79.6            83.3   runs/s
line-layout                 80.0            83.7 
line-layout                 80.1            83.9    
line-layout                 80.1            83.9    
    Mean                    80.0            83.7   Diff    4.70%


I don't have the details for the slowdown in floats_20_100, floats_2_100 and floats_50_100.  Profiling doesn't point to cade I changed.

[Early Warning System Bot]

Comment on attachment 166782 [details]
Patch

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

[Gyuyoung Kim]

Comment on attachment 166782 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 166782 [details]
Patch

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

[Michael Saboff]

Created attachment 166785 [details]
Patch with PLATFORM(MAC) ifdef's removed

This should fix the build errors for  other platforms.

[WebKit Review Bot]

Comment on attachment 166785 [details]
Patch with PLATFORM(MAC) ifdef's removed

Attachment 166785 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14132473

New failing tests:
fast/text/offsetForPosition-complex-fallback.html
css3/font-feature-settings-rendering.html
fast/text/word-space-with-kerning.html
fast/text/shaping/shaping-selection-rect.html
fast/text/justify-ideograph-leading-expansion.html
fast/text/font-kerning.html
fast/text/complex-preferred-logical-widths.html
fast/css/text-rendering.html
fast/text/word-space-with-kerning-2.html
fast/text/international/text-spliced-font.html
fast/text/font-variant-ligatures.html

[Dirk Pranke]

Created attachment 166967 [details]
sample test run from chromium-linux@r130322 + patch

here you go ...

[Dirk Pranke]

Created attachment 167004 [details]
results from a debug build

results from a debug build ... lots of crashes.

[Michael Saboff]

Created attachment 167142 [details]
Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

The problems found by the chromium test bot is due to the chromium platform code expecting all TextRun's contain 16 bit text data.  A quick check shows that the other platforms have this issue.

Added a new ENABLE_CAN_USE_8BIT_TEXTRUN macro to Platform.h and set it for PLATFORM(MAC).  Other platforms will need to update their platform code that uses TextRun's to handle 8 bit text.

Also wrapped debug only code in #ifndef NDEBUG.  This took care of the performance issues.  New results:

                        Baseline    With Changes
flexbox-column-nowrap   102202.6        106557.6   runs/s
flexbox-column-nowrap   102850.4        107438.7
flexbox-column-nowrap   103790.0        107801.1
flexbox-column-nowrap   103997.7        107876.5
    Mean                103210.2        107518.5   Diff    4.08%
flexbox-column-wrap     102530.4        105129.5   runs/s
flexbox-column-wrap     105323.5        105373.8
flexbox-column-wrap     107329.5        105420.4
flexbox-column-wrap     107347.2        105992.0
    Mean                105632.7        105478.9   Diff   -0.15%
flexbox-row-nowrap      138271.6        143728.1   runs/s
flexbox-row-nowrap      140718.1        145321.6
flexbox-row-nowrap      144811.8        146044.2
flexbox-row-nowrap      146147.0        148958.8
    Mean                142487.1        146013.1   Diff    2.47%
flexbox-row-wrap        103482.7        103598.8   runs/s       
flexbox-row-wrap        103778.7        105639.1                
flexbox-row-wrap        104554.4        106845.4
flexbox-row-wrap        105866.3        109833.9        
    Mean                104420.5        106479.3   Diff    1.97%
floats_100_100             200.0           198.0   ms 
floats_100_100             201.0           198.5
floats_100_100             201.5           199.0   
floats_100_100             202.5           199.5   
    Mean                   201.3           198.8   Diff    1.24%
floats_100_100_nested      215.5           213.5   ms
floats_100_100_nested      215.5           213.5
floats_100_100_nested      217.5           216.5   
floats_100_100_nested      218.5           216.5   
    Mean                   216.8           215.0   Diff    0.81%
floats_20_100              409.9           410.7   ms
floats_20_100              411.4           412.3
floats_20_100              413.0           414.1   
floats_20_100              416.3           414.7   
    Mean                   412.6           413.0   Diff   -0.08%
floats_20_100_nested       612.3           614.0   ms
floats_20_100_nested       618.3           615.3
floats_20_100_nested       635.2           616.3   
floats_20_100_nested       635.7           634.2   
    Mean                   625.4           620.0   Diff    0.87%
floats_2_100               204.6           205.4   ms
floats_2_100               205.7           205.9
floats_2_100               205.9           206.8   
floats_2_100               206.0           207.4   
    Mean                   205.6           206.4   Diff   -0.40%
floats_2_100_nested        463.6           457.2   ms
floats_2_100_nested        463.9           459.1
floats_2_100_nested        464.5           460.0   
floats_2_100_nested        466.7           460.1   
    Mean                   464.7           459.1   Diff    1.20%
floats_50_100              305.6           302.6   ms     
floats_50_100              312.4           309.6
floats_50_100              313.0           310.4   
floats_50_100              314.4           311.6   
    Mean                   311.4           308.6   Diff    0.90%
floats_50_100_nested       367.6           370.0   ms     
floats_50_100_nested       370.8           370.8
floats_50_100_nested       371.8           371.0   
floats_50_100_nested       375.6           371.0   
    Mean                   371.5           370.7   Diff    0.20%
line-layout                 79.6            83.2   runs/s
line-layout                 80.0            83.7 
line-layout                 80.1            83.9    
line-layout                 80.1            84.1    
    Mean                    80.0            83.7   Diff    4.70%

[Geoffrey Garen]

Comment on attachment 167142 [details]
Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

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

This patch looks good to me, modulo a few style comments. I'm going to hold off on saying r+, though, since you mentioned that Dan is also going to review this.

> Source/WTF/wtf/Platform.h:1030
> +#define ENABLE_CAN_USE_8BIT_TEXTRUN 1

Let's call this WTF_USE_8BIT_TEXTRUN.

> Source/WebCore/platform/graphics/WidthIterator.cpp:84
> -    return m_font->glyphDataForCharacter(character, mirror);
> +    return m_font->glyphDataAndPageForCharacter(character, mirror).first;

Since this was a speedup for your patch, let's inline glyphDataForCharacter and move it to the .h file.

> Source/WebCore/xml/parser/MarkupTokenBase.h:197
> +#endif

Since we know that all callers have LChar data in their vectors, let's change the declaration here to take const Vector<LChar, 32>& so we can verify the character type is correct at compile time.

[Eric Seidel (no email)]

CCing other chromiumers who have recently worked with RenderText and might be interested to CC this go by.

Comment on attachment 167142 [details]
Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

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

> Source/WebCore/ChangeLog:11
> +        now written as rt->getChar(n).

I would have called this characterAt()

> Source/WebCore/ChangeLog:18
> +        for PLATFORM(MAC).  Other platform can update this setting in Platform.h when their platform specific use of

s/use/uses/

> Source/WebCore/rendering/InlineTextBox.cpp:862
> +    if (string.length() != static_cast<unsigned>(length) || m_start)
> +        string = string.substringSharingImpl(m_start, length);

Is there a reason why you added ASSERT(static_cast<unsigned>(m_start + length) <= string.length()) above in paint() but not here in paintSelection()?

> Source/WebCore/rendering/InlineTextBox.h:110
> +    TextRun constructTextRun(RenderStyle*, const Font&, String, int, BufferForAppendingHyphen* = 0) const;

I think you should have left the parameter name maximumLength here, since it’s not implied by the type.

> Source/WebCore/rendering/RenderBlock.cpp:7555
> +{
> +    ASSERT(style);
> +
> +    TextDirection textDirection = LTR;
> +    bool directionalOverride = style->rtlOrdering() == VisualOrder;
> +
> +    TextRun run(characters, length, 0, 0, expansion, textDirection, directionalOverride);
> +    if (textRunNeedsRenderingContext(font))
> +        run.setRenderingContext(SVGTextRunRenderingContext::create(context));
> +
> +    return run;
> +}

Couldn’t this have been written simply as a call to the version that takes TextRunFlags, passing the DefaultTextRunFlags?

> Source/WebCore/rendering/RenderBlock.cpp:7619
> +    return constructTextRunInternal(context, font, string.characters(), length, style, expansion, flags);

Why not characters16()?

> Source/WebCore/rendering/RenderBlock.h:272
> +    static TextRun constructTextRun(RenderObject* context, const Font&, const RenderText*, unsigned, unsigned, RenderStyle*,

You should name the unsigned parameters here.

> Source/WebCore/rendering/RenderCombineText.cpp:74
> +void RenderCombineText::stringToRender(int start, String& string, int& length) const

We normally use “get” in the name of functions that use out parameters like this. How about getStringToRender()?

> Source/WebCore/rendering/RenderCombineText.h:35
> +    void stringToRender(int, String&, int&) const;

I’d probably keep the parameter name here.

I also agree with Geoff’s suggestions. The patch is OK, but between the two of use, I think there are enough comments to warrant another pass. The thing that bothers me the most is getChar(). I really prefer characterAt().

[Michael Saboff]

Created attachment 168117 [details]
Patch with updates from reviewers

Made the changes suggested by Geoff.

(In reply to comment #12)
> (From update of attachment 167142 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167142&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        now written as rt->getChar(n).
> 
> I would have called this characterAt()

Done!

> > Source/WebCore/ChangeLog:18
> > +        for PLATFORM(MAC).  Other platform can update this setting in Platform.h when their platform specific use of
> 
> s/use/uses/

Changed the sentence to: Other platform can update this setting in Platform.h when their platform specific code is updated to TextRun's with 8 bit data.

> > Source/WebCore/rendering/InlineTextBox.cpp:862
> > +    if (string.length() != static_cast<unsigned>(length) || m_start)
> > +        string = string.substringSharingImpl(m_start, length);
> 
> Is there a reason why you added ASSERT(static_cast<unsigned>(m_start + length) <= string.length()) above in paint() but not here in paintSelection()?

Added the second ASSERT.

> > Source/WebCore/rendering/InlineTextBox.h:110
> > +    TextRun constructTextRun(RenderStyle*, const Font&, String, int, BufferForAppendingHyphen* = 0) const;
> 
> I think you should have left the parameter name maximumLength here, since it’s not implied by the type.

Added back parameter names.

> > Source/WebCore/rendering/RenderBlock.cpp:7555
> > +{
> > +    ASSERT(style);
> > +
> > +    TextDirection textDirection = LTR;
> > +    bool directionalOverride = style->rtlOrdering() == VisualOrder;
> > +
> > +    TextRun run(characters, length, 0, 0, expansion, textDirection, directionalOverride);
> > +    if (textRunNeedsRenderingContext(font))
> > +        run.setRenderingContext(SVGTextRunRenderingContext::create(context));
> > +
> > +    return run;
> > +}
> 
> Couldn’t this have been written simply as a call to the version that takes TextRunFlags, passing the DefaultTextRunFlags?

Passing the extra parameter cost performance.

> > Source/WebCore/rendering/RenderBlock.cpp:7619
> > +    return constructTextRunInternal(context, font, string.characters(), length, style, expansion, flags);
> 
> Why not characters16()?

Because characters() will up convert an 8 bit string if necessary.  This allows an 8 bit string to create a 16 bit text run.

> > Source/WebCore/rendering/RenderBlock.h:272
> > +    static TextRun constructTextRun(RenderObject* context, const Font&, const RenderText*, unsigned, unsigned, RenderStyle*,
> 
> You should name the unsigned parameters here.

Done.

> > Source/WebCore/rendering/RenderCombineText.cpp:74
> > +void RenderCombineText::stringToRender(int start, String& string, int& length) const
> 
> We normally use “get” in the name of functions that use out parameters like this. How about getStringToRender()?

Done.

> > Source/WebCore/rendering/RenderCombineText.h:35
> > +    void stringToRender(int, String&, int&) const;
> 
> I’d probably keep the parameter name here.

Done.

Note that this patch will fail the style-bot due to the parameter names in the .h files.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Pl..." exit_code: 1
Source/WebCore/rendering/RenderCombineText.h:35:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:268:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:268:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:268:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:271:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:271:  The parameter name "text" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:271:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:274:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:274:  The parameter name "text" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:274:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:277:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:277:  The parameter name "text" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:277:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:281:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:281:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:285:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderBlock.h:285:  The parameter name "style" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 17 in 23 files


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

Comment on attachment 167142 [details]
Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

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

>>> Source/WebCore/rendering/RenderBlock.cpp:7619
>>> +    return constructTextRunInternal(context, font, string.characters(), length, style, expansion, flags);
>> 
>> Why not characters16()?
> 
> Because characters() will up convert an 8 bit string if necessary.  This allows an 8 bit string to create a 16 bit text run.

Hmm… but here we know that the string is either empty or 16-bit. Maybe we should handle with the empty case separately so that we can rely on the knowledge that the string is 16-bit otherwise?

[Michael Saboff]

Comment on attachment 167142 [details]
Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

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

>>>> Source/WebCore/rendering/RenderBlock.cpp:7619
>>>> +    return constructTextRunInternal(context, font, string.characters(), length, style, expansion, flags);
>>> 
>>> Why not characters16()?
>> 
>> Because characters() will up convert an 8 bit string if necessary.  This allows an 8 bit string to create a 16 bit text run.
> 
> Hmm… but here we know that the string is either empty or 16-bit. Maybe we should handle with the empty case separately so that we can rely on the knowledge that the string is 16-bit otherwise?

My last comment was in error.  It is characters() because of the empty case.  characters() handles both the empty case and the 16 bit case.

[Michael Saboff]

Committed r131311: <http://trac.webkit.org/changeset/131311>