[webkit-reviews] review denied: [Bug 35233] Optimize Latin-1 decoding in TextCodecLatin1::decode() : [Attachment 49217] Same patch with nits addressed

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 35233: Optimize Latin-1 decoding in TextCodecLatin1::decode()
https://bugs.webkit.org/show_bug.cgi?id=35233

Attachment 49217: Same patch with nits addressed
https://bugs.webkit.org/attachment.cgi?id=49217&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
We capitalize acronyms in WebKit code, so the constant should be called
nonASCIIMask rather than nonAsciiMask.

> +#if CPU(X86_64) || CPU(SPARC64) || CPU(IA64) || CPU(PPC64)
> +const uintptr_t nonAsciiMask = 0x8080808080808080;
> +#else
> +const uintptr_t nonAsciiMask = 0x80808080;
> +#endif

This can be done without ifdefs as is done in the WTF header file
HashFunctions.h.

    template<size_t size> struct NonASCIIMask;
    template<> struct NonASCIIMask<4> { static unsigned value() { return
0x80808080U; } }
    template<> struct NonASCIIMask<8> { static unsigned long long value() {
return 0x8080808080808080UL; }

Then say:

    if (chunk & NonASCIIMask<sizeof(uintptr_t)>::value())

While this is a bit harder to read, it's safer. The code in the patch as is
would fail in a subtle way if you had a 64-bit platform not listed in the
ifdefs, only checking half of the bits.

The copying can be done the same way:

> +		       dest[0] = src[0];
> +		       dest[1] = src[1];
> +		       dest[2] = src[2];
> +		       dest[3] = src[3];
> +#if CPU(X86_64) || CPU(SPARC64) || CPU(IA64) || CPU(PPC64)
> +		       dest[4] = src[4];
> +		       dest[5] = src[5];
> +		       dest[6] = src[6];
> +		       dest[7] = src[7];
> +#endif

This copying can also be done with the template technique above, with an inline
template member function that copies the correct number of bytes.

Possibly better for platforms that allow unaligned access, this can be done
with a single line:

    *reinterpret_cast<uintptr_t*>(dest) = chunk;

You can see in StringHash.h that many platforms do allow unaligned access. I
predict it would be a speedup to use that when possible.