lib: remove queue implementation from JSStreamWrap #17918

addaleax · 2017-12-29T20:42:16Z

The streams implementation generally ensures that only one write()
call is active at a time. JSStreamWrap instances still kept
queue of write reqeuests in spite of that; refactor it away.

Also, fold isAlive() into a constant function on the native side.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib

bnoordhuis

Nice work.

bnoordhuis · 2017-12-29T20:50:52Z

lib/internal/wrap_js_stream.js

+    this[kCurrentWriteRequest] = null;
+    this[kCurrentShutdownRequest] = null;
+
+    // Start reading


Dot at end. Also, somewhat superfluous comment.

I don’t think it’s superfluous – I could imagine there’s quite a few who wouldn’t know why you’d ever want to call .read(0).

bnoordhuis · 2017-12-29T20:52:54Z

lib/internal/wrap_js_stream.js


    this.stream.cork();
-    for (var n = 0; n < bufs.length; n++)
-      this.stream.write(bufs[n], done);
+    for (var i = 0; i < bufs.length; i++)


If you're making stylistic changes anyway: let i = ... (or for (const buf of bufs))

We are not allowed to use let in loops, the linter yells about that. :(

for (const buf of bufs) seems to work though!

bnoordhuis · 2017-12-29T20:56:13Z

lib/internal/wrap_js_stream.js


-    var pending = bufs.length;
+    const handle = this._handle;


The linter would complain about that. :)

I'm not seeing it, though. Same for the const handle = ... on line 112.

It’s passed to the self.finishWrite() call down on line 162

Oh, indeed. Unfortunate fold in the diff.

jasnell · 2017-12-29T21:21:50Z

lib/internal/wrap_js_stream.js

-    handle.onwrite = (req, bufs) => this.doWrite(req, bufs);
+    // Inside of the following functions, `this` refers to the handle
+    // and `this.owner` refers to this JSStreamWrap instance.
+    handle.isClosing = function() { return this.owner.isClosing(); };


would be slightly better to avoid the closures entirely and just make these top level functions.

@jasnell done!

addaleax · 2018-01-02T15:02:09Z

CI: https://ci.nodejs.org/job/node-test-commit/15175/

billouboq · 2018-01-01T19:14:09Z

lib/internal/wrap_js_stream.js


-    var pending = bufs.length;
+    const handle = this._handle;
+    const self = this;


Is it still needed ?

@billouboq it’s used inside the done() function below :)

mcollina · 2018-01-03T15:03:28Z

lib/internal/wrap_js_stream.js


    this.stream.cork();
-    for (var n = 0; n < bufs.length; n++)
-      this.stream.write(bufs[n], done);
+    for (const buf of bufs)


can you use a for(;;) loop here? for-of is still extremely slower than for(;;).

mcollina · 2018-01-03T15:04:07Z

lib/internal/wrap_js_stream.js

