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,http2: refactor _write and _writev in net.Socket and http2.Http2Stream #20643

Closed
wants to merge 1 commit into from

Conversation

ryzokuken
Copy link
Contributor

@ryzokuken ryzokuken commented May 9, 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

Roadmap

  • Remove inconsistencies between http2.Http2Stream._write and http2.Http2Stream._writev
  • Move all logic into a new method http2.Http2Stream._writeGeneric
  • Remove inconsistencies between net.Socket._writeGeneric and http2.Http2Stream._writeGeneric
  • Refactor common logic into a method in the base class/method, possibly writeGeneric from internal/stream_base_commons.

/cc @addaleax @nodejs/http2 @nodejs/net

@ryzokuken ryzokuken self-assigned this May 9, 2018
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels May 9, 2018
@addaleax addaleax added wip Issues and PRs that are still a work in progress. net Issues and PRs related to the net subsystem. labels May 9, 2018
@mcollina
Copy link
Member

mcollina commented May 9, 2018

I'm concerned about making the writeGeneric having two different signatures (with and without the encoding). Supporting two different signature would force V8 to introduce a trampoline, which will slow things down as that is one of the hottest function in core

That logic needs tidy up anyway, so let's find a way to keep the number of arguments consistent.

@ryzokuken
Copy link
Contributor Author

@mcollina You mean _writeGeneric? I tried to keep the signature exactly the same as the one that already existed in net.Socket for consistency and also because I thought that everyone is onboard with that pattern. If net.Socket's _writeGeneric seems like an anti-pattern, I could try to do better but it does seem good enough given that it's actually taking the same arguments just that the encoding variable is set to an empty string.

That said, please share whatever you have in mind and hopefully we would be able to make it a tad faster and simpler.

@mcollina
Copy link
Member

My bad, I misread a writeGeneric call with a writevGeneric.

@ryzokuken
Copy link
Contributor Author

That's what I did the first time around too 😅, but yeah. They're different functions, so I hope everything is fine, right? Should I proceed?

@ryzokuken
Copy link
Contributor Author

ryzokuken commented May 10, 2018

A few questions:

  1. Why are we calling this._writeGeneric.bind(this, writev, data, encoding, cb) and not () => this._writeGeneric(writev, data, encoding, cb) in Http2Stream._writeGeneric unlike Socket._writeGeneric? Because we're calling this's _writeGeneric, won't this retain the same value? Why did we explicitly bind this? Am I missing something?

-- more to come --

@ryzokuken
Copy link
Contributor Author

okay, tests pass so I'm keeping it here until someone asks me to remove it.

One more thing:

Why does Socket._writeGeneric has:

this.once('connect', function connect() {
  this._writeGeneric(writev, data, encoding, cb);
});

even though normal functions have their own this? Does this depend on that behavior, or should an arrow function suffice? Let me try it out.

@ryzokuken
Copy link
Contributor Author

