StringProtoFuncToUpperCase has a local implementation similar to what exists in StringImpl::upper(). All of the work should be done in StringImpl, including optimizations for 8 bit strings.
Created attachment 123170 [details] Patch Neutral on sunspider and v8. Provides minor benefit to Dromaeo JavaScript tests.
Comment on attachment 123170 [details] Patch r=me
Committed r106167: <http://trac.webkit.org/changeset/106167>
Rolled out in http://trac.webkit.org/changeset/106170 due to a test failure in LayoutTests/fast/js/string-capitalization.html. Need to resolve that issue and submit fixed patch.
Created attachment 124834 [details] Updated patch with fix for s-sharp This patch is performance neutral on sunspider, v8 and kraken. It speeds up the dromaeo-object-string toUpperCase subtest from 238runs/sec to 347runs/sec (46%). Unfortunately, this provides little benefit to the dromaeo javascript overall.
Comment on attachment 124834 [details] Updated patch with fix for s-sharp View in context: https://bugs.webkit.org/attachment.cgi?id=124834&action=review > Source/JavaScriptCore/ChangeLog:3 > +2012-01-31 Michael Saboff <msaboff@apple.com> > + > +2012-01-31 Michael Saboff <msaboff@apple.com> Double name here. > Source/JavaScriptCore/ChangeLog:18 > + (JSC::stringProtoFuncToLowerCase): Since all you did was tweak whitespace in this function, I suggest removing this line. > Source/JavaScriptCore/runtime/StringPrototype.cpp:1206 > + StringImpl* ourImpl = s.impl(); I would have called this sImpl for consistency with sSize above. > Source/JavaScriptCore/runtime/StringPrototype.cpp:1208 > + if (ourImpl == upper.get()) The call to .get() here should not be needed. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:366 > + const UChar* source16; This source16 variable declared so long before it’s used is only necessary because we don’t want to call characters() in the fast 16-bit case. But I think one more check of is8Bit() is probably cheap enough so we can just call characters() and avoid it. Either way is probably OK. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:388 > + // There are three special case characters. > + // 1. sharp-S (0xdf) converts to "SS" (two characters) > + // 2. micro (0xb5) converts to upper case micro (0x39c, a 16 bit character) > + // 3. y umlaut (0xff) converts to upper case Y umlaut (0x178, a 16 bit character) The code doesn’t assume that there are exactly three of these, so I don’t think we need to list them. The code simply assumes that there is exactly one character that converts to two, and that is U+00DF. I think a better comment would focus on the fact that latinSmallLetterSharpS is the only character in the range U+0000-U+00FF that converts into more than one character, because that’s all the code assumes. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:391 > + if (UNLIKELY(c == 0xdf)) Normally we would use a constant named latinSmallLetterSharpS in the header CharacterNames.h rather than coding 0xdf here in line. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:395 > + // Have a character that is 16bit when converted to uppercase. I think you could word this comment better. I would write this: // Since this upper-cased character does not fit in an 8-bit string, we should convert to 16-bit. > Source/JavaScriptCore/wtf/text/StringImpl.cpp:412 > + if (UNLIKELY(c == 0xdf)) { I’m not sure this UNLIKELY is all that helpful.
Committed r106417: <http://trac.webkit.org/changeset/106417>