Skip to content
This repository has been archived by the owner on Aug 11, 2020. It is now read-only.

quic: QuicCID refactors and docs #286

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jan 15, 2020

There are two commits here:

  1. Makes some simple fixes and adds some docs
  2. The main commit, uses QuicCID more consistently

// similar to what we can do with TraceEvents... that would
// allow us to pass the QuicCID directly to Debug and have it
// converted to hex only if the category is enabled so we can
// skip committing resources here.
Copy link
Member

Choose a reason for hiding this comment

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

Fully agree 👍

It’s a bit tricky because that means we can’t use fprintf() directly anymore, but that’s probably pretty okay actually given that having a C++-y replacement would also help solve issues like nodejs/node#28761.

src/quic/node_quic_socket.cc Outdated Show resolved Hide resolved
src/quic/node_quic_socket.cc Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2020

btw, QuicCID currently isn't as efficient as it could be in the common case. I'll be working on that today

Updated!

@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2020

@addaleax ... added a new commit that avoids copying in QuicCID when not necessary to do so. Can you please take a look when you get a moment?

@addaleax
Copy link
Member

@jasnell Did you benchmark this? (I can also do that if you like.) My best intuition would be that the new code actually adds non-trivial overhead without much gain.

Copying an ngtcp2_cid is very cheap, on both x64 and arm it compiles down to a mere 4 instructions (https://godbolt.org/z/uhTCFQ), whereas using std::vector<...> comes with the overhead of an extra heap allocation, and it requires updating at least the same amount of memory for a copy or move operation (std::vector<> is usually 3 word-sized values itself, i.e. the management memory for a vector is about as large as a ntcp2_cid itself).

@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2020

No, didn't benchmark it yet but thinking it you're right. We're generally talking only 32 bytes. I'll drop the additional commit but keep it around in a separate branch just in case we want to move over for some reason actually, I'll keep it but I'll go back to just allocating a ngtcp2_cid struct in the class... one min and I'll have that updated.

Much of the time, QuicCID is wrapping a `const ngtcp2_cid*`, when that
is the case, avoid copying it's value and use the pointer directly.
Otherwise, allocate an internal buffer.
@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2020

Ok, take another look and let me know if that's better :-)

jasnell added a commit that referenced this pull request Jan 15, 2020
PR-URL: #286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jan 15, 2020
PR-URL: #286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit that referenced this pull request Jan 15, 2020
Much of the time, QuicCID is wrapping a `const ngtcp2_cid*`, when that
is the case, avoid copying it's value and use the pointer directly.
Otherwise, allocate an internal buffer.

PR-URL: #286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell
Copy link
Member Author

jasnell commented Jan 15, 2020

Landed!

@jasnell jasnell closed this Jan 15, 2020
jasnell added a commit to jasnell/quic that referenced this pull request Feb 3, 2020
PR-URL: nodejs#286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit to jasnell/quic that referenced this pull request Feb 3, 2020
PR-URL: nodejs#286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell added a commit to jasnell/quic that referenced this pull request Feb 3, 2020
Much of the time, QuicCID is wrapping a `const ngtcp2_cid*`, when that
is the case, avoid copying it's value and use the pointer directly.
Otherwise, allocate an internal buffer.

PR-URL: nodejs#286
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants