-
Notifications
You must be signed in to change notification settings - Fork 31k
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
stream: refactor clearBuffer and writableState #45682
Conversation
Review requested:
|
Doesn't seem to be reproducible. A re-run of the benchmarks shows:
|
Thanks for the update @mscdex. Do you have any particular reason for this flakiness? I've added a new commit and re-run the benchmark CI. If it's still the same, I'll update the description and remove the references to the benchmark results. |
No, but it's always best to re-run the benchmarks several times to ensure consistent results. |
const len = objectMode ? 1 : chunk.length; | ||
doWrite(stream, state, false, len, chunk, encoding, callback); | ||
} while (i < buffered.length && !state.writing); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unnecessary change
@@ -537,16 +529,18 @@ function clearBuffer(stream, state) { | |||
return; | |||
} | |||
|
|||
state.bufferProcessing = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary length operation was done before state.bufferProcessing
assignment is done. This change eliminates that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see it. This is actually slower for the case where bufferedLength is 0... since it unnecessarily sets and unsets bufferProcessing.
This pull request refactors
clearBuffer
function and removes the unnecessarytypeof isDuplex
check on WritableState class.