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.
Created attachment 115042 [details] Patch
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?
(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.
Committed r100202: <http://trac.webkit.org/changeset/100202>