streams: add cork option to pipe

[calvinmetcalf]

Adds an option to .pipe to cork it before each write and
then uncork it next tick, based on discussion at
nodejs/readable-stream#145

[chrisdickinson]

Curious: why not just call cork on the destination stream to start with? Then the highwatermark handling will take care of subsequent writev flushing?

[trevnorris]

cork knows about the high water mark? There's something I must be misunderstanding.

[mscdex]

there should probably be a comma after Before each write

[calvinmetcalf]

@chrisdickinson because then you have to wait until you hit the highwatermark before you actually write anything, see the comment by@indutny in the readable stream issue

[mscdex]

I wonder if pulling this anonymous function out of maybeCork() would help performance-wise?

[chrisdickinson]

OK, I was wrong – cork and uncork have no idea about highwatermark. That said, I'm not sure I'm fully on board with this change (though I may be misunderstanding it.)

So, to check my assumptions about how streams work (and in particular, how net.Streams work):

1. First tick of the event loop.
   1. We write once to the net.Socket. It immediately tries to flush to the underlying TCPWrap.
   2. We write again to the net.Socket, N times. These all buffer, while the first write finishes.
2. First write completes.
   1. We flush all buffered writes to the TCPWrap using _writev.
   2. All writes that happen while this large buffer flushes are themselves buffered, until HWM.

Right now streams will be flushing chunks without using cork, uncork. The only write that is guaranteed to escape that is the first write. The size of the outgoing packet is determined by the amount of data that the source stream can generate during packet flushes.

[chrisdickinson]

Things that could cause my assumptions to be incorrect:

1. nodelay TCPWrap writes could be synchronous (I don't know if they are for sure, but some initial research seems to point to "they're async.")
2. the importance of batching writes for large packets is higher than I assume (that is to say, we want to collect more than "amount of data the source can generate during a flush" bytes before flushing.)

These both seem like TCP-specific concerns – it might be better to solve them at the net.Socket level than at the stream.Writable level.

[calvinmetcalf]

the importance of batching writes for large packets is higher than I assume (that is to say, we want to collect more than "amount of data the source can generate during a flush" bytes before flushing.)

This is the more general benefit that could apply to other streams that want to strike a balance between per write overhead and latency

[trevnorris]

Just for reference, this is basically the same thing the http module does today.

[trevnorris]

Another key component in this is the interaction with uv_try_write. You want to immediately write out as much as the kernel can handle then queue the remaining. It's not uncommon that all writes can be done immediately. This affects all uv_stream_t instances.

[calvinmetcalf]

@mscdex updated based on your suggestions

[indutny]

@trevnorris still it is faster to do just one writev call than multiple ones.

[trevnorris]

@indutny internally doesn't it automatically write out as much as possible using uv_try_write before setting up the WriteReq?

[indutny]

Yes, but it will pass multiple buffers as a single input with writev.

[trevnorris]

Sure, but uv_try_write only takes one at a time. That's what I was trying to get at above. Thought it may be a performance advantage to write immediately until uv_try_write fails, then queue up the remaining for writev. Simply because between the first set of running uv_try_write the kernel may have flushed some of the data and could accept more when writev ran.

But the timing could also be so minimal that it doesn't really matter.

[indutny]

Not really, it takes multiple:

UV_EXTERNintuv_try_write(uv_stream_t*handle,
constuv_buf_tbufs[],
unsigned intnbufs);

[indutny]

Though, your comments are quite correct. I am not suggesting this should be a default behavior in any way. But for my use case it would be beneficial to introduce this option, otherwise I will need to concatenate the buffers manually in memory.

[trevnorris]

Doh. Memory failure. Thanks for correcting me.

My comment was more just an observation I realized while looking over this PR. Definitely not something I think should be introduced in this PR. :-)

[chrisdickinson]

@calvinmetcalf:

This is the more general benefit that could apply to other streams that want to strike a balance between per write overhead and latency

We're talking about bringing back lowWatermark?

@indutny:

Based on this comment, it seems like you should be seeing at least one writev of size N>1. Is the problem that:

• you are not seeing anywritev of size N>1 happen, or
• the writev calls you are seeing are all of N<desired-size, or
• the initial writev call is of size N==1?

[indutny]

@chrisdickinson they can't happen because I am piping to the socket, not writing to it myself. So every write results in separate write() syscall, unless some of them will fail to complete immediately and will result in buffering (which is rare in the most setups I am using).

[ronkorving]

I have a use-case where I'm piping from a _transform to a writable, and would benefit from the same solution. For me however, nextTick would be overkill (not sure what the cost of a nexttick is tbh), as _transform already uses a callback to denote the end of a batch of writes. Perhaps the transform use case could be optimized?

[ronkorving]

Not technically correct in the case of autoCork being false.

[calvinmetcalf]

True should be more like, 'no need to cork'

[ronkorving]

I just submitted #2167 which I think might really benefit from this.

[brendanashworth]

Is there a time when dest.cork isn't a function? Won't it error anyways if it isn't a writable stream?

[indutny]

Old streams?

[brendanashworth]

Ah, that makes sense! Thanks!

[jasnell]

@calvinmetcalf ... ping ... is this still something you'd like to pursue?

[calvinmetcalf]

sure I can rebase

[calvinmetcalf]

closing this as I'm not so sure we need this