[webkit-reviews] review denied: [Bug 92475] Use 8 bit atomic strings where possible : [Attachment 154883] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Dan Carney
<dcarney at google.com>'s request for review:
Bug 92475: Use 8 bit atomic strings where possible
https://bugs.webkit.org/show_bug.cgi?id=92475

Attachment 154883: Patch
https://bugs.webkit.org/attachment.cgi?id=154883&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154883&action=review


Before moving forward with this, please get some numbers on the performance
gain:

How much CPU time does the change cost in the positive and negative cases?
How many times do we each each branch in real web pages?
How many bytes saved (don't forget it is possible we do 16bits conversion later
in which case your change causes extra memory, not less)?

> Source/WTF/wtf/text/ASCIIFastPath.h:117
> +inline bool charactersAreAll8Bit(const UChar* characters, size_t length)
> +{
> +    MachineWord allCharBits = orAllCharacters(characters, length);
> +    MachineWord non8BitMask = Non8BitMask<sizeof(MachineWord)>::value();
> +    return !(allCharBits & non8BitMask);
> +}

This is the wrong place for this function. The file is ASCIIFastPath, it is
about the ASCII functions.

> Source/WTF/wtf/text/AtomicString.cpp:117
> +static inline void constructMinimalStringImpl(StringImpl*& location, const
UChar * buffer, unsigned length)

const UChar * buffer -> coding style.

This function should be in StringImpl, not in AtomicString.

> Source/WTF/wtf/text/AtomicString.cpp:120
> +	   // optimize for 8 bit strings

Bad formatting for your comment, see
http://www.webkit.org/coding/coding-style.html.
This comment is pretty useless anyway, you could remove it.

> Source/WTF/wtf/text/AtomicString.cpp:122
> +	   location = StringImpl::createUninitialized(length, data8).leakRef();


This leakRef() makes the ownership difficult to follow.
Please return a PassRefPtr from here instead.

> Source/WTF/wtf/text/AtomicString.cpp:125
> +	       data8[i] = (LChar) data16[i];

Please use an explicit C++ cast, not a C cast.

> Source/WTF/wtf/text/AtomicString.cpp:218
> +	       // optimize for ascii

The style is wrong (see above). You can also remove this comment.

> Source/WTF/wtf/text/AtomicString.cpp:219
> +	       location = StringImpl::create((const LChar*) buffer.characters,
buffer.length).leakRef();

Cast (see above).