Optimise MaskWebSocketFramePayload().

Optimise MaskWebSocketFramePayload().

Add benchmarks for MaskWebSocketFramePayload(). Add a test to ensure
that it gives the correct result regardless of how the input is aligned or
chunked.

Optimise MaskWebSocketFramePayload() to apply the mask one word at a time
instead of one byte at a time. This reduces the time to mask 64k from 0.073ms
to 0.008ms on 32bit, and 0.079ms to 0.005ms on 64bit.

Input shorter than two words in length is still masked one byte at a time,
so there is no significant change in performance for small payloads.

TEST=net_unittests --gtest_filter='WebSocketFrameTestMaskBenchmark.*' --websocket-mask-iterations=10000

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173448

[Takashi Toyoshima, 2012-12-14 07:03:56]

Cool.
Thank you for performance improvement.

I think the TEST line of change description should not contain out/Release
prefix for net_unittests.

[Takashi Toyoshima, 2012-12-14 07:04:11]

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
File net/websockets/websocket_frame.cc (right):

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame.cc:157: char* const data,
Sorry, I'm not familiar with this 'const' usage.

Pointer argument is passed as a value. So basically, original pointer of the
caller is protected by default. Is this 'const' really meaningful? And is there
any reason to keep the header definition for this function without this const?

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame.cc:221: *reinterpret_cast<size_t*>(merged) ^=
packed_mask_key;
You should apply cast<PackedMaskType> here.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
File net/websockets/websocket_frame_unittest.cc (right):

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:24: static const int kLongPayloadSize
= 1<<16;
[optional] I feel that usually we place spaces before and after '<<' operator as
do against other operators.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:392: printf("Benchmarking
MaskWebSocketFramePayload() for %d iterations\n",
Usually we use LOG or DLOG for this kind of messages instead of direct output to
stdout/stderr.
DLOG affects only in debug build.
See, base/logging.h

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:403: printf("Payload size %d took
%.03f microseconds per iteration\n",
ditto.

[Adam Rice, 2012-12-14 08:05:37]

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
File net/websockets/websocket_frame.cc (right):

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame.cc:157: char* const data,
Resolved offline.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame.cc:221: *reinterpret_cast<size_t*>(merged) ^=
packed_mask_key;
On 2012/12/14 07:04:11, Takashi Toyoshima (chromium) wrote:
> You should apply cast<PackedMaskType> here.

Done.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
File net/websockets/websocket_frame_unittest.cc (right):

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:24: static const int kLongPayloadSize
= 1<<16;
On 2012/12/14 07:04:11, Takashi Toyoshima (chromium) wrote:
> [optional] I feel that usually we place spaces before and after '<<' operator
as
> do against other operators.

Done.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:392: printf("Benchmarking
MaskWebSocketFramePayload() for %d iterations\n",
On 2012/12/14 07:04:11, Takashi Toyoshima (chromium) wrote:
> Usually we use LOG or DLOG for this kind of messages instead of direct output
to
> stdout/stderr.
> DLOG affects only in debug build.
> See, base/logging.h

I chose LOG() since the benchmark is only useful when built in Release mode.

https://chromiumcodereview.appspot.com/11572010/diff/1/net/websockets/websock...
net/websockets/websocket_frame_unittest.cc:403: printf("Payload size %d took
%.03f microseconds per iteration\n",
On 2012/12/14 07:04:11, Takashi Toyoshima (chromium) wrote:
> ditto.

Done.

[Takashi Toyoshima, 2012-12-14 08:15:37]

Thanks!
LGTM.

[commit-bot: I haz the power, 2012-12-14 08:47:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/11572010/2002

[commit-bot: I haz the power, 2012-12-14 09:13:54]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Adam Rice, 2012-12-14 09:25:29]

Compile fix for Windows. PTAL.

[Takashi Toyoshima, 2012-12-14 09:37:55]

still lgtm.
I post trybot job for win_rel.
After you can see passing this build, check Commit button again.

[commit-bot: I haz the power, 2012-12-14 11:38:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/11572010/13001

[commit-bot: I haz the power, 2012-12-14 15:39:02]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-15 01:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/11572010/13001

[commit-bot: I haz the power, 2012-12-15 07:56:06]

Retried try job too often on win_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-17 10:55:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ricea@chromium.org/11572010/13001

[commit-bot: I haz the power, 2012-12-17 12:00:59]

Change committed as 173448