-
-        handle.finishWrite(req, errCode);
+      setImmediate(() => {
+        self.finishWrite(handle, errCode);


you can avoid this closure completely with setImmediate(finishWrite, self, handle, errCode).

mcollina · 2018-01-03T15:05:00Z

lib/internal/wrap_js_stream.js

+
+      this.finishWrite(handle, uv.UV_ECANCELED);
+      this.finishShutdown(handle, uv.UV_ECANCELED);
+


can you remove this closure as well?

addaleax · 2018-01-03T18:07:27Z

@mcollina I’ll get to addressing your comments later, but I’ll point out that at least in the case of the for loop that was a suggestion by an earlier comment (I can switch back tho) and that this is the slow path already, so squeezing the last bits of performance out of it might not be that important

The streams implementation generally ensures that only one write() call is active at a time. `JSStreamWrap` instances still kept queue of write reqeuests in spite of that; refactor it away. Also, fold `isAlive()` into a constant function on the native side.

addaleax · 2018-01-04T20:40:40Z

@mcollina I’ve addressed the for (const … of …) suggestion by you.

I also tried to benchmark all of the suggested changes via the benchmark added in #17983, but did not get a statistically significant improvement as a result, so I’d prefer to stick to the more readable code here.

mcollina · 2018-01-05T10:17:05Z

I'm 👍 if you want to keep if it does not regress anything. I prefer to not use code that might become problematic just for stylistic reasons (and for (;;) is beautiful).

Have you thought about refactoring all those closures as well? I don't expect to get any perf boost, but it might help memory consumption and the gc in the long run. Feel free to ignore those, it's just a nit.

mcollina

LGTM

addaleax · 2018-01-05T17:11:05Z

@mcollina I benchmarked all of your suggestions together (via that linked benchmark). I hadn’t really thought about gc, thanks for the reminder! What would be the best way to measure the impact of a change like that?

bnoordhuis · 2018-01-05T20:28:05Z

@addaleax Slight tangent but an idea I've had is roughly to:

Instrument with v8::Isolate::AddGCEpilogueCallback()
Log memory usage reported by v8::Isolate::GetHeapSpaceStatistics() after every GC
Collate the data after the process exits

With the idea that you compare runs of the benchmarks or the test suite to see if there are large swings in GC frequency or heap usage.

mcollina · 2018-01-05T22:03:16Z

I used --trace-gc-object-stats in the past with great success.

joyeecheung · 2018-01-06T10:08:32Z

@bnoordhuis I think what you've descried is basically what --trace_gc --trace_gc_verbose does?

bnoordhuis · 2018-01-06T11:24:33Z

By 'log' I mean 'write to file' in a format that's machine readable.

--trace_gc dumps everything to stdout interleaved with normal output. Okay for eyeballing stuff, not so great for automation.

addaleax · 2018-01-07T20:32:13Z

Landed in b171adc

I’ll take a look at the closures at another point, this PR isn’t introducing new ones and I’d like to get around to addressing the issue from #17938 (comment)

The streams implementation generally ensures that only one write() call is active at a time. `JSStreamWrap` instances still kept queue of write reqeuests in spite of that; refactor it away. Also, fold `isAlive()` into a constant function on the native side. PR-URL: #17918 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

MylesBorins · 2018-01-09T10:17:47Z

This is currently breaking some stuff on v9.x

It is possible that after #18050 this will work

addaleax · 2018-01-10T15:43:35Z

@MylesBorins It applies and tests cleanly on my machine … could you show the error you were seeing?

MylesBorins · 2018-01-10T17:21:48Z

@addaleax I'm assuming it was unblocked by the http2 update. Will look at this again in a bit

The streams implementation generally ensures that only one write() call is active at a time. `JSStreamWrap` instances still kept queue of write reqeuests in spite of that; refactor it away. Also, fold `isAlive()` into a constant function on the native side. PR-URL: #17918 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 29, 2017

bnoordhuis approved these changes Dec 29, 2017

View reviewed changes

jasnell reviewed Dec 29, 2017

View reviewed changes

jasnell approved these changes Dec 29, 2017

View reviewed changes

tniessen approved these changes Dec 29, 2017

View reviewed changes

JungMinu approved these changes Dec 30, 2017

View reviewed changes

cjihrig approved these changes Dec 30, 2017

View reviewed changes

addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jan 2, 2018

TimothyGu approved these changes Jan 3, 2018

View reviewed changes

billouboq reviewed Jan 3, 2018

View reviewed changes

mcollina reviewed Jan 3, 2018

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2018

addaleax added 4 commits January 4, 2018 20:38

[squash] bnoordhuis comments

0cf413b

[squash] jasnell comment

51e7ecc

[squash] move away from for-const-of loop

a17b0d9

addaleax force-pushed the wrap-js-stream-simplify branch from 4be705e to a17b0d9 Compare January 4, 2018 20:35

mcollina approved these changes Jan 5, 2018

View reviewed changes

addaleax added dont-land-on-v4.x labels Jan 7, 2018

addaleax added the dont-land-on-v8.x label Jan 7, 2018

addaleax closed this Jan 7, 2018

addaleax deleted the wrap-js-stream-simplify branch January 7, 2018 20:32

MylesBorins added the backport-requested-v9.x label Jan 9, 2018

addaleax removed the backport-requested-v9.x label Jan 10, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove queue implementation from JSStreamWrap #17918

lib: remove queue implementation from JSStreamWrap #17918

addaleax commented Dec 29, 2017

bnoordhuis left a comment

bnoordhuis Dec 29, 2017

addaleax Dec 29, 2017

bnoordhuis Dec 29, 2017

addaleax Dec 29, 2017

bnoordhuis Dec 29, 2017

addaleax Dec 29, 2017

bnoordhuis Dec 29, 2017

addaleax Dec 29, 2017

bnoordhuis Dec 29, 2017

jasnell Dec 29, 2017

addaleax Dec 29, 2017

addaleax commented Jan 2, 2018

billouboq Jan 1, 2018

addaleax Jan 4, 2018

mcollina Jan 3, 2018

mcollina Jan 3, 2018

mcollina Jan 3, 2018

addaleax commented Jan 3, 2018

addaleax commented Jan 4, 2018

mcollina commented Jan 5, 2018

mcollina left a comment

addaleax commented Jan 5, 2018

bnoordhuis commented Jan 5, 2018

mcollina commented Jan 5, 2018

joyeecheung commented Jan 6, 2018

bnoordhuis commented Jan 6, 2018

addaleax commented Jan 7, 2018

MylesBorins commented Jan 9, 2018

addaleax commented Jan 10, 2018

MylesBorins commented Jan 10, 2018


		this.finishWrite(handle, uv.UV_ECANCELED);
		this.finishShutdown(handle, uv.UV_ECANCELED);

lib: remove queue implementation from JSStreamWrap #17918

lib: remove queue implementation from JSStreamWrap #17918

Conversation

addaleax commented Dec 29, 2017

Checklist

Affected core subsystem(s)

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jan 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jan 3, 2018

addaleax commented Jan 4, 2018

mcollina commented Jan 5, 2018

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Jan 5, 2018

bnoordhuis commented Jan 5, 2018

mcollina commented Jan 5, 2018

joyeecheung commented Jan 6, 2018

bnoordhuis commented Jan 6, 2018

addaleax commented Jan 7, 2018

MylesBorins commented Jan 9, 2018

addaleax commented Jan 10, 2018

MylesBorins commented Jan 10, 2018