Merge all http-outgoing transfomers into _HttpOutgoing, including simpler(better) buffering.

Merge all http-outgoing transfomers into _HttpOutgoing, including simpler(better) buffering.

This speeds up the system by:
- Using a simpler buffering mechanism, where we fill up a 8kb buffer, pass it on, and allocating a new.
- Avoiding up to 4 streams for each stream added to the http-outgoing. We now only create one extra stream.

BUG=
R=sgjesse@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33272

[Lasse Reichstein Nielsen, 2014-03-03 11:37:23]

Just scanned for comments.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:426: (_target as _HttpOutboundConsumer).outbound =
this;
Consider just doing an assignment to a _HttpOutboundConsumer variable.
Or have a getter
  _HttpOutboundConsumer get _httpTarget => _target
to avoid using "as". Using "as" always does a dynamic type check, even in
unchecked mode, and you don't want that when you know that the type is right.

In short: never use "as".

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:516: stream.drain().catchError((_) {});
The "drain" method just ignores input and waits for done.
Wouldn't it be better to start listening and immediately cancel? Then the
underlying system may be able to stop the data from even being sent (in some
hypothetical world where the sender is told that we stopped listening).
Drain uses a cancelOnError subscription, so it should be able to handle the
cancel.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:541: "No content while contentLength was specified to
be greater "
while -> and
Or perhaps "even though".

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1037: cancelOnError: true);
If you want to avoid extra closures, you can swap the assignments to controller
and sub:
  sub = stream.listen((data) {...},
                      onError: null, onDone: null);
  controller = new StreamController(
    onPause: sub.pause,
    onResume: sub.resume,
    sync: true);
  sub.onError(controller.addError);
  sub.onDone(controller.close);

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1039: // If incoming is being drained, the pauseFuture
is != null. Pause output
If -> while? Is this the only case where pauseFuture is non-null?
"!= null" -> non-null.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1062: // If we was already closed, return that future.
was -> are

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1064: // If we earlier saw an error, return immidiate.
The internals are already
if we have seen an error, return immediately.
internals -> ? (What are they?)

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1072: " $_bytesWritten bytes written while expected "
while -> but.

https://codereview.chromium.org/185543004/diff/1/tests/standalone/io/http_cli...
File tests/standalone/io/http_client_connect_test.dart (right):

https://codereview.chromium.org/185543004/diff/1/tests/standalone/io/http_cli...
tests/standalone/io/http_client_connect_test.dart:173: //
testGetDataServerForceClose();
Then remove it if it cannot run.

[Anders Johnsen, 2014-03-03 19:43:14]

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:426: (_target as _HttpOutboundConsumer).outbound =
this;
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> Consider just doing an assignment to a _HttpOutboundConsumer variable.
> Or have a getter
>   _HttpOutboundConsumer get _httpTarget => _target
> to avoid using "as". Using "as" always does a dynamic type check, even in
> unchecked mode, and you don't want that when you know that the type is right.
> 
> In short: never use "as".

I see. However, this is planned to go away, so I'll not touch it for now (see
other TODOs in this CL).

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:516: stream.drain().catchError((_) {});
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> The "drain" method just ignores input and waits for done.
> Wouldn't it be better to start listening and immediately cancel? Then the
> underlying system may be able to stop the data from even being sent (in some
> hypothetical world where the sender is told that we stopped listening).
> Drain uses a cancelOnError subscription, so it should be able to handle the
> cancel.

No. The remote user may see that as write failing, thus aborting the request.
That the local user doesn't want to look at the request data should be all
transparent.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:541: "No content while contentLength was specified to
be greater "
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> while -> and
> Or perhaps "even though".

Done.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1037: cancelOnError: true);
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> If you want to avoid extra closures, you can swap the assignments to
controller
> and sub:
>   sub = stream.listen((data) {...},
>                       onError: null, onDone: null);
>   controller = new StreamController(
>     onPause: sub.pause,
>     onResume: sub.resume,
>     sync: true);
>   sub.onError(controller.addError);
>   sub.onDone(controller.close);

I'm fine by doing it like this. Also, this will not create extra closures (it's
just forwarding the methods).

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1039: // If incoming is being drained, the pauseFuture
is != null. Pause output
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> If -> while? Is this the only case where pauseFuture is non-null?
> "!= null" -> non-null.

Done. No, it can be it in many cases (one of the reasons I want to merge writing
headers into this).

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1062: // If we was already closed, return that future.
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> was -> are

Done.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1064: // If we earlier saw an error, return immidiate.
The internals are already
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> if we have seen an error, return immediately.
> internals -> ? (What are they?)

Done.

https://codereview.chromium.org/185543004/diff/1/sdk/lib/io/http_impl.dart#ne...
sdk/lib/io/http_impl.dart:1072: " $_bytesWritten bytes written while expected "
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> while -> but.

Done.

https://codereview.chromium.org/185543004/diff/1/tests/standalone/io/http_cli...
File tests/standalone/io/http_client_connect_test.dart (right):

https://codereview.chromium.org/185543004/diff/1/tests/standalone/io/http_cli...
tests/standalone/io/http_client_connect_test.dart:173: //
testGetDataServerForceClose();
On 2014/03/03 11:37:23, Lasse Reichstein Nielsen wrote:
> Then remove it if it cannot run.

Ahh yes, adding issue #.

[Søren Gjesse, 2014-03-04 09:29:14]

LGTM

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.da...
sdk/lib/io/http_impl.dart:466: } catch (error, s) {
s is not used.

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.da...
sdk/lib/io/http_impl.dart:1039: _addChunk(data, controller.add);
Please moving this up to the two placed where it falls through to here. Then you
can remove the returns as well.

https://codereview.chromium.org/185543004/diff/110001/tests/standalone/io/htt...
File tests/standalone/io/http_outgoing_size_test.dart (right):

https://codereview.chromium.org/185543004/diff/110001/tests/standalone/io/htt...
tests/standalone/io/http_outgoing_size_test.dart:14: for (int i = 0; i <
sendData.length; i++) sendData[i] = i;
Change i to i % 255 for clarity.

[Anders Johnsen, 2014-03-04 13:54:16]

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.da...
sdk/lib/io/http_impl.dart:466: } catch (error, s) {
On 2014/03/04 09:29:15, Søren Gjesse wrote:
> s is not used.

Done.

https://codereview.chromium.org/185543004/diff/110001/sdk/lib/io/http_impl.da...
sdk/lib/io/http_impl.dart:1039: _addChunk(data, controller.add);
On 2014/03/04 09:29:15, Søren Gjesse wrote:
> Please moving this up to the two placed where it falls through to here. Then
you
> can remove the returns as well.

Done.

https://codereview.chromium.org/185543004/diff/110001/tests/standalone/io/htt...
File tests/standalone/io/http_outgoing_size_test.dart (right):

https://codereview.chromium.org/185543004/diff/110001/tests/standalone/io/htt...
tests/standalone/io/http_outgoing_size_test.dart:14: for (int i = 0; i <
sendData.length; i++) sendData[i] = i;
On 2014/03/04 09:29:15, Søren Gjesse wrote:
> Change i to i % 255 for clarity.

Done, with % 256

[Anders Johnsen, 2014-03-04 13:58:56]

Committed patchset #9 manually as r33272 (presubmit successful).