| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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.
Sorry, something went wrong.
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 :)
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
I like how you did this, and I'm cool with this being merged. |
Sorry, something went wrong.
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 <tjfontaine@gmail.com> Reviewed-by: Trevor Norris <trev.norris@gmail.com>
| Back | FazBrowse Home | New Git URL |
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.
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