Skip to content
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

net,src: refactor writeQueueSize tracking #17650

Closed

Conversation

apapirovski
Copy link
Member

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

@apapirovski 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 apapirovski requested a review from addaleax December 13, 2017 14:25
@nodejs-github-bot 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
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@apapirovski
Copy link
Member Author

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

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

apapirovski commented Dec 13, 2017

@apapirovski
Copy link
Member Author

apapirovski commented Dec 13, 2017

@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.

lib/net.js Outdated
@@ -870,7 +870,8 @@ function afterWrite(status, handle, req, err) {
if (self !== process.stderr && self !== process.stdout)
debug('afterWrite', status);

self[kLastWriteQueueSize] = 0;
if (req.async)
self[kLastWriteQueueSize] = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -162,6 +162,7 @@ class StreamResource {
uv_buf_t* bufs,
size_t count,
uv_stream_t* send_handle) = 0;
virtual bool IsAsync();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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…

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apapirovski Maybe make it HasWriteQueue()?

@apapirovski apapirovski force-pushed the patch-write-queue-size branch from 5938061 to c81d08a Compare December 14, 2017 03:41
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 14, 2017
@addaleax
Copy link
Member

@apapirovski
Copy link
Member Author

apapirovski commented Dec 14, 2017

@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.

@apapirovski
Copy link
Member Author

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

@apapirovski apapirovski force-pushed the patch-write-queue-size branch 4 times, most recently from 88dc4f1 to 23f0f3f Compare December 15, 2017 14:34
@apapirovski apapirovski force-pushed the patch-write-queue-size branch 2 times, most recently from 0d1c759 to 586dadb Compare December 16, 2017 21:20
@apapirovski
Copy link
Member Author

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.

@apapirovski
Copy link
Member Author

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

@addaleax
Copy link
Member

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/

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 16, 2017
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.
@apapirovski apapirovski force-pushed the patch-write-queue-size branch from 586dadb to a99b447 Compare December 18, 2017 14:14
@apapirovski
Copy link
Member Author

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

(Rebased after _writableState.length removal landed.)

@apapirovski apapirovski added the baking-for-lts PRs that need to wait before landing in a LTS release. label Dec 18, 2017
apapirovski added a commit that referenced this pull request Dec 18, 2017
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>
@apapirovski
Copy link
Member Author

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.

@apapirovski apapirovski deleted the patch-write-queue-size branch December 18, 2017 15:00
@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 19, 2017
@MylesBorins
Copy link
Contributor

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

@addaleax addaleax mentioned this pull request Jan 10, 2018
2 tasks
addaleax pushed a commit to addaleax/node that referenced this pull request Jan 10, 2018
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
boingoing pushed a commit to nodejs/node-chakracore that referenced this pull request Jan 18, 2018
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>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2018
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>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
@ronag ronag mentioned this pull request Jun 27, 2020
4 tasks
@targos targos removed the baking-for-lts PRs that need to wait before landing in a LTS release. label Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants