This change is part of adding 8 bit string handling to the rendering code.
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.
Comment on attachment 166782 [details] Patch Attachment 166782 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14114884
Comment on attachment 166782 [details] Patch Attachment 166782 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14135403
Comment on attachment 166782 [details] Patch Attachment 166782 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14137372
Created attachment 166785 [details] Patch with PLATFORM(MAC) ifdef's removed This should fix the build errors for other platforms.
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
Created attachment 166967 [details] sample test run from chromium-linux@r130322 + patch here you go ...
Created attachment 167004 [details] results from a debug build results from a debug build ... lots of crashes.
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%
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.
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().
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.
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?
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.
Committed r131311: <http://trac.webkit.org/changeset/131311>