[Webkit-unassigned] [Bug 92475] Use 8 bit atomic strings where possible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=92475


Benjamin Poulain <benjamin at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #154883|review?                     |review-
               Flag|                            |




--- Comment #4 from Benjamin Poulain <benjamin at webkit.org>  2012-08-02 14:47:18 PST ---
(From update of attachment 154883)
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).

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.