Tests pass again, I'm either making progress, or we have weak test cases. Two things next:

  1. The _unrefTimer call in Socket comes before the destroyed check, as opposed to Http2Stream, which checks first. I believe we should go with the http2 way, trying this.

  2. In the Socket check, the socket is being destroyed (this.destroy(new ERR_SOCKET_CLOSED(), cb), but the Http2Stream version just drops the data on the floor and hopes for the best. This might take more time getting streamlined.

@ryzokuken
Copy link
Contributor Author

Okay, when it comes to the actual logic for refreshing timers, they're both quite similar.

In net.Socket, we have a method called _unrefTimer which goes like:

Socket.prototype._unrefTimer = function _unrefTimer() {
  for (var s = this; s !== null; s = s._parent) {
    if (s[kTimeout])
      s[kTimeout][refreshFnSymbol]();
  }
};

and in http2.Http2Stream, we just call this[kUpdateTimer](), which goes like:

  [kUpdateTimer]() {
    if (this.destroyed)
      return;
    if (this[kTimeout])
      this[kTimeout][refreshFnSymbol]();
    if (this[kSession])
      this[kSession][kUpdateTimer]();
  }

for now, the least I think I could do is switch from the iterative approach in Socket to the recursive approach as in Http2Stream. After that, perhaps the code could be refactored to be a bit more similar?

@ryzokuken
Copy link
Contributor Author

Okay, it wasn't recursing, but rather refreshing the associated session. That said, the code still looks a tad similar this way. Should I keep it in?

@ryzokuken ryzokuken force-pushed the refactor-stream-write branch 2 times, most recently from 1732c4f to 1257d56 Compare May 10, 2018 20:36
@ryzokuken
Copy link
Contributor Author

ryzokuken commented May 10, 2018

Current roadblocks (inconsistencies):

  • this._pendingData and this._pendingEncoding.
  • Explicitly destroying the Socket using this.destroy(new ERR_SOCKET_CLOSED(), cb)
  • if (!this.headersSent) this[kProceed](); in Http2Stream

@ryzokuken
Copy link
Contributor Author

Ping @mcollina @addaleax

@addaleax
Copy link
Member

this._pendingData and this._pendingEncoding.

Maybe something that we also want in HTTP/2?

Anyway, this LGTM as is and I’d be okay with landing this as-is and iterating onwards from there.

@apapirovski
Copy link
Member

I haven't had a chance to fully review but 2 things off the top:

  • _writeGeneric shouldn't just be exposed on http2 like that. It should either be a helper function or on a Symbol.
  • I'm not sure how I feel about changing how timeouts are handled in net. That code is performance sensitive, it runs every single time that any action happens on the socket.

@ryzokuken
Copy link
Contributor Author

@addaleax

Maybe something that we also want in HTTP/2?

While we may not need the properties yet, I believe they could prove helpful while fleshing out future behavior. Anyway, let me know if the added cruft of two simple properties would be worth a little consistency and nifty properties that could be used in the future (or perhaps even right now).

@apapirovski

_writeGeneric shouldn't just be exposed on http2 like that. It should either be a helper function or on a Symbol.

While I think I could just move to a Symbol pretty easily, I don't think it should be a problem because:

  1. _writeGeneric is also exposed for net.Socket?
  2. _write and _writev are exposed for http2.Http2Stream anyway?

That said, if you insist, I will make that change.

I'm not sure how I feel about changing how timeouts are handled in net.

Again, I made the change only to make things a tad more consistent, and you have a much better idea that me how this could affect speed in the long term. If you say, I could revert that particular part back to the original.

@apapirovski
Copy link
Member

While I think I could just move to a Symbol pretty easily, I don't think it should be a problem because:

The fact that _writeGeneric is exposed on the Socket is just bad behaviour we're stuck with — it comes from a time when we didn't have many options for hiding private properties and methods. Now we do and since it's a private implementation detail, it shouldn't leak to the user. As for _write and _writev, they are a part of the streams contract.

Again, I made the change only to make things a tad more consistent, and you have a much better idea that me how this could affect speed in the long term. If you say, I could revert that particular part back to the original.

I don't think things need to be more consistent between those two functions. I get the point of making _write and _writev more consistent, I don't think any unrelated functions need to be.

As far as how it's slower: calling a function (adding it to the stack) is slower than just a loop and since it's now recursive there's no way it can get inlined by V8.

@ryzokuken
Copy link
Contributor Author

As far as how it's slower: calling a function (adding it to the stack) is slower than just a loop and since it's now recursive there's no way it can get inlined by V8.

Get it, loop would definitely be faster. That said, I'm thinking: as long as the two classes each have an exposed function (maybe called _refreshTimer) which could be called from _writeGeneric, it won't really matter how it's implemented. Therefore, reverting the function back to original and adding a _refreshTimer function for http2 seems appropriate.

The fact that _writeGeneric is exposed on the Socket is just bad behaviour we're stuck with

It seems that'll no longer be a problem once I'm done with this (same with it being exposed on Http2Stream), because the final goal of the PR would be to move the function to a base class.

@apapirovski
Copy link
Member

Keep in mind that there would be the same issue with exposing _refreshTimer as with exposing _writeGeneric.

@ryzokuken
Copy link
Contributor Author

ryzokuken commented May 15, 2018

@apapirovski could we not expose it but still call it from a base class, somehow?

@ryzokuken
Copy link
Contributor Author

@apapirovski @addaleax This looks better, I suppose?

@apapirovski
Copy link
Member

LGTM. Could also be a separate function to avoid the symbol lookup when calling it but this is fine as the first version.

_unrefTimer still needs to be reverted. Then we could likely land this and iterate from there.

@ryzokuken
Copy link
Contributor Author

@apapirovski reverting _unrefTimer in a second.

@ryzokuken
Copy link
Contributor Author

@BridgeAR okay! In that case, this is ready to merge.

Awaiting @apapirovski's explicit approval before landing.

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this!

@ryzokuken
Copy link
Contributor Author

@apapirovski Thanks for all the constructive feedback and guidance! Will land this today.

@ryzokuken
Copy link
Contributor Author

Landing this.

Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.
@ryzokuken
Copy link
Contributor Author

Squashing and running CI, will land in the morning.

@ryzokuken
Copy link
Contributor Author

@ryzokuken
Copy link
Contributor Author

Landed in a7fa0db

@ryzokuken ryzokuken closed this May 19, 2018
ryzokuken added a commit that referenced this pull request May 19, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

PR-URL: #20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

PR-URL: #20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax mentioned this pull request May 22, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

PR-URL: nodejs#20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

PR-URL: nodejs#20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

PR-URL: nodejs#20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Refactor writable part (the _write and _writev functions) in net.Socket
and http2.Http2Stream classes.
Also involves adding a generic "WriteGeneric" method to the Http2Stream
class based on net.Socket._writeGeneric, but behind a symbol.

Backport-PR-URL: #22850
PR-URL: #20643
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants