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

Defer immediate reads #37

Merged
merged 1 commit into from
Mar 9, 2018
Merged

Defer immediate reads #37

merged 1 commit into from
Mar 9, 2018

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Mar 9, 2018

Immediate reads have been introduced to support in-memory streams and STDIN on Windows, but this causes problems during piping of streams that always have data available, because it blocks everything else. This is resolved by deferring the promise resolution to the next tick.

Immediate reads have been introduced to support in-memory streams and STDIN on Windows, but this causes problems during piping of streams that always have data available, because it blocks everything else. This is resolved by deferring the promise resolution to the next tick.
@trowski trowski merged commit ca9128a into master Mar 9, 2018
@kelunik kelunik deleted the fix-immediate branch March 9, 2018 16:48
@bwoebi
Copy link
Member

bwoebi commented Mar 10, 2018

While this is necessary, I am wondering though what it does to individual latencies. Especially if the OS has e.g. 256 KB in its buffer … with a default chunk size of 8192 we're now going to wait for 32 ticks until we can process that one batch of data.

It is generally favorable to batch-process data for latency (obviously, as long as the batch processed data is limited to a certain amount, avoiding the problem described here).

Therefore I do not like that change in this form. Especially considering that we're fixing a rather edge case here. It would perhaps be a good idea to allow up to X read() calls in one tick (perhaps 32) and only then defer. (and reset that counter in onReadable)

@kelunik
Copy link
Member Author

kelunik commented Mar 10, 2018

@bwoebi That's what I previously did. We decided that it's probably not worth it. It's now the same latency as without #24. #24 has only been implemented to support Windows' STDIN.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants