src: minor quic packet cleanups

Author: jasnell

src/quic/bindingdata.h

@@ -11,8 +11,8 @@
 #include <node.h>
 #include <node_mem.h>
 #include <v8.h>
+#include <list>
 #include <unordered_map>
-#include <vector>
 #include "defs.h"
 
 namespace node::quic {
@@ -172,7 +172,7 @@ class BindingData final
   // Purge the packet free list to free up memory.
   JS_METHOD(FlushPacketFreelist);
 
-  std::vector<BaseObjectPtr<BaseObject>> packet_freelist;
+  std::list<BaseObjectPtr<BaseObject>> packet_freelist;
 
   std::unordered_map<Endpoint*, BaseObjectPtr<BaseObject>> listening_endpoints;
 

src/quic/packet.cc

@@ -120,6 +120,8 @@ BaseObjectPtr<Packet> Packet::Create(Environment* env,
 }
 
 BaseObjectPtr<Packet> Packet::Clone() const {
+  // Cloning is copy-free. Our data_ is a shared_ptr so we can just
+  // share it with the cloned packet.
   auto& binding = BindingData::Get(env());
   if (binding.packet_freelist.empty()) {
     JS_NEW_INSTANCE_OR_RETURN(env(), obj, {});
@@ -135,8 +137,8 @@ BaseObjectPtr<Packet> Packet::FromFreeList(Environment* env,
                                            const SocketAddress& destination) {
   auto& binding = BindingData::Get(env);
   if (binding.packet_freelist.empty()) return {};
-  auto obj = binding.packet_freelist.back();
-  binding.packet_freelist.pop_back();
+  auto obj = binding.packet_freelist.front();
+  binding.packet_freelist.pop_front();
   CHECK(obj);
   CHECK_EQ(env, obj->env());
   auto packet = BaseObjectPtr<Packet>(static_cast<Packet*>(obj.get()));
@@ -185,6 +187,8 @@ void Packet::Done(int status) {
   // big, we don't want to accumulate these things forever.
   auto& binding = BindingData::Get(env());
   if (binding.packet_freelist.size() >= kMaxFreeList) {
+    Debug(this, "Freelist full, destroying packet");
+    data_.reset();
     return;
   }
 
@@ -195,14 +199,18 @@ void Packet::Done(int status) {
   binding.packet_freelist.push_back(std::move(self));
 }
 
+Packet::operator bool() const {
+  return data_ != nullptr;
+}
+
 std::string Packet::ToString() const {
   if (!data_) return "Packet (<empty>)";
   return "Packet (" + data_->ToString() + ")";
 }
 
 void Packet::MemoryInfo(MemoryTracker* tracker) const {
   tracker->TrackField("destination", destination_);
-  tracker->TrackField("data", data_);
+  if (data_) tracker->TrackField("data", data_);
 }
 
 BaseObjectPtr<Packet> Packet::CreateRetryPacket(

src/quic/packet.h

@@ -28,22 +28,23 @@ struct PathDescriptor final {
   std::string ToString() const;
 };
 
-// A Packet encapsulates serialized outbound QUIC data.
-// Packets must never be larger than the path MTU. The
-// default QUIC packet maximum length is 1200 bytes,
-// which we assume by default. The packet storage will
-// be stack allocated up to this size.
+// A Packet encapsulates serialized outbound QUIC data. Packets must never be
+// larger than the path MTU. The default QUIC packet maximum length is 1200
+// bytes, which we assume by default. The packet storage will be stack allocated
+// up to this size.
 //
-// Packets are maintained in a freelist held by the
-// BindingData instance. When using Create() to create
-// a Packet, we'll check to see if there is a free
-// packet in the freelist and use it instead of starting
-// fresh with a new packet. The freelist can store at
-// most kMaxFreeList packets
+// Packets are maintained in a freelist held by the BindingData instance. When
+// using Create() to create a Packet, we'll check to see if there is a free
+// packet in the freelist and use it instead of starting fresh with a new
+// packet. The freelist can store at most kMaxFreeList packets. This is a
+// performance optimization to avoid excessive allocation churn when creating
+// lots of packets since each one is ReqWrap and has a fair amount of associated
+// overhead. However, we don't want to accumulate too many of these in the
+// freelist either, so we cap the size.
 //
-// Packets are always encrypted so their content should
-// be considered opaque to us. We leave it entirely up
-// to ngtcp2 how to encode QUIC frames into the packet.
+// Packets are always encrypted so their content should be considered opaque
+// to us. We leave it entirely up to ngtcp2 how to encode QUIC frames into
+// the packet.
 class Packet final : public ReqWrap<uv_udp_send_t> {
  private:
   struct Data;
@@ -56,11 +57,9 @@ class Packet final : public ReqWrap<uv_udp_send_t> {
     virtual void PacketDone(int status) = 0;
   };
 
-  // Do not use the Packet constructors directly to create
-  // them. These are public only to support MakeBaseObject.
-  // Use the Create, or Create variants to create or
-  // acquire packet instances.
-
+  // Do not use the Packet constructors directly to create them. These are
+  // public only to support MakeBaseObject. Use the Create, or Create variants
+  // to create or acquire packet instances.
   Packet(Environment* env,
          Listener* listener,
          v8::Local<v8::Object> object,
@@ -80,13 +79,17 @@ class Packet final : public ReqWrap<uv_udp_send_t> {
   size_t length() const;
   operator uv_buf_t() const;
   operator ngtcp2_vec() const;
+  operator bool() const;
 
   // Modify the size of the packet after ngtcp2 has written
   // to it. len must be <= length(). We call this after we've
   // asked ngtcp2 to encode frames into the packet and ngtcp2
   // tells us how many of the packets bytes were used.
   void Truncate(size_t len);
 
+  // Create (or acquire from the freelist) a Packet with the given
+  // destination and length. The diagnostic_label is used to help
+  // identify the packet purpose in debugging output.
   static BaseObjectPtr<Packet> Create(
       Environment* env,
       Listener* listener,