Merge bitcoin/bitcoin#28452: Do not use std::vector = {} to release m…

…emory

3fcd7fc Do not use std::vector = {} to release memory (Pieter Wuille)

Pull request description:

  It appears that invoking `v = {};` for an `std::vector<...> v` is equivalent to `v.clear()`, which does not release its allocated memory. There are a number of places in the codebase where it appears to be used for that purpose however (mostly written by me). Replace those with `std::vector<...>{}.swap(v);` (using a helper function `ClearShrink` in util/vector.h).

  To explain what is going on: `v = {...};` is equivalent in general to `v.operator=({...});`. For many types, the `{}` is converted to the type of `v`, and then assigned to `v` - which for `std::vector` would ordinarily have the effect of clearing its memory (constructing a new empty vector, and then move-assigning it to `v`). However, since `std::vector<T>` has an `operator=(std::initializer_list<T>)` defined, it has precedence (since no implicit conversion is needed), and with an empty list, that is equivalent to `clear()`.

  I did consider using `v = std::vector<T>{};` as replacement for `v = {};` instances where memory releasing is desired, but it appears that it does not actually work universally either. `V{}.swap(v);` does.

ACKs for top commit:
  ajtowns:
    utACK 3fcd7fc
  stickies-v:
    ACK 3fcd7fc
  theStack:
    Code-review ACK 3fcd7fc

Tree-SHA512: 6148558126ec3c8cfd6daee167ec1c67b360cf1dff2cbc132bd71768337cf9bc4dda3e5a9cf7da4f7457d2123288eeba77dd78f3a17fa2cfd9c6758262950cc5

src/headerssync.cpp

@@ -7,6 +7,7 @@
 #include <pow.h>
 #include <timedata.h>
 #include <util/check.h>
+#include <util/vector.h>
 
 // The two constants below are computed using the simulation script on
 // https://gist.github.com/sipa/016ae445c132cdf65a2791534dfb7ae1
@@ -51,9 +52,9 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
 void HeadersSyncState::Finalize()
 {
     Assume(m_download_state != State::FINAL);
-    m_header_commitments = {};
+    ClearShrink(m_header_commitments);
     m_last_header_received.SetNull();
-    m_redownloaded_headers = {};
+    ClearShrink(m_redownloaded_headers);
     m_redownload_buffer_last_hash.SetNull();
     m_redownload_buffer_first_prev_hash.SetNull();
     m_process_all_remaining_headers = false;

src/net.cpp

@@ -35,6 +35,7 @@
 #include <util/threadinterrupt.h>
 #include <util/trace.h>
 #include <util/translation.h>
+#include <util/vector.h>
 
 #ifdef WIN32
 #include <string.h>
@@ -899,8 +900,7 @@ void V1Transport::MarkBytesSent(size_t bytes_sent) noexcept
         m_bytes_sent = 0;
     } else if (!m_sending_header && m_bytes_sent == m_message_to_send.data.size()) {
         // We're done sending a message's data. Wipe the data vector to reduce memory consumption.
-        m_message_to_send.data.clear();
-        m_message_to_send.data.shrink_to_fit();
+        ClearShrink(m_message_to_send.data);
         m_bytes_sent = 0;
     }
 }
@@ -1123,8 +1123,8 @@ void V2Transport::ProcessReceivedMaybeV1Bytes() noexcept
         SetReceiveState(RecvState::V1);
         SetSendState(SendState::V1);
         // Reset v2 transport buffers to save memory.
-        m_recv_buffer = {};
-        m_send_buffer = {};
+        ClearShrink(m_recv_buffer);
+        ClearShrink(m_send_buffer);
     } else {
         // We have not received enough to distinguish v1 from v2 yet. Wait until more bytes come.
     }
@@ -1184,8 +1184,7 @@ bool V2Transport::ProcessReceivedKeyBytes() noexcept
             /*ignore=*/false,
             /*output=*/MakeWritableByteSpan(m_send_buffer).last(BIP324Cipher::EXPANSION));
         // We no longer need the garbage.
-        m_send_garbage.clear();
-        m_send_garbage.shrink_to_fit();
+        ClearShrink(m_send_garbage);
 
         // Construct version packet in the send buffer.
         m_send_buffer.resize(m_send_buffer.size() + BIP324Cipher::EXPANSION + VERSION_CONTENTS.size());
