This repository was archived by the owner on Apr 22, 2023. It is now read-only.
stream: switch _writableState.buffer to queue#8826
Closed
chrisdickinson wants to merge 2 commits intonodejs:v0.12from
Closed
stream: switch _writableState.buffer to queue#8826chrisdickinson wants to merge 2 commits intonodejs:v0.12from
chrisdickinson wants to merge 2 commits intonodejs:v0.12from
Conversation
In cases where many small writes are made to a stream lacking _writev, the array data structure backing the WriteReq buffer would greatly increase GC pressure. Specifically, in the fs.WriteStream case, the clearBuffer routine would only clear a single WriteReq from the buffer before exiting, but would cause the entire backing array to be GC'd. Switching to [].shift lessened pressure, but still the bulk of the time was spent in memcpy. This replaces that structure with a linked list-backed queue so that adding and removing from the queue is O(1). In the _writev case, collecting the buffer requires an O(N) loop over the buffer, but that was already being performed to collect callbacks, so slowdown should be neglible.
There was a problem hiding this comment.
This is internal/private/undocumented state, I'm not sure we need to do this deprecation warning as we've never really explained to people how to interact with this.
Author
There was a problem hiding this comment.
Cool -- I was mostly concerned with supporting projects like vstream that reach into ReadableState for metrics. I figured that if there's one I know about, there's probably more that I don't know about :)
|
This looks great, I would love to also get a proposal on what it means for streams consumers to inspect the undocumented readable/writable state, and work up an API around that. |
|
I like how you did this, and I'm cool with this being merged. |
chrisdickinson
added a commit
that referenced
this pull request
Dec 18, 2014
In cases where many small writes are made to a stream lacking _writev, the array data structure backing the WriteReq buffer would greatly increase GC pressure. Specifically, in the fs.WriteStream case, the clearBuffer routine would only clear a single WriteReq from the buffer before exiting, but would cause the entire backing array to be GC'd. Switching to [].shift lessened pressure, but still the bulk of the time was spent in memcpy. This replaces that structure with a linked list-backed queue so that adding and removing from the queue is O(1). In the _writev case, collecting the buffer requires an O(N) loop over the buffer, but that was already being performed to collect callbacks, so slowdown should be neglible. PR-URL: #8826 Reviewed-by: Timothy J Fontaine <[email protected]> Reviewed-by: Trevor Norris <[email protected]>
Author
|
Merged in 9158666. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In cases where many small writes are made to a stream lacking
_writev, the array data structure backing theWriteReqbuffer would greatly increase GC pressure.Specifically, in the
fs.WriteStreamcase, theclearBufferroutine would only clear a singleWriteReqfrom the buffer before exiting, but would cause the entire backing array to be GC'd. Switching to[].shiftlessened pressure, but still the bulk of the time was spent inmemcpy.This replaces that structure with a linked list-backed queue so that adding and removing from the queue is O(1). In the
_writevcase, collecting the buffer requires an O(N) loop over the buffer, but that was already being performed to collect callbacks, so slowdown should be neglible.Example code (courtesy @hayes, who originally discovered this slowdown while dealing with log output to bunyan):
before the change:
array.shift()
(this replaces the common c=1 slice case with
[].shift()):as queue:
I'm working on getting a better "after" flamegraph at the moment.
As a measurable effect: the current version clears ~300 or so WriteReqs/second on my machine; the queue version clears ~50k WriteReqs/second.
cc @wraithan
R=@trevnorris @misterdjules