Bug 101421 – StringBuilder::append(UChar) with an 8 bit quantity shouldn't change the contents to 16 bits

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

StringBuilder objects keep track of whether or not the contents are all 8 bit in m_is8Bit.  If appendChar(UChar) is called, m_is8Bit is set to false.

An improvement is to look at the argument to StringBuilder::append(UChar) when m_is8Bit is true and see if the value can fit in 8 bits.  If so, keep m_is8Bit true and append the character to the 8 bit buffer.

[Michael Saboff]

Created attachment 172720 [details]
Patch

This appears to be a progression on most of the Parser performance tests.  Here is a comparison of the arithmetic average of 8 test runs for the parser tests:

Test                 Baseline        With Patch   Change
html-parser         2026.46ms      1989.21ms      +2.8%
html5-full-render   4274.28ms      4176.83ms      +2.3%
innerHTML-setter     263.14runs/s   269.21runs/s  +2.3%
simple-url            48.47runs/s    51.14runs/s  +5.5%
textarea-parsing      82.73runs/s    83.00runs/s  +0.3%
tiny-inner-HTML        5.08runs/s     5.07runs/s  -0.2%
url-parser           111.32runs/r   117.61runs/s  +5.7%
xml-parser             7.07runs/s     7.20runs/s  +1.8%

[WebKit Review Bot]

Comment on attachment 172720 [details]
Patch

Clearing flags on attachment: 172720

Committed r133726: <http://trac.webkit.org/changeset/133726>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 172720 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.cpp:251
> +        if (length == 1 && !(*characters & ~0xff)) {
> +            // Append as 8 bit character
> +            LChar lChar = static_cast<LChar>(*characters);
> +            append(&lChar, 1);
> +            return;
> +        }

It seems a little strange to have the 1-character special case here instead of in the append(UChar) function.

[Darin Adler]

Comment on attachment 172720 [details]
Patch

Now that I have some time to think about it, I realize I would have done this differently. I would have left the StringBuilder::append(const UChar, unsigned) function alone. And I would have added this to the start of append(UChar c):

    if (!(c & ~0xff)) {
        append(static_cast<LChar>(c));
        return;
    }

I wonder whether that version of the patch would have better or worse performance than the one you landed?

[Michael Saboff]

(In reply to comment #5)
> (From update of attachment 172720 [details])
> Now that I have some time to think about it, I realize I would have done this differently. I would have left the StringBuilder::append(const UChar, unsigned) function alone. And I would have added this to the start of append(UChar c):
> 
>     if (!(c & ~0xff)) {
>         append(static_cast<LChar>(c));
>         return;
>     }
> 
> I wonder whether that version of the patch would have better or worse performance than the one you landed?

I'll try that out, measure performance and report.  I suspect that it will be pretty close to equal.  The reason I went the way I did was to reduce bloat due to a larger inlined function.  Therefore I'll also check object size differences as well.

[Ojan Vafai]

It's hard to tell with all the noise, but it also looks like this was a ~11% improvement in setting style.

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstyleprototype/report.html?rev=166663&graph=jslib_style_prototype_Prototype___setStyle__&trace=score&history=50

[Ojan Vafai]

Actually, it's an improvement on a bunch of the jquery benchmarks:

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166613&graph=jslib_style_jquery_jQuery___height___x10&trace=score&history=150

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166613&graph=jslib_style_jquery_jQuery___width___x10&trace=score&history=150

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166664&graph=jslib_style_jquery_jQuery___css_color__x100&trace=score&history=150

http://build.chromium.org/f/chromium/perf/mac-release-10.6-webkit-latest/dromaeo_jslibstylejquery/report.html?rev=166664&graph=jslib_style_jquery_jQuery____is__visible_&trace=score&history=150

Probably some others too, but I'll stop there. :)