[WIP] lib: merge onread handlers for http2 streams & net.Socket#20993
[WIP] lib: merge onread handlers for http2 streams & net.Socket#20993aks- wants to merge 1 commit intonodejs:masterfrom
Conversation
nodejs-github-bot
added
the
lib / src
| }); | ||
|
|
||
| Object.defineProperty(Socket.prototype, kUpdateTimer, { | ||
| get: function() { |
There was a problem hiding this comment.
We should probably avoid getters for hot paths like a socket read callback function.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| function _onread(handle, stream, nread, buf) { | ||
| stream[kUpdateTimer](); |
There was a problem hiding this comment.
Isn't this deviating from http2's original implementation? Previously for http2 this was only called inside the conditional below.
There was a problem hiding this comment.
I don’t think that’s an issue either, and arguably a bit more correct this way.
| } | ||
|
|
||
| // Last chunk was received. End the readable side. | ||
| debug(`Http2Stream ${stream[kID]} [Http2Session ` + |
There was a problem hiding this comment.
This is missing in the new implementation
|
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 |
| function _onread(handle, stream, nread, buf) { | ||
| stream[kUpdateTimer](); | ||
|
|
||
| if (nread > 0 && !stream.destroyed) { |
There was a problem hiding this comment.
This is not the same as the previous http2 implementation, which executed the logic within this block when nread >= 0, not just nread > 0.
There was a problem hiding this comment.
I don’t think omitting 0-sized reads is an issue.
|
I agree, there seems to be subtle differences that would make merging difficult, especially efficiently. |
| class ServerHttp2Stream extends Http2Stream { | ||
| constructor(session, handle, id, options, headers) { | ||
| super(session, options); | ||
| handle.owner = this; |
There was a problem hiding this comment.
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
|
@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 |
mscdex
left a comment
mscdex
left a comment
There was a problem hiding this comment.
I think having benchmark results would be good considering some of the changes being made here.
|
@ryzokuken Do you maybe have the time to look at this? |
|
@addaleax will take a look tonight, sure! |
|
Got an updated version in #22449 |
Refs: nodejs#20993 Co-authored-by: Anna Henningsen <anna@addaleax.net>
|
Done! |
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes