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.
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%
Comment on attachment 172720 [details] Patch Clearing flags on attachment: 172720 Committed r133726: <http://trac.webkit.org/changeset/133726>
All reviewed patches have been landed. Closing bug.
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.
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?
(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.
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
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. :)