Bug 72317 – Towards 8 bit strings - Add 8 bit handling to JSString Ropes

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

[Michael Saboff]

The current JSString rope implementation only handles 16 bit strings.  It will up convert 8 bit string to 16 bit when the rope is resolved.

[Michael Saboff]

Created attachment 115042 [details]
Patch

[Darin Adler]

Comment on attachment 115042 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSString.cpp:118
> -// Overview: this methods converts a JSString from holding a string in rope form
> +// Overview: These methods converts a JSString from holding a string in rope form

Should say “These functions convert” rather than “These methods converts”.

> Source/JavaScriptCore/runtime/JSString.h:130
> +            m_is8Bit = m_value.impl()->is8Bit() ? 1 : 0;

Should not need the ? 1 : 0 here.

> Source/JavaScriptCore/runtime/JSString.h:138
> +            m_is8Bit = m_value.impl()->is8Bit() ? 1 : 0;

Should not need the ? 1 : 0 here.

> Source/JavaScriptCore/runtime/JSString.h:146
> +            m_is8Bit = (s1->is8Bit() && s2->is8Bit()) ? 1 : 0;

Should not need the ? 1 : 0 here.

> Source/JavaScriptCore/runtime/JSString.h:155
> +            m_is8Bit = (s1->is8Bit() && s2->is8Bit() &&  s3->is8Bit()) ? 1 : 0;

Should not need the ? 1 : 0 here.

> Source/JavaScriptCore/runtime/JSString.h:252
> -        void resolveRopeSlowCase(ExecState*, UChar*) const;
> +        void resolveRopeSlowCase8(ExecState*, LChar*) const;
> +        void resolveRopeSlowCase16(ExecState*, UChar*) const;

I suggest we just use take advantage of overloading here rather than putting 8/16 in the function names.

> Source/JavaScriptCore/runtime/JSString.h:264
> +        unsigned m_is8Bit : 1;

This should be bool rather than unsigned. We can’t use signed types for bit fields because of compiler differences, but we can and should use bool.

> Source/JavaScriptCore/runtime/JSString.h:270
> +        ALWAYS_INLINE bool is8Bit() const { return m_is8Bit; }

I don’t think we really need ALWAYS_INLINE here. Did you find this wasn’t getting inlined?

> Source/JavaScriptCore/runtime/JSString.h:377
> +        ASSERT(offset <= static_cast<unsigned>(s.length()));
> +        ASSERT(length <= static_cast<unsigned>(s.length()));
> +        ASSERT(offset + length <= static_cast<unsigned>(s.length()));
> +        if (!length)
> +            return globalData->smallStrings.emptyString(globalData);
> +        if (length == 1) {
> +            UChar c = s[offset];
> +            if (c <= maxSingleCharacterString)
> +                return globalData->smallStrings.singleCharacterString(globalData, c);
> +        }

None of this code is 8/16 bit specific. Is there some easy way to share this part instead of having it copied and pasted twice?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 115042 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115042&action=review
> 
> > Source/JavaScriptCore/runtime/JSString.cpp:118
> > -// Overview: this methods converts a JSString from holding a string in rope form
> > +// Overview: These methods converts a JSString from holding a string in rope form
> 
> Should say “These functions convert” rather than “These methods converts”.
> 
> > Source/JavaScriptCore/runtime/JSString.h:130
> > +            m_is8Bit = m_value.impl()->is8Bit() ? 1 : 0;
> 
> Should not need the ? 1 : 0 here.
> 
> > Source/JavaScriptCore/runtime/JSString.h:138
> > +            m_is8Bit = m_value.impl()->is8Bit() ? 1 : 0;
> 
> Should not need the ? 1 : 0 here.
> 
> > Source/JavaScriptCore/runtime/JSString.h:146
> > +            m_is8Bit = (s1->is8Bit() && s2->is8Bit()) ? 1 : 0;
> 
> Should not need the ? 1 : 0 here.
> 
> > Source/JavaScriptCore/runtime/JSString.h:155
> > +            m_is8Bit = (s1->is8Bit() && s2->is8Bit() &&  s3->is8Bit()) ? 1 : 0;
> 
> Should not need the ? 1 : 0 here.
> 
> > Source/JavaScriptCore/runtime/JSString.h:252
> > -        void resolveRopeSlowCase(ExecState*, UChar*) const;
> > +        void resolveRopeSlowCase8(ExecState*, LChar*) const;
> > +        void resolveRopeSlowCase16(ExecState*, UChar*) const;
> 
> I suggest we just use take advantage of overloading here rather than putting 8/16 in the function names.
> 
> > Source/JavaScriptCore/runtime/JSString.h:264
> > +        unsigned m_is8Bit : 1;
> 
> This should be bool rather than unsigned. We can’t use signed types for bit fields because of compiler differences, but we can and should use bool.
> 
> > Source/JavaScriptCore/runtime/JSString.h:270
> > +        ALWAYS_INLINE bool is8Bit() const { return m_is8Bit; }
> 
> I don’t think we really need ALWAYS_INLINE here. Did you find this wasn’t getting inlined?

Made changes as suggested to the above suggestions.

> > Source/JavaScriptCore/runtime/JSString.h:377
> > +        ASSERT(offset <= static_cast<unsigned>(s.length()));
> > +        ASSERT(length <= static_cast<unsigned>(s.length()));
> > +        ASSERT(offset + length <= static_cast<unsigned>(s.length()));
> > +        if (!length)
> > +            return globalData->smallStrings.emptyString(globalData);
> > +        if (length == 1) {
> > +            UChar c = s[offset];
> > +            if (c <= maxSingleCharacterString)
> > +                return globalData->smallStrings.singleCharacterString(globalData, c);
> > +        }
> 
> None of this code is 8/16 bit specific. Is there some easy way to share this part instead of having it copied and pasted twice?

Removed this change from this patch.  Will address it in an upcoming patch.

[Michael Saboff]

Committed r100202: <http://trac.webkit.org/changeset/100202>