[webkit-reviews] review granted: [Bug 97237] [WebSocket] Receiving a large message is really slow : [Attachment 165008] Vector<char> instead of char array in WebSocketChannel

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted  review:
Bug 97237: [WebSocket] Receiving a large message is really slow
https://bugs.webkit.org/show_bug.cgi?id=97237

Attachment 165008: Vector<char> instead of char array in WebSocketChannel
https://bugs.webkit.org/attachment.cgi?id=165008&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=165008&action=review


Looks great to me. Marking r+ for now, will cq+ once EWS reports success.

In the future, please do mark patches r? and cq? as appropriate, so that they
are visible in review queue.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:394
> +	   LOG(Network, "WebSocket buffer overflow (%lu+%lu)",
static_cast<unsigned long>(m_buffer.size()), static_cast<unsigned long>(len));

Not related to this patch at all, but: this is a runtime condition, and should
be logged to developer console, not to a debug channel.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:405
> +    memmove(m_buffer.data(), m_buffer.data() + len, m_buffer.size() - len);
> +    m_buffer.resize(m_buffer.size() - len);

This looks suspicious. We are still doing lots of unnecessary copying.

You could have used Vector::remove() here - not that it would have improved
performance characteristics, but it would be one line instead of three.