@@ -1275,7 +1274,7 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
             // Ignore flag does not matter for garbage authentication. Any valid packet functions
             // as authentication. Receive and process the version packet next.
             SetReceiveState(RecvState::VERSION);
-            m_recv_garbage = {};
+            ClearShrink(m_recv_garbage);
             break;
         case RecvState::VERSION:
             if (!ignore) {
@@ -1295,9 +1294,9 @@ bool V2Transport::ProcessReceivedPacketBytes() noexcept
             Assume(false);
         }
         // Wipe the receive buffer where the next packet will be received into.
-        m_recv_buffer = {};
+        ClearShrink(m_recv_buffer);
         // In all but APP_READY state, we can wipe the decoded contents.
-        if (m_recv_state != RecvState::APP_READY) m_recv_decode_buffer = {};
+        if (m_recv_state != RecvState::APP_READY) ClearShrink(m_recv_decode_buffer);
     } else {
         // We either have less than 3 bytes, so we don't know the packet's length yet, or more
         // than 3 bytes but less than the packet's full ciphertext. Wait until those arrive.
@@ -1511,7 +1510,7 @@ CNetMessage V2Transport::GetReceivedMessage(std::chrono::microseconds time, bool
         LogPrint(BCLog::NET, "V2 transport error: invalid message type (%u bytes contents), peer=%d\n", m_recv_decode_buffer.size(), m_nodeid);
         reject_message = true;
     }
-    m_recv_decode_buffer = {};
+    ClearShrink(m_recv_decode_buffer);
     SetReceiveState(RecvState::APP);
 
     return msg;
@@ -1545,7 +1544,7 @@ bool V2Transport::SetMessageToSend(CSerializedNetMsg& msg) noexcept
     m_cipher.Encrypt(MakeByteSpan(contents), {}, false, MakeWritableByteSpan(m_send_buffer));
     m_send_type = msg.m_type;
     // Release memory
-    msg.data = {};
+    ClearShrink(msg.data);
     return true;
 }
 
@@ -1577,7 +1576,7 @@ void V2Transport::MarkBytesSent(size_t bytes_sent) noexcept
     // Wipe the buffer when everything is sent.
     if (m_send_pos == m_send_buffer.size()) {
         m_send_pos = 0;
-        m_send_buffer = {};
+        ClearShrink(m_send_buffer);
     }
 }
 

src/test/util_tests.cpp

@@ -1791,4 +1791,29 @@ BOOST_AUTO_TEST_CASE(util_WriteBinaryFile)
     BOOST_CHECK(valid);
     BOOST_CHECK_EQUAL(actual_text, expected_text);
 }
+
+BOOST_AUTO_TEST_CASE(clearshrink_test)
+{
+    {
+        std::vector<uint8_t> v = {1, 2, 3};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        BOOST_CHECK_EQUAL(v.capacity(), 0);
+    }
+
+    {
+        std::vector<bool> v = {false, true, false, false, true, true};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        BOOST_CHECK_EQUAL(v.capacity(), 0);
+    }
+
+    {
+        std::deque<int> v = {1, 3, 3, 7};
+        ClearShrink(v);
+        BOOST_CHECK_EQUAL(v.size(), 0);
+        // std::deque has no capacity() we can observe.
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()

src/util/vector.h

@@ -49,4 +49,22 @@ inline V Cat(V v1, const V& v2)
     return v1;
 }
 
+/** Clear a vector (or std::deque) and release its allocated memory. */
+template<typename V>
+inline void ClearShrink(V& v) noexcept
+{
+    // There are various ways to clear a vector and release its memory:
+    //
+    // 1. V{}.swap(v)
+    // 2. v = V{}
+    // 3. v = {}; v.shrink_to_fit();
+    // 4. v.clear(); v.shrink_to_fit();
+    //
+    // (2) does not appear to release memory in glibc debug mode, even if v.shrink_to_fit()
+    // follows. (3) and (4) rely on std::vector::shrink_to_fit, which is only a non-binding
+    // request. Therefore, we use method (1).
+
+    V{}.swap(v);
+}
+
 #endif // BITCOIN_UTIL_VECTOR_H