RFC: Sending pre-encoded text over a web socket.

Add WebSocket.addUtf8Text to allow sending pre-encoded text without a round-trip UTF-8 conversion.

Use it to implement the vm-service, where in particular we are concerned about the space overhead of the conversion leading to the process being killed on iOS.

Closes #27129

R=johnmccutchan@google.com, lrn@google.com

Committed: https://github.com/dart-lang/sdk/commit/aa38062a239c7507fa9a902f53473b2cc6b150cb

[rmacnak, 2016-08-19 00:28:14]

rmacnak@google.com changed reviewers:
+ lrn@google.com, zra@google.com

[Cutch, 2016-08-19 13:43:31]

Alternatively, we could just provide an explicit API:

enum WebSocketMessageType {
  TEXT,
  BINARY
}

void sendMessage(WebSocketMessageType type, List<int> bytes);

We don't need to introduce a new type and we give the developer control to
workaround our abstraction.

[zra, 2016-08-19 15:41:38]

On 2016/08/19 13:43:31, Cutch wrote:
> Alternatively, we could just provide an explicit API:
> 
> enum WebSocketMessageType {
>   TEXT,
>   BINARY
> }
> 
> void sendMessage(WebSocketMessageType type, List<int> bytes);
> 
> We don't need to introduce a new type and we give the developer control to
> workaround our abstraction.

+1

[rmacnak, 2016-08-19 20:37:53]

Updated to the sendMessage variant.  There's no public path between the
_WebSocketImpl and the _WebSocketOutgoingTransform, so the implementation still
uses a wrapper type.

[siva, 2016-08-19 20:52:53]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-08-19 20:52:54]

Ryan can you post numbers indicating the actual memory footprint reduction you
see using the BINARY form.

[rmacnak, 2016-08-19 21:46:28]

On 2016/08/19 20:52:54, siva wrote:
> Ryan can you post numbers indicating the actual memory footprint reduction you
> see using the BINARY form.

dart2js hello,
peak RSS at initial Observatory visit -> peak RSS after getSourceReport(reports:
['_CallSites']), which has a 23MB response

before 165M -> 344M
before 158M -> 341M
before 161M -> 346M

after 157M -> 236M
after 161M -> 233M
after 172M -> 220M

[Cutch, 2016-08-23 23:13:28]

johnmccutchan@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-08-23 23:13:29]

lgtm

[Lasse Reichstein Nielsen, 2016-08-24 05:10:12]

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.dart
File sdk/lib/io/websocket.dart (right):

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.da...
sdk/lib/io/websocket.dart:419: void sendMessage(WebSocketMessageType type,
List<int> bytes);
For consistency, the name should start with "add".
Maybe "addRawMessage"?

Also, what is the difference between 
  add(uint8List)
and
  sendMessage(WebSocketMessageType.binary, uint8List)
?
If there isn't any, there is no need to duplicate the functionality.
Instead just have:
  /**
   * Sends a text message with the text represented by [bytes].
   *
   * The [bytes] should be valid UTF-8 encoded Unicode characters.
   */
  addUtf8String(List<int> bytes);

[Lasse Reichstein Nielsen, 2016-08-24 05:51:04]

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.dart
File sdk/lib/io/websocket.dart (right):

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.da...
sdk/lib/io/websocket.dart:419: void sendMessage(WebSocketMessageType type,
List<int> bytes);
Or maybe addUtf8Text - it seems that WebSockets talk about "text", not "string".

It should probably also be specified what happens if the encoding is incorrect. 

Is it parsed with invalid sequences replaced by the replacement character, or is
the message lost?

[Cutch, 2016-08-24 14:01:41]

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.dart
File sdk/lib/io/websocket.dart (right):

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.da...
sdk/lib/io/websocket.dart:419: void sendMessage(WebSocketMessageType type,
List<int> bytes);
On 2016/08/24 05:51:04, Lasse Reichstein Nielsen wrote:
> Or maybe addUtf8Text - it seems that WebSockets talk about "text", not
"string".
> 
> It should probably also be specified what happens if the encoding is
incorrect. 
> 
> Is it parsed with invalid sequences replaced by the replacement character, or
is
> the message lost?


According to the spec https://tools.ietf.org/html/rfc6455#section-8.1 the
connection must be closed by the receiving end.

[Lasse Reichstein Nielsen, 2016-08-24 14:16:48]

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.dart
File sdk/lib/io/websocket.dart (right):

https://codereview.chromium.org/2260073002/diff/20001/sdk/lib/io/websocket.da...
sdk/lib/io/websocket.dart:419: void sendMessage(WebSocketMessageType type,
List<int> bytes);
That's a perfectly reasonable response, and it should be documented here as
well. IT's not necessary for `add` where we do the encoding, but here we should
document what the user should do and what happens if they don't.

[rmacnak, 2016-08-24 17:46:41]

Description was changed from

==========
RFC: Sending pre-encoded text over a web socket.

I'd like to be able to send a byte array as a text frame over a web socket to
avoid a round-trip conversion. In particular, the space overhead of the
conversion, which can lead to the process being killed on iOS/Android. Here's a
proposed way to do it.
==========

to

==========
Add WebSocket.addUtf8Text to allow sending pre-encoded text without a round-trip
UTF-8 conversion.

Use it to implement the vm-service, where in particular we are concerned about
the space overhead of the conversion leading to the process being killed on iOS.

Closes #27129
==========

[rmacnak, 2016-08-24 17:49:27]

Recast as addUtf8Text. PTAL.

[Lasse Reichstein Nielsen, 2016-08-24 20:12:46]

lgtm

https://chromiumcodereview.appspot.com/2260073002/diff/40001/sdk/lib/io/webso...
File sdk/lib/io/websocket_impl.dart (right):

https://chromiumcodereview.appspot.com/2260073002/diff/40001/sdk/lib/io/webso...
sdk/lib/io/websocket_impl.dart:1187: throw new ArgumentError(bytes);
throw new ArgumentError.value(bytes, "bytes", "Is not a list of bytes");

[rmacnak, 2016-08-24 23:14:43]

https://chromiumcodereview.appspot.com/2260073002/diff/40001/sdk/lib/io/webso...
File sdk/lib/io/websocket_impl.dart (right):

https://chromiumcodereview.appspot.com/2260073002/diff/40001/sdk/lib/io/webso...
sdk/lib/io/websocket_impl.dart:1187: throw new ArgumentError(bytes);
On 2016/08/24 20:12:46, Lasse Reichstein Nielsen wrote:
> throw new ArgumentError.value(bytes, "bytes", "Is not a list of bytes");

Done.

[rmacnak, 2016-08-24 23:15:13]

Description was changed from

==========
Add WebSocket.addUtf8Text to allow sending pre-encoded text without a round-trip
UTF-8 conversion.

Use it to implement the vm-service, where in particular we are concerned about
the space overhead of the conversion leading to the process being killed on iOS.

Closes #27129
==========

to

==========
Add WebSocket.addUtf8Text to allow sending pre-encoded text without a round-trip
UTF-8 conversion.

Use it to implement the vm-service, where in particular we are concerned about
the space overhead of the conversion leading to the process being killed on iOS.

Closes #27129

R=johnmccutchan@google.com, lrn@google.com

Committed:
https://github.com/dart-lang/sdk/commit/aa38062a239c7507fa9a902f53473b2cc6b150cb
==========

[rmacnak, 2016-08-24 23:15:14]

Committed patchset #4 (id:60001) manually as
aa38062a239c7507fa9a902f53473b2cc6b150cb (presubmit successful).