Summary: | StringBuilder::append(UChar) with an 8 bit quantity shouldn't change the contents to 16 bits | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||
Component: | JavaScriptCore | Assignee: | Michael Saboff <msaboff> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, ojan, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Michael Saboff
2012-11-06 19:12:56 PST
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. :) |