Pre-reserve memory and avoid off-by-one in queue length

dc/s2n-quic-dc/src/path/secret/map/state.rs

@@ -34,6 +34,13 @@ pub(crate) struct PeerMap(
 pub(crate) struct IdMap(Mutex<hashbrown::HashTable<Arc<Entry>>>);
 
 impl PeerMap {
+    fn reserve(&self, additional: usize) {
+        self.0
+            .lock()
+            .unwrap_or_else(|e| e.into_inner())
+            .reserve(additional, |e| self.hash(e));
+    }
+
     fn hash(&self, entry: &Entry) -> u64 {
         self.hash_key(entry.peer())
     }
@@ -87,6 +94,13 @@ impl PeerMap {
 }
 
 impl IdMap {
+    fn reserve(&self, additional: usize) {
+        self.0
+            .lock()
+            .unwrap_or_else(|e| e.into_inner())
+            .reserve(additional, |e| self.hash(e));
+    }
+
     fn hash(&self, entry: &Entry) -> u64 {
         self.hash_key(entry.id())
     }
@@ -137,24 +151,6 @@ impl IdMap {
     }
 }
 
-// # Managing memory consumption
-//
-// For regular rotation with live peers, we retain at most two secrets: one derived from the most
-// recent locally initiated handshake and the most recent remote initiated handshake (from our
-// perspective). We guarantee that at most one handshake is ongoing for a given peer pair at a
-// time, so both sides will have at least one mutually trusted entry after the handshake. If a peer
-// is only acting as a client or only as a server, then one of the peer maps will always be empty.
-//
-// Previous entries can safely be removed after a grace period (EVICTION_TIME). EVICTION_TIME
-// is only needed because a stream/datagram might be opening/sent concurrently with the new
-// handshake (e.g., during regular rotation), and we don't want that to fail spuriously.
-//
-// We also need to manage secrets for no longer existing peers. These are peers where typically the
-// underlying host has gone away and/or the address for it has changed. At 95% occupancy for the
-// maximum size allowed, we will remove least recently used secrets (1% of these per minute). Usage
-// is defined by access to the entry in the map. Unfortunately we lack any good way to authenticate
-// a peer as *not* having credentials, especially after the peer is gone. It's possible that in the
-// future information could also come from the TLS provider.
 pub(super) struct State<C, S>
 where
     C: 'static + time::Clock + Sync + Send,
@@ -247,6 +243,7 @@ where
             rehandshake_period: Duration::from_secs(3600 * 24),
             peers: Default::default(),
             ids: Default::default(),
+            eviction_queue: Default::default(),
             cleaner: Cleaner::new(),
             signer,
             control_socket,
@@ -256,6 +253,15 @@ where
             request_handshake: RwLock::new(None),
         };
 
+        // Growing to double our maximum inserted entries should ensure that we never grow again, see:
+        // https://github.com/rust-lang/hashbrown/blob/3bcb84537de01372cab2c1cd3bbfd8577a67ce05/src/raw/mod.rs#L2614
+        //
+        // In practice we don't pin a particular version of hashbrown but there's definitely at
+        // most a constant factor of growth left (vs continuous upwards resizing) with any
+        // reasonable implementation.
+        state.peers.reserve(2 * state.max_capacity);
+        state.ids.reserve(2 * state.max_capacity);
+
         let state = Arc::new(state);
 
         state.cleaner.spawn_thread(state.clone());
@@ -522,8 +528,8 @@ where
 
             queue.push_back(Arc::downgrade(&entry));
 
-            // We are out of room, need to prune some entries.
-            if queue.len() == self.max_capacity {
+            // We went beyond queue limit, need to prune some entries.
+            if queue.len() > self.max_capacity {
                 // FIXME: Consider a more interesting algorithm, e.g., scanning the first N entries
                 // if the popped entry is still live to see if we can avoid dropping a live entry.
                 // May not be worth it in practice.