Bug 76647 – StringProtoFuncToUpperCase should call StringImpl::upper similar to StringProtoToLowerCase

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 123170 [details]
Patch

Neutral on sunspider and v8. Provides minor benefit to Dromaeo JavaScript tests.

[Geoffrey Garen]

Comment on attachment 123170 [details]
Patch

r=me

[Michael Saboff]

Committed r106167: <http://trac.webkit.org/changeset/106167>

[Michael Saboff]

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.

[Michael Saboff]

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.

[Darin Adler]

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
> [phone]  Michael Saboff  <[e-mail]>
> +
> [phone]  Michael Saboff  <[e-mail]>

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.

[Michael Saboff]

Committed r106417: <http://trac.webkit.org/changeset/106417>