[webkit-reviews] review denied: [Bug 79880] [WebSocket]Optimize the masking/unmasking operation by 64-bit data type : [Attachment 130317] Patch

[bugzilla-daemon at webkit.org]

Zoltan Herczeg <zherczeg at webkit.org> has denied joey <li.yin at intel.com>'s
request for review:
Bug 79880: [WebSocket]Optimize the masking/unmasking operation by 64-bit data
type
https://bugs.webkit.org/show_bug.cgi?id=79880

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

------- Additional Comments from Zoltan Herczeg <zherczeg at webkit.org>
In general this looks well, and probably works well no other machines like ARM
with NEON.

What I dont like is &base[offset] forms, please use base + offset instead of
them.

View in context: https://bugs.webkit.org/attachment.cgi?id=130317&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

Remove this line. And please describe why is a win here as well. And add write
some perf results.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:113
> +static void maskFrameDataByUint64(char *src, size_t dataLength, const char
*mask)

Please don't abbreaviate. Use source instead of src. Or buffer since it is also
a destination...
And add an "inline" keyword. Otherwise this is probably not a win.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:119
> +    if (dataLength > 0) {

WebKit prefers early return. So use:

if (datalength == 0)
    return;

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:123
> +	       maskAlignedArray[i] = mask[index % maskingKeyWidthInBytes];

Is maskingKeyWidthInBytes is <= 8 and power of 2 all the time? We should add a
COMPILE_ASSERT for it!

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:133
> +	   for (size_t i = 0; i < dataLength % 8; i++)
> +		pSrc[i] ^= maskAlignedArray[i];

Just thinking about these fors...

dataLength &= ~0x7;
while (dataLength-- > 0)
    *pSrc++ ^= *maskAlignedArray++;

I think compiler like this better.