RESOLVED FIXED 96590
Added 8 bit path to WidthIterator::advance()
https://bugs.webkit.org/show_bug.cgi?id=96590
Summary Added 8 bit path to WidthIterator::advance()
Michael Saboff
Reported 2012-09-12 18:51:39 PDT
This is part of adding 8 bit code path to text rendering.
Attachments
Patch (20.61 KB, patch)
2012-09-12 19:11 PDT, Michael Saboff
no flags
Patch with updates (21.73 KB, patch)
2012-09-13 09:54 PDT, Michael Saboff
no flags
Updated Patch with development #if (21.67 KB, patch)
2012-09-13 09:58 PDT, Michael Saboff
ggaren: review+
Michael Saboff
Comment 1 2012-09-12 19:11:07 PDT
Michael Saboff
Comment 2 2012-09-13 09:54:19 PDT
Created attachment 163893 [details] Patch with updates Updated the original patch with two changes. Changed Source/WebCore/platform/graphics/Latin1TextIterator.h to be an svn copy of Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h to retain history. Ran performance tests of original patch and it showed a regression in the Layout test. Changed SurrogatePairAwareTextIterator to have inline versions of advance and consume with a new consumeSlowCase for the possible surrogate pair cases. Performance result of this patch are: Baseline With patch Diff flexbox-column-nowrap 103708.6 109858.0 flexbox-column-nowrap 109955.7 106864.1 flexbox-column-nowrap 108447.6 106678.0 flexbox-column-nowrap 106098.8 107921.9 Mean 107052.7 107830.5 -0.81% flexbox-column-wrap 109909.5 106872.5 flexbox-column-wrap 108883.3 105228.3 flexbox-column-wrap 106647.2 106476.6 flexbox-column-wrap 105324.4 107460.5 Mean 107691.1 106509.5 1.10% flexbox-row-nowrap 146211.5 142378.8 flexbox-row-nowrap 147438.1 143749.6 flexbox-row-nowrap 143789.2 144680.3 flexbox-row-nowrap 148418.2 145950.7 Mean 146464.3 144189.8 -1.55% flexbox-row-wrap 104452.9 109111.9 flexbox-row-wrap 109038.1 109778.9 flexbox-row-wrap 107117.2 109969.8 flexbox-row-wrap 107728.8 107058.3 Mean 107084.3 108979.7 1.77% floats_100_100_nested 222.5 217.5 floats_100_100_nested 224.0 215.5 floats_100_100_nested 224.5 214.5 floats_100_100_nested 224.0 216.5 Mean 223.8 216.0 3.46% floats_100_100 205.0 198.5 floats_100_100 207.0 201.0 floats_100_100 205.5 199.5 floats_100_100 206.0 200.5 Mean 205.9 199.9 2.91% floats_2_100_nested 463.3 463.2 floats_2_100_nested 462.7 464.3 floats_2_100_nested 464.7 463.7 floats_2_100_nested 467.0 462.3 Mean 464.4 463.4 0.23% floats_2_100 203.0 203.2 floats_2_100 203.8 207.6 floats_2_100 205.9 207.5 floats_2_100 203.3 203.7 Mean 204.0 205.5 -0.74% floats_20_100_nested 623.5 613.7 floats_20_100_nested 621.5 617.2 floats_20_100_nested 629.8 615.8 floats_20_100_nested 627.3 615.2 Mean 625.5 615.5 1.61% floats_20_100 426.3 424.0 floats_20_100 422.6 420.3 floats_20_100 423.6 420.3 floats_20_100 426.9 420.1 Mean 424.8 421.2 0.86% floats_50_100_nested 380.0 372.8 floats_50_100_nested 378.0 368.0 floats_50_100_nested 378.0 370.6 floats_50_100_nested 377.4 371.4 Mean 378.4 370.7 2.02% floats_50_100 307.2 300.4 floats_50_100 304.4 300.0 floats_50_100 307.2 299.0 floats_50_100 304.4 299.8 Mean 305.8 299.8 1.96% line-layout 75.5 78.2 line-layout 75.7 78.5 line-layout 75.4 78.2 line-layout 75.4 78.2 Mean 75.5 78.3 3.68%
Michael Saboff
Comment 3 2012-09-13 09:58:34 PDT
Created attachment 163894 [details] Updated Patch with development #if Same comments as prior patch.
Geoffrey Garen
Comment 4 2012-09-13 10:08:25 PDT
Comment on attachment 163894 [details] Updated Patch with development #if View in context: https://bugs.webkit.org/attachment.cgi?id=163894&action=review > Source/WebCore/platform/graphics/Latin1TextIterator.h:2 > + * Copyright (C) Research In Motion Limited 2011. All rights reserved. Why the RIM copyright, and no Apple copyright? If this is new code, please use the license @ http://www.webkit.org/coding/bsd-license.html. > Source/WebCore/platform/graphics/Latin1TextIterator.h:30 > + // The passed in UChar pointer starts at 'currentCharacter'. The iterator operatoes on the range [currentCharacter, lastCharacter]. Typo: operatoes. > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:42 > + if (character < 0x3041) This should be a named constant. > Source/WebCore/platform/graphics/WidthIterator.cpp:257 > + if (int(m_currentCharacter) >= offset) No need to cast: m_currentCharacter is an int.
Michael Saboff
Comment 5 2012-09-13 10:43:11 PDT
(In reply to comment #4) > (From update of attachment 163894 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163894&action=review > > > Source/WebCore/platform/graphics/Latin1TextIterator.h:2 > > + * Copyright (C) Research In Motion Limited 2011. All rights reserved. > > Why the RIM copyright, and no Apple copyright? > > If this is new code, please use the license @ http://www.webkit.org/coding/bsd-license.html. The Apple copyright is above the RIM copyright. This file is largely leveraged from SurrogatePairAwareTextIterator.h and therefore I kept the copyright from that file and added the Apple copyright after consulting. Consulted with Dan on this. > > Source/WebCore/platform/graphics/Latin1TextIterator.h:30 > > + // The passed in UChar pointer starts at 'currentCharacter'. The iterator operatoes on the range [currentCharacter, lastCharacter]. > > Typo: operatoes. Fixed. > > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.h:42 > > + if (character < 0x3041) > > This should be a named constant. Added constant to WTF/wtf/unicode/CharacterNames.h. > > Source/WebCore/platform/graphics/WidthIterator.cpp:257 > > + if (int(m_currentCharacter) >= offset) > > No need to cast: m_currentCharacter is an int. m_currentCharacter is an unsigned (Source/WebCore/platform/graphics/WidthIterator.h line 62). I changed this to if (m_currentCharacter >= static_cast<unsigned>(offset)) since we really want to change offset to unsigned.
Michael Saboff
Comment 6 2012-09-13 14:03:33 PDT
Note You need to log in before you can comment on or make changes to this bug.