quic: fix memory fragmentation

[jasnell]

Previously, QuicPacket was allocating an std::vector<uint8_t> of NGTCP2_MAX_PKT_SIZE bytes, then the packet would be serialized into the buffer, and the std::vector would be resized based on the number of bytes serialized. I suspect the memory fragmentation that you're seeing is because of those resize operations not freeing memory in chunks that are aligned with the allocation. This changes QuicPacket to use a stack allocation that is always NGTCP2_MAX_PKT_SIZE bytes and the size of the serialized packet is just recorded without any resizing. When the memory is freed now, it should be freed in large enough chunks to cover subsequent allocations.

Original PR: nodejs/quic#388

cc: @nodejs/quic

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

[addaleax]

I’ll echo this comment (nodejs/quic#388 (comment)):

std::vector::resize() never frees memory. If that is the expectation here, you should call std::vector::shrink_to_fit() after it. I suspect that that would be the right fix here if that’s what’s causing the problems… this PR in its current state should only ever increase memory usage because it makes QuickPacket never use less than the maximum amount of data?

i.e. I think this PR should not change the situation at all, and a call to .shrink_to_fit() is what’s missing here.

[jasnell]

I can convert this to use shrink to fit but the change here does appear to fix the originally reported problem: nodejs/quic#388 (comment) ... specifically, I think we can actually get a better result here by avoiding the use of the vector entirely.

[jasnell]

Let's see how CI goes across all platforms... ;-) https://ci.nodejs.org/job/node-test-commit/39059/

[mcollina]

lgtm

[nodejs-github-bot]

Normal (non-QUIC) CI: https://ci.nodejs.org/job/node-test-pull-request/31924/

[nodejs-github-bot]

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

[jasnell]

Landed in d7d79f2

[addaleax]

I can convert this to use shrink to fit but the change here does appear to fix the originally reported problem: nodejs/quic#388 (comment) ... specifically, I think we can actually get a better result here by avoiding the use of the vector entirely.

@jasnell Okay, do you mind if I open a PR that does shrink memory? Because I don’t think unconditionally allocating the maximum packet length is an acceptable solution, tbh.

[jasnell]

What I've found in testing is either (a) we are sending very small generally fixed sized packets or (b) we are sending very close to full sized packets, there's rarely anything in between and we generally know when we're going to send either. What I think I'd like to see first is: when we know we're going to send a smaller packet size, we just create the QuicPacket with the smaller size so we can avoid resizing it at all.

The other thing I've been considering is have a ring buffer of QuicPacket instances that we can just reuse. Grab one, serialize into it, send it, put it back on the queue when it's down to be reused so that we can avoid the extra allocations almost entirely.

That said, you know I never mind a PR :-)

[addaleax]

Grab one, serialize into it, send it, put it back on the queue when it's down to be reused so that we can avoid the extra allocations almost entirely.

To be clear, my concern wasn’t so much the time spent on allocations, but more the fact that these packets now keep memory usage up for a potentially long time.

[jasnell]

QuicPacket objects should only exist long enough to receive the serialized frames from ngtcp2 and be sent by the UDP wrap. They should not be long lived at all, and if they are, there's another problem to be fixed, as they should be freed as soon as libuv signals that the UDP packet has been sent. Before making further changes here, let's do some profiling to see how long these are kept alive.

[addaleax]

Ah yeah, if they aren’t kept alive until an ACK from the receiving side I think that should be okay :)

[jasnell]

Yeah, the acks only impact the buffer at the pre-serialized state. The QuicPacket only ever contains serialized frames which are always transient. That's why I'm thinking a ring buffer may be beneficial here.