net,http2: refactor _write and _writev in net.Socket and http2.Http2Stream

[ryzokuken]

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

[mcollina]

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]

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

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

[ryzokuken]

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]

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]

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]

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]

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]

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]

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]

Ping @mcollina @addaleax

[addaleax]

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]

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]

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

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]

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]

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

[ryzokuken]

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

[ryzokuken]

@apapirovski @addaleax This looks better, I suppose?

[apapirovski]

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]

@apapirovski reverting _unrefTimer in a second.

[ryzokuken]

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

Awaiting @apapirovski's explicit approval before landing.

[apapirovski]

LGTM. Thanks for working on this!

[ryzokuken]

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

[ryzokuken]

Landing this.

[ryzokuken]

Squashing and running CI, will land in the morning.

[ryzokuken]

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

[ryzokuken]

Landed in a7fa0db