Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: clarify writable.cork method #30442

Closed
wants to merge 8 commits into from
Closed

doc: clarify writable.cork method #30442

wants to merge 8 commits into from

Conversation

GKJCJG
Copy link
Contributor

@GKJCJG GKJCJG commented Nov 12, 2019

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

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.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Nov 12, 2019
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new description. I personally would still keep something about the adverse performance impact around though.

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.
@GKJCJG
Copy link
Contributor Author

GKJCJG commented Nov 14, 2019

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.

@GKJCJG GKJCJG changed the title Clarify role of writable.cork() doc: Clarify role of writable.cork() Nov 14, 2019
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
Copy link
Member

@ronag ronag Nov 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

P.S. I think there is a slight error in your PR description (see the markup for the second checkbox).

@ronag
Copy link
Member

ronag commented Nov 17, 2019

@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.

@GKJCJG GKJCJG changed the title doc: Clarify role of writable.cork() doc: clarify role of writable.cork() Nov 18, 2019
@GKJCJG GKJCJG changed the title doc: clarify role of writable.cork() doc: clarify role of writable.cork method Nov 18, 2019
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the first/current version more correct than then the second one.

Specifically:

  1. 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.
  2. 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.
  3. Do not use the term “drain”, as it recall a Writable stream term.

@lpinca
Copy link
Member

lpinca commented Nov 18, 2019

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

It actually depends on the implementation of _writev() and I think this sentence

implementations that implement the writable._writev() method can perform buffered writes in a more optimized manner

with "can" is trying to address this.

@mcollina
Copy link
Member

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.

More detailed explanation of underlying dynamics.
@GKJCJG GKJCJG changed the title doc: clarify role of writable.cork method doc: clarify writable.cork method Nov 21, 2019
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

doc/api/stream.md Outdated Show resolved Hide resolved
doc/api/stream.md Outdated Show resolved Hide resolved
GKJCJG and others added 2 commits December 3, 2019 17:00
Co-Authored-By: Anna Henningsen <github@addaleax.net>
Co-Authored-By: Anna Henningsen <github@addaleax.net>
@GKJCJG
Copy link
Contributor Author

GKJCJG commented Dec 3, 2019

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?

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 3, 2019
@addaleax
Copy link
Member

addaleax commented Dec 3, 2019

@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.

@BridgeAR
Copy link
Member

@BridgeAR
Copy link
Member

@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

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2019
BridgeAR pushed a commit that referenced this pull request Jan 1, 2020
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>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 1, 2020

Landed in e9695d9

@GKJCJG congratulations on your first commit to Node.js! 🎉

I pleased the linter and the commit message while landing.

@BridgeAR BridgeAR closed this Jan 1, 2020
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
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>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
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>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
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>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants