[webkit-reviews] review requested: [Bug 66161] Investigate storing strings in 8-bit buffers when possible : [Attachment 112623] Patch with updates suggested in comment #26, stylebot and build failures

[bugzilla-daemon at webkit.org]

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

Attachment 112623: Patch with updates suggested in comment #26, stylebot and
build failures
https://bugs.webkit.org/attachment.cgi?id=112623&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
> > 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.

It didn't inline, so I changed to (1) - size / buffer check, slow case the
rest.

> > 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.

The existing code doesn't inline this method.  The StringHash class is used in
StringImpl.h to create a template class therefore the complete method in the .h
file.

> > Source/JavaScriptCore/wtf/text/StringImpl.cpp:59
> > +	 if (has16BitShadow() && ((ownership == BufferInternal) || (ownership
== BufferOwned))) {
> 
> if has16BitShadow() is true, we should unconditionally fastFree.

Done.

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

Yes, done.

> > 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.

Updated StringImpl::lower(), ::upper() and ::foldCase() to work with Latin-1
strings with an ASCII fast path.

> > 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.

Removed.

Also cleaned up the sizeof(char) that should be sizeof(LChar) and other nits.