quic: hopefully avoid memory fragmentation issue

[jasnell]

@splitice ... I'm hoping this provides a reasonable fix for #386

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.

This is the most likely culprit in the current implementation so let me know if this patch fixes it. Will keep this open until I get confirmation from you one way or the other.

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

[splitice]

Thanks @jasnell. I've cherry-picked built and installed it. I'll aim to leave the hardware running with glibc malloc and your commit over the weekend for report on Monday.

[addaleax]

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.

@jasnell You’re 100 % correct here – 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?

[jasnell]

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?

The QuicPacket instances are transient and exist only as long as it takes to serialize the complete the UDP send, then they are freed as quickly as possible after the udp send completes. I want to refactor this a bit more to allow us to size them more explicitly when we know we aren't going to need the full 1252 bytes. Because of coalescing requirements, we don't know in advance how much of the packet ngtcp2 is going to use every time we ask it to serialize.

[splitice]

@jasnell This seems to work well. Currently sitting at 55MB after 3 days.

[lundibundi]

The PR message talks about using NGTCP2_MAX_PKT_SIZE but new code uses NGTCP2_MAX_PKTLEN_IPV4 is that correct? (note that the check in constructor QuicPacket::QuicPacket(const char* diagnostic_label, size_t len) still checks for NGTCP2_MAX_PKT_SIZE)

Also, NGTCP2_MAX_PKT_SIZE recently changed to NGTCP2_DEFAULT_MAX_UDP_PAYLOAD_SIZE (ngtcp2/ngtcp2@38feb91)

[jasnell]

I'll double check on that. May have mentally mixed those up

[jasnell]

This seems to work well. Currently sitting at 55MB after 3 days.

Fantastic. Will get this updated in the next day or two and get it landed.

[jasnell]

Closing in favor of nodejs/node#33912