stream: Pipe data in chunks matching read data #4777
Conversation
|
The pipeChunkSize argument is going away. But I can see that just shifting off the top chunk could potentially be faster, in cases where the size doesn't matter, since it saves a few copies. A better approach would be to default to Negative numbers should emit error, and be documented. That's confusing and weird. I'd rather not overload the semantics even more than they already are. |
This creates better flow for large values of lowWaterMark.
|
Thanks for the feedback. I have updated the code to accommodate your suggestion, making for a much simpler patch, which is nice. By the way, this is not only performance related, but also flow related, where currently a pipe will always flow in chunks of >= lowWaterMark. This doesn't work all that well when the data is slow to process. |
|
So, this is not actually much of a performance impact, afaict, but it does seem more correct, and it will certainly be more efficient in the corner cases where you're not just shoving data out as fast as possible (which, for obvious reasons, are harder to benchmark). It doesn't break any tests, and since the |
|
I'm not sure if it's just my machine or what, but this commit is causing @isaacs unless a fix can be found quickly, think it should be reverted. hard to determine if this is affecting other similar areas i'm working on. |
|
Yes, this started with the port to streams2. On Tuesday, February 19, 2013, Gil Pedersen wrote:
|
* buffer:
- You can now supply an encoding argument when filling a
Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
an existing Buffer will also work with
Buffer#fill(buffer[, start[, end]]). See the API documentation for
details on how this works. (Trevor Norris) nodejs#4935
- Buffer#indexOf() no longer requires a byteOffset argument if you
also wish to specify an encoding:
Buffer#indexOf(val[, byteOffset][, encoding]).
(Trevor Norris) nodejs#4803
* child_process: spawn() and spawnSync() now support a 'shell' option
to allow for optional execution of the given command inside a shell.
If set to true, cmd.exe will be used on Windows and /bin/sh
elsewhere. A path to a custom shell can also be passed to override
these defaults. On Windows, this option allows .bat. and .cmd files
to be executed with spawn() and spawnSync(). (Colin Ihrig) nodejs#4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
strict limitation of allowable header characters.
(James M Snell) nodejs#5237
* dgram: socket.send() now supports accepts an array of Buffers or
Strings as the first argument. See the API docs for details on how
this works. (Matteo Collina) nodejs#4374
* http: Fix a bug where handling headers will mistakenly trigger an
'upgrade' event where the server is just advertising its protocols.
This bug can prevent HTTP clients from communicating with HTTP/2
enabled servers. (Fedor Indutny) nodejs#4337
* net: Added a listening Boolean property to net and http servers to
indicate whether the server is listening for connections.
(José Moreira) nodejs#4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
it from inside another MakeCallback() call no longer causes the
nextTick queue or Promises microtask queue to be processed out of
order. (Trevor Norris) nodejs#4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
TLS protocol version of the current connection. (Brian White) nodejs#4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
new vm.Script() to interact with V8's code cache. When a new
vm.Script object is created with the 'produceCachedData' set to true
a Buffer with V8's code cache data will be produced and stored in
cachedData property of the returned object. This data in turn may be
supplied back to another vm.Script() object with a 'cachedData'
option if the supplied source is the same. Successfully executing a
script from cached data can speed up instantiation time. See the API
docs for details. (Fedor Indutny) nodejs#4777
* performance: Improvements in:
- process.nextTick() (Ruben Bridgewater) nodejs#5092
- path module (Brian White) nodejs#5123
- querystring module (Brian White) nodejs#5012
- streams module when processing small chunks (Matteo Collina) nodejs#4354
PR-URL: nodejs/node#5295
This creates better flow when lowWaterMark is much bigger than bufferSize, and backpressure is applied.
Also changes the read() function to accept the number '-1', which will return a chunk of optimal size, as determined by read().
I'm not certain about the read() change, since it further overloads the meaning of the read() argument. In one sense it might make more sense to use any negative number because, though the implementation has previously rejected negative read() arguments, the documentation doesn't prohibit them. Thus, logically, this would simply be a bug fix, and not further overloading the meaning of the read() argument.
I will happily make another patch without the overloaded read() argument, but initially I opted for the simpler implementation (with bigger implications).
Also, I have verified that it passes any relevant unit tests, and that it increases performance under the provided conditions.