doc: clarify writable.cork method - #30442
Conversation
Sorry, something went wrong.
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying resource clearer.
BridgeAR
left a comment
There was a problem hiding this comment.
I like the new description. I personally would still keep something about the adverse performance impact around though.
Sorry, something went wrong.
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying destination clearer - while keeping performance considerations clearly in mind.
|
Thanks for the feedback. I think it is good to make sure that performance considerations are kept clearly in mind. I've added some verbiage to that effect. |
Sorry, something went wrong.
| The primary intent of `writable.cork()` is to accommodate a situation in which | ||
| it is more performant to write several small chunks to the internal buffer | ||
| rather than drain them immediately to the underlying destination. Such | ||
| buffering is usually inadvisable as it typically degrades performance, but |
There was a problem hiding this comment.
Such buffering is usually inadvisable as it typically degrades performance
I don't think degraded performance is every advisable. This could be a bit more simple.
Such buffering typically degrades performance
Also it could be a bit more explicit. Why and how does it degrade performance? I would think it actually is just as likely to improve performance (if _writev is implemented) at a cost of latency and memory usage? Do we have any benchmarks to clarify this?
Hint, should probably mention latency and memory usage more explicitly.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for your suggestion regarding the phrasing. I have modified the sentence accordingly. Regarding the details of performance degradation due to buffering, I view these as beyond the scope of the current change. They are described in detail in https://nodejs.org/api/stream.html#stream_buffering and https://nodejs.org/api/stream.html#stream_writable_write_chunk_encoding_callback and referred to frequently throughout the description of the Stream API, and my intent is not to provide additional detail here, but only to improve the structure of a sentence that was not clearly interpretable as it stood.
Sorry, something went wrong.
ronag
left a comment
There was a problem hiding this comment.
LGTM
P.S. I think there is a slight error in your PR description (see the markup for the second checkbox).
Sorry, something went wrong.
|
@GKJCJG Please fix your commits though. The first commit needs to have a correctly formatted message (you can see the linting rules in the travis failure). Will make life easier for whoever lands this change. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
I find the first/current version more correct than then the second one.
Specifically:
- it is more performant to work in batches (_writev), in all cases where a native resource is involved as it minimizes the JS/C++ transitions. It’s not the buffering of small chunks that’s costly, but rather the overahead of processing them one-by-one.
- Buffering is the key function of streams, and it’s needed to perform backpressure/control flow. The new text implies it’s bad and something to normally avoid and I disagree.
- Do not use the term “drain”, as it recall a Writable stream term.
Sorry, something went wrong.
It actually depends on the implementation of _writev() and I think this sentence
with "can" is trying to address this. |
Sorry, something went wrong.
|
I think something like this is better reflect our implementations and its usage. The primary intent of `writable.cork()` is to accommodate a situation in which several small chunks are written to the stream in rapid succession. Instead of immediately forwarding them to the underlining destination, `writable.cork()` buffers all the chunks until `writable.uncork()` is called, which will pass them all to `writable._writev()`, if present. This prevents an head-of-line blocking situation where data is being buffered while waiting for the first small chunk to be processed. Note that using `writable.cork()` without implementing `writable._writev()` is likely to have an adverse effect on throughput. |
Sorry, something went wrong.
More detailed explanation of underlying dynamics.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Anna Henningsen <github@addaleax.net>
|
Travis CI keeps failing at the check of the commit message, but it seems as though it's still using the very first commit message I used before I added the "doc" prefix. Any suggestions for satisfying its commit message linting? |
Sorry, something went wrong.
|
@GKJCJG Please ignore it. I know it’s super annoying that Travis complains, but whoever merges this will take care of fixing the commit message. |
Sorry, something went wrong.
Sorry, something went wrong.
|
@GKJCJG please run the doc linter once (or the whole one make lint). Seems like there are two linter errors: 01:49:04 Running Markdown linter on docs... 01:49:33 �[4m�[33mdoc/api/stream.md�[39m�[24m 01:49:33 367:72 �[33mwarning�[39m Remove trailing whitespace no-trailing-spaces remark-lint 01:49:33 371:1 �[33mwarning�[39m Remove 1 line before node no-consecutive-blank-lines remark-lint |
Sorry, something went wrong.
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying destination clearer - while keeping performance considerations clearly in mind. PR-URL: #30442 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying destination clearer - while keeping performance considerations clearly in mind. PR-URL: #30442 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying destination clearer - while keeping performance considerations clearly in mind. PR-URL: #30442 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
The syntax of the sentence describing the role of writable.cork() was unclear. This rephrase aims to make the distinction between writing to the buffer and draining immediately to the underlying destination clearer - while keeping performance considerations clearly in mind. PR-URL: #30442 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com>
The syntax of the sentence describing the role of writable.cork() was
unclear. This rephrase aims to make the distinction between writing
to the buffer and draining immediately to the underlying destination
clearer.
Checklist