[webkit-reviews] review denied: [Bug 66161] Investigate storing strings in 8-bit buffers when possible : [Attachment 112601] Updated patch addressing comments #21

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 66161: Investigate storing strings in 8-bit buffers when possible
https://bugs.webkit.org/show_bug.cgi?id=66161

Attachment 112601: Updated patch addressing comments #21
https://bugs.webkit.org/attachment.cgi?id=112601&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=112601&action=review


The fastFree and ASCII vs Latin1 issues look like real bugs. The rest would
just improve readability and performance.

I have a feeling that, in the end, we'll want all the inline fast cases to
cover the 8bit case, moving the 16bit cases to out-of-line slow cases. (Right
now, everything is in the same case, which probably defeats inlining. And we
can't inline the 8bit case yet, since most strings are still 16bit.)

> Source/JavaScriptCore/runtime/UString.h:140
> +NEVER_INLINE bool equalSlowCase(const UString& s1, const UString& s2);
> +
>  ALWAYS_INLINE bool operator==(const UString& s1, const UString& s2)

Still worried this is too big to inline.

If it doesn't inline, perhaps you could: (1) inline just the size and buffer
checks, and out-of-line everything else; (2) Do (1), and always just call
memcmp with the right length for the slow case.

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:129
> +	       unsigned char c = m_buffer[i];
> +	       destination[i] = c;

This can just be destination[i] = m_buffer[i].

> Source/JavaScriptCore/wtf/text/StringConcatenate.h:205
> +	       unsigned char c = m_buffer[i];
> +	       destination[i] = c;

Ditto.

> Source/JavaScriptCore/wtf/text/StringHash.h:56
> +	       if (a->is8Bit()) {

As above, I'm concerned that this can't inline anymore, and might need
rejiggering to inline properly.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:59
> +    if (has16BitShadow() && ((ownership == BufferInternal) || (ownership ==
BufferOwned))) {

if has16BitShadow() is true, we should unconditionally fastFree.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:184
> +    m_copyData16 = static_cast<UChar*>(fastCalloc(len, sizeof(UChar)));

Can this be fastMalloc instead?

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:407
> +    if (is8Bit()) {
> +	   LChar* data;
> +	   RefPtr <StringImpl>newImpl = createUninitialized(m_length, data);
> +	   for (int32_t i = 0; i < length; i++)
> +	       data[i] = toASCIILower(m_data8[i]);
> +	   return newImpl.release();
> +    }

What happens here for a Latin1 string, when you call toASCIILower on its
characters? I think you need to check each character for being ASCII.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:748
> +    size_t matchStringLength = strlen(reinterpret_cast<const char
*>(matchString));

No space before "*", please.

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:1017
> +	   memcpy(data, m_data8, position * sizeof(char));
> +	   if (str)
> +	       memcpy(data + position, str->m_data8, lengthToInsert *
sizeof(char));
> +	   memcpy(data + position + lengthToInsert, m_data8 + position +
lengthToReplace,
> +		  (length() - position - lengthToReplace) * sizeof(char));

Minor nit: should be sizeof(LChar) rather than sizeof(char) in these 3 places
-- even though the math works out the same.

> Source/JavaScriptCore/wtf/text/StringImpl.h:252
> +    // FIXME: Add Vector<char> adopt?

This is probably more appropriate as a bug report, with an explanation of why
you were considering that.

> Source/JavaScriptCore/wtf/text/StringImpl.h:272
> +    ALWAYS_INLINE const UChar* characters() const

Eventually, we'll want to rename characters() to "deprecatedCharacters()" to
communicate this. (Not right now, though.)