FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[WIP] lib: merge onread handlers for http2 streams & net.Socket by aks- · Pull Request #20993 · nodejs/node · GitHub

/ node Public

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

Closed
aks- wants to merge 1 commit into
nodejs:masterfrom
aks-:refactor-net-http-module
ConversationCommits (1)ChecksFiles changed
Closed

[WIP] lib: merge onread handlers for http2 streams & net.Socket#20993
aks- wants to merge 1 commit into
nodejs:masterfrom
aks-:refactor-net-http-module

Conversation

aks- commented May 28, 2018

Copy link
Copy Markdown
Member
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 added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 28, 2018
mscdex reviewed May 28, 2018
View reviewed changes
Comment thread lib/net.js
});

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

mscdex May 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Jun 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 reviewed May 28, 2018
View reviewed changes
Comment thread lib/internal/stream_base_commons.js
}

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

mscdex May 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Jun 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

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

mscdex May 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is missing in the new implementation

apapirovski commented May 28, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed May 28, 2018
View reviewed changes
Comment thread lib/internal/stream_base_commons.js
function _onread(handle, stream, nread, buf) {
stream[kUpdateTimer]();

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

mscdex May 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax Jun 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

mscdex commented May 28, 2018

Copy link
Copy Markdown
Contributor

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

addaleax reviewed Jun 2, 2018
View reviewed changes
Comment thread lib/internal/http2/core.js
class ServerHttp2Stream extends Http2Stream {
constructor(session, handle, id, options, headers) {
super(session, options);
handle.owner = this;

addaleax Jun 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Jun 2, 2018

Copy link
Copy Markdown
Member

@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 suggested changes Jun 2, 2018
View reviewed changes

mscdex left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

addaleax commented Jun 5, 2018

Copy link
Copy Markdown
Member

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

ryzokuken commented Jun 5, 2018

Copy link
Copy Markdown
Contributor

@addaleax will take a look tonight, sure!

addaleax mentioned this pull request Aug 21, 2018
Closed
2 tasks

addaleax commented Aug 21, 2018

Copy link
Copy Markdown
Member

Got an updated version in #22449

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

addaleax commented Sep 2, 2018

Copy link
Copy Markdown
Member

Done!

addaleax closed this Sep 2, 2018
addaleax pushed a commit that referenced this pull request Sep 2, 2018
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
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
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax left review comments
+1 more reviewer
mscdex mscdex requested changes
Reviewers whose approvals may not affect merge requirements

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

Back | FazBrowse Home | New Git URL