net,src: refactor writeQueueSize tracking by apapirovski · Pull Request #17650 · nodejs/node · GitHub

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. (This has no performance implications based on the benchmarks at net/tcp-raw* which use writeQueueSize extensively.)

For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size entirely in JS land.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

net, src, tls, test

CI: https://ci.nodejs.org/job/node-test-pull-request/12080/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1147/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1148/

Very nice!

Looks like a couple of the CI failures are related, though :(

I can try to look into the Windows failures.

Also, I restarted CitGM since one of the build parameters was slightly off

@addaleax I've almost got the resolution for the OS X failure, looking into Windows

CI: https://ci.nodejs.org/job/node-test-pull-request/12088/
CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1151/

@addaleax This should be ready to review again, just one new commit. The new IsAsync() lets us remove a hack from _writeGeneric which checked whether queue size was 0 alongside req.async (which in turn necessitated a hack in tls.js to make writeQueueSize non-0 during the handshake).

Now req.async (WriteWrap.prototype.async) represents the actual async status of a write request.

What would happen if you just set this to the actual value of the write queue size at this point? Do you know that?

This should always be 0 at this point. I could put an assert to confirm and run our test suite. The getter is obviously a bit more expensive.

Hm … but what if multiple writes were scheduled and this callback ran after the first one finished? That’s a possibility, no?

Only a single write can be scheduled at a time. req.cb() needs to be called (bottom of this function) before the next one will occur.

This feels odd … I think this would be a bit clearer if it was a set/get pair on the WriteWrap instance, defaulting to true, where LibuvStreamWrap::DoWrite sets the value?

I don’t think StreamResource is the right place for this…

I think this might be more of a naming issue maybe? I do think it conceptually belongs on StreamResource because it's supposed to represent whether there's anything in the queue. Right now it's mostly for LibuvStreamWrap but it's not guaranteed that's the only sync write we'll have.

I mean, yeah, if this stays here I think we could come up with a better name :)

But what we use it for is checking whether a particular write was happening asynchronously, right?

I guess it has a dual purpose. Let me think about it a bit more.

@apapirovski Maybe make it HasWriteQueue()?

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

@addaleax CI is pointless... :( Windows is down and that's the only system we need.

Edit: to clarify, I ran a CI last night for everything else but didn't post it here. Just waiting on Windows & Linux to come back.

Opened an issue on https://github.com/nodejs/build — seems like @mhdawson is looking into it already... I guess we wait.

CI: https://ci.nodejs.org/job/node-test-pull-request/12152/

This should work. Will need to look at some of the write code in the future, there are some differences between Windows & *nix that I'm not necessarily comfortable with.

@addaleax This is ready to review finally, should be the last time. Thanks!

I do think we want to run CITGM again, but that should be all:

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1158/

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land.

CI: https://ci.nodejs.org/job/node-test-pull-request-lite/27/

(Rebased after _writableState.length removal landed.)

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land. PR-URL: #17650 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Landed in d36e1b4.

I've marked this as baking-for-lts as I would like to make sure there are no regressions in user code. It shouldn't need to bake too long though.

This does not land cleanly on v9.x could you please backport

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land. PR-URL: nodejs#17650

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land. PR-URL: nodejs/node#17650 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Currently, writeQueueSize is never used in C++ and barely used within JS. Instead of constantly updating the value on the JS object, create a getter that will retrieve the most up-to-date value from C++. For the vast majority of cases though, create a new prop on Socket.prototype[kLastWriteQueueSize] using a Symbol. Use this to track the current write size, entirely in JS land. Backport-PR-URL: #18084 PR-URL: #17650 Reviewed-By: Anna Henningsen <anna@addaleax.net>

apapirovski added lib / src Issues and PRs related to general changes in the lib or src directory. net Issues and PRs related to the net subsystem. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Dec 13, 2017

apapirovski requested a review from addaleax December 13, 2017 14:25

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 13, 2017

apapirovski added dont-land-on-v4.x labels Dec 13, 2017

addaleax approved these changes Dec 13, 2017

View reviewed changes

apapirovski force-pushed the patch-write-queue-size branch 4 times, most recently from 2a83e33 to 5938061 Compare December 13, 2017 21:39

addaleax reviewed Dec 13, 2017

View reviewed changes

apapirovski force-pushed the patch-write-queue-size branch from 5938061 to c81d08a Compare December 14, 2017 03:41

addaleax approved these changes Dec 14, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 14, 2017

apapirovski force-pushed the patch-write-queue-size branch 4 times, most recently from 88dc4f1 to 23f0f3f Compare December 15, 2017 14:34

apapirovski force-pushed the patch-write-queue-size branch 2 times, most recently from 0d1c759 to 586dadb Compare December 16, 2017 21:20

addaleax approved these changes Dec 16, 2017

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 16, 2017

apapirovski force-pushed the patch-write-queue-size branch from 586dadb to a99b447 Compare December 18, 2017 14:14

apapirovski added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 18, 2017

apapirovski closed this Dec 18, 2017

apapirovski deleted the patch-write-queue-size branch December 18, 2017 15:00

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Dec 19, 2017

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

addaleax mentioned this pull request Jan 10, 2018

tls: simplify write mechanism #17883

Closed

2 tasks

addaleax mentioned this pull request Jan 10, 2018

[v9.x] net,src: refactor writeQueueSize tracking #18084

Closed

addaleax added backported-to-v9.x and removed backport-requested-v9.x labels Jan 10, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

ronag mentioned this pull request Jun 27, 2020

net: fix bufferSize #34088

Closed

4 tasks

targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022

net,src: refactor writeQueueSize tracking by apapirovski · Pull Request #17650 · nodejs/node · GitHub

Uh oh!

Uh oh!

net,src: refactor writeQueueSize tracking - #17650

net,src: refactor writeQueueSize tracking#17650apapirovski wants to merge 1 commit intonodejs:masternodejs/node:masterfrom apapirovski:patch-write-queue-sizeapapirovski/node:patch-write-queue-sizeCopy head branch name to clipboard

Conversation

apapirovski commented Dec 13, 2017

Checklist

Affected core subsystem(s)

Uh oh!

apapirovski commented Dec 13, 2017 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

apapirovski commented Dec 13, 2017

Uh oh!

apapirovski commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apapirovski commented Dec 14, 2017

Uh oh!

apapirovski commented Dec 16, 2017

Uh oh!

apapirovski commented Dec 16, 2017

Uh oh!

addaleax commented Dec 16, 2017

Uh oh!

apapirovski commented Dec 18, 2017

Uh oh!

apapirovski commented Dec 18, 2017

Uh oh!

MylesBorins commented Jan 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

net,src: refactor writeQueueSize tracking#17650
apapirovski wants to merge 1 commit into
nodejs:masterfrom
apapirovski:patch-write-queue-size

apapirovski commented Dec 13, 2017 •
edited by addaleax
Loading

apapirovski commented Dec 13, 2017 •
edited
Loading

apapirovski commented Dec 13, 2017 •
edited
Loading

apapirovski commented Dec 14, 2017 •
edited
Loading