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.

Sign up for GitHub

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

Jump to bottom

[WIP] lib: merge onread handlers for http2 streams & net.Socket #20993

Closed
wants to merge 1 commit into from
Closed

[WIP] lib: merge onread handlers for http2 streams & net.Socket #20993

wants to merge 1 commit into from

Conversation

aks-
Copy link
Member

@aks- aks- commented May 28, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 28, 2018
mscdex
mscdex reviewed May 28, 2018
View reviewed changes
lib/net.js
@@ -513,6 +514,12 @@ Object.defineProperty(Socket.prototype, 'bufferSize', {
}
});

Object.defineProperty(Socket.prototype, kUpdateTimer, {
get: function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably avoid getters for hot paths like a socket read callback function.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn’t expect it to make a big difference? I’d expect V8 to inline this – if benchmark runs come back okay, is this still an issue?

That being said, it might be nice to move over to using kUpdateTimer in all cases.

mscdex
mscdex reviewed May 28, 2018
View reviewed changes
lib/internal/stream_base_commons.js
@@ -81,8 +81,40 @@ function afterWriteDispatched(self, req, err, cb) {
}
}

function _onread(handle, stream, nread, buf) {
stream[kUpdateTimer]();
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this deviating from http2's original implementation? Previously for http2 this was only called inside the conditional below.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think that’s an issue either, and arguably a bit more correct this way.

mscdex
mscdex reviewed May 28, 2018
View reviewed changes
lib/internal/http2/core.js
}

// Last chunk was received. End the readable side.
debug(`Http2Stream ${stream[kID]} [Http2Session ` +
Copy link
Contributor

Choose a reason for hiding this comment

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

This is missing in the new implementation

@apapirovski
Copy link
Contributor

apapirovski commented May 28, 2018 •
edited
Loading

I'm not sure how feasible it is to do a proper job of merging these. HTTP2 has pretty different requirements here. The bits that are shared are actually quite minimal (basically the first if and even then there's extra debugging calls & properties that are not shared).

mscdex
mscdex reviewed May 28, 2018
View reviewed changes
lib/internal/stream_base_commons.js
function _onread(handle, stream, nread, buf) {
stream[kUpdateTimer]();

if (nread > 0 && !stream.destroyed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the same as the previous http2 implementation, which executed the logic within this block when nread >= 0, not just nread > 0.

Copy link
Member

Choose a reason for hiding this comment

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

I don’t think omitting 0-sized reads is an issue.

@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label May 28, 2018
@mscdex
Copy link
Contributor

mscdex commented May 28, 2018

I agree, there seems to be subtle differences that would make merging difficult, especially efficiently.

addaleax
addaleax reviewed Jun 2, 2018
View reviewed changes
lib/internal/http2/core.js
@@ -2122,6 +2102,7 @@ function afterOpen(session, options, headers, streamOptions, err, fd) {
class ServerHttp2Stream extends Http2Stream {
constructor(session, handle, id, options, headers) {
super(session, options);
handle.owner = this;
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be better to attach [kOwner] in net.js (and later switching over to always using that symbol over there)

Generally, it would imo be a good idea to have a standardized way to access the JS wrapper object from a C++ handle, and handle[kOwner] might be exactly that

@addaleax
Copy link
Member

addaleax commented Jun 2, 2018

@aks- Thanks for doing this! I’m sorry I didn’t reply sooner, feel free to @ me if you like. Could you rebase this against master? I’ll try to look into the test failures (if they are still there)

mscdex
mscdex suggested changes Jun 2, 2018
View reviewed changes
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

I think having benchmark results would be good considering some of the changes being made here.

@addaleax
Copy link
Member

addaleax commented Jun 5, 2018

@ryzokuken Do you maybe have the time to look at this?

@ryzokuken
Copy link
Contributor

@addaleax will take a look tonight, sure!

@addaleax addaleax mentioned this pull request Aug 21, 2018
Closed
2 tasks
@addaleax
Copy link
Member

Got an updated version in #22449

addaleax added a commit to addaleax/node that referenced this pull request Aug 27, 2018
@aks- @addaleax
lib: merge onread handlers for http2 streams & net.Socket
9c1443b 
Refs: nodejs#20993
Co-authored-by: Anna Henningsen <anna@addaleax.net>
@addaleax
Copy link
Member

addaleax commented Sep 2, 2018

Done!

@addaleax addaleax closed this Sep 2, 2018
addaleax pushed a commit that referenced this pull request Sep 2, 2018
@aks- @addaleax
lib: merge onread handlers for http2 streams & net.Socket
1126dfe 
Refs: #20993
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #22449
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Sep 2, 2018
@aks- @targos
lib: merge onread handlers for http2 streams & net.Socket
45a6108 
Refs: #20993
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #22449
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@aks- @targos
lib: merge onread handlers for http2 streams & net.Socket
f0be053 
Refs: #20993
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #22449
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
@aks- @targos
lib: merge onread handlers for http2 streams & net.Socket
2a88731 
Refs: #20993
Co-authored-by: Anna Henningsen <anna@addaleax.net>

PR-URL: #22449
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@mscdex mscdex mscdex requested changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aks- @apapirovski @mscdex @addaleax @ryzokuken @nodejs-github-bot