Skip to content

Commit

Permalink
quic: avoid memory fragmentation issue
Browse files Browse the repository at this point in the history
Original PR: nodejs/quic#388

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.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #33912
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
  • Loading branch information
jasnell committed Jun 18, 2020
1 parent 16116f5 commit d7d79f2
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 14 deletions.
5 changes: 3 additions & 2 deletions src/quic/node_quic_socket-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ namespace quic {
std::unique_ptr<QuicPacket> QuicPacket::Create(
const char* diagnostic_label,
size_t len) {
CHECK_LE(len, MAX_PKTLEN);
return std::make_unique<QuicPacket>(diagnostic_label, len);
}

Expand All @@ -27,8 +28,8 @@ std::unique_ptr<QuicPacket> QuicPacket::Copy(
}

void QuicPacket::set_length(size_t len) {
CHECK_LE(len, data_.size());
data_.resize(len);
CHECK_LE(len, MAX_PKTLEN);
len_ = len;
}

int QuicEndpoint::Send(
Expand Down
13 changes: 6 additions & 7 deletions src/quic/node_quic_socket.cc
Original file line number Diff line number Diff line change
Expand Up @@ -83,24 +83,23 @@ bool IsShortHeader(
} // namespace

QuicPacket::QuicPacket(const char* diagnostic_label, size_t len) :
data_(len),
data_{0},
len_(len),
diagnostic_label_(diagnostic_label) {
CHECK_LE(len, NGTCP2_MAX_PKT_SIZE);
CHECK_LE(len, MAX_PKTLEN);
}

QuicPacket::QuicPacket(const QuicPacket& other) :
QuicPacket(other.diagnostic_label_, other.data_.size()) {
memcpy(data_.data(), other.data_.data(), other.data_.size());
QuicPacket(other.diagnostic_label_, other.len_) {
memcpy(&data_, &other.data_, other.len_);
}

const char* QuicPacket::diagnostic_label() const {
return diagnostic_label_ != nullptr ?
diagnostic_label_ : "unspecified";
}

void QuicPacket::MemoryInfo(MemoryTracker* tracker) const {
tracker->TrackField("data", data_);
}
void QuicPacket::MemoryInfo(MemoryTracker* tracker) const {}

QuicSocketListener::~QuicSocketListener() {
if (socket_)
Expand Down
18 changes: 13 additions & 5 deletions src/quic/node_quic_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,13 @@ class JSQuicSocketListener : public QuicSocketListener {
void OnDestroy() override;
};

// This is just a formality as the QUIC spec normatively
// defines that the ipv4 max pktlen is always going to be
// larger than the ipv6 max pktlen, but in the off chance
// ever changes (which is unlikely) we check here.
constexpr size_t MAX_PKTLEN =
std::max<size_t>(NGTCP2_MAX_PKTLEN_IPV4, NGTCP2_MAX_PKTLEN_IPV6);

// A serialized QuicPacket to be sent by a QuicSocket instance.
class QuicPacket : public MemoryRetainer {
public:
Expand Down Expand Up @@ -141,7 +148,7 @@ class QuicPacket : public MemoryRetainer {
// QuicPacket instance will be freed.
static inline std::unique_ptr<QuicPacket> Create(
const char* diagnostic_label = nullptr,
size_t len = NGTCP2_MAX_PKTLEN_IPV4);
size_t len = MAX_PKTLEN);

// Copy the data of the QuicPacket to a new one. Currently,
// this is only used when retransmitting close connection
Expand All @@ -151,11 +158,11 @@ class QuicPacket : public MemoryRetainer {

QuicPacket(const char* diagnostic_label, size_t len);
QuicPacket(const QuicPacket& other);
uint8_t* data() { return data_.data(); }
size_t length() const { return data_.size(); }
uint8_t* data() { return data_; }
size_t length() const { return len_; }
uv_buf_t buf() const {
return uv_buf_init(
const_cast<char*>(reinterpret_cast<const char*>(data_.data())),
const_cast<char*>(reinterpret_cast<const char*>(&data_)),
length());
}
inline void set_length(size_t len);
Expand All @@ -166,7 +173,8 @@ class QuicPacket : public MemoryRetainer {
SET_SELF_SIZE(QuicPacket);

private:
std::vector<uint8_t> data_;
uint8_t data_[MAX_PKTLEN];
size_t len_ = MAX_PKTLEN;
const char* diagnostic_label_ = nullptr;
};

Expand Down

0 comments on commit d7d79f2

Please sign in to comment.