The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27596 (assumeutxo (2) by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Concept ACK on the msgs, but do we really want to release all mem all the time for the others? Without the shrinks, we essentially keep a high-water mark for memory where future allocations are unnecessary.

I poked quickly at m_send_buffer for in MarkBytesSent for example, and it seems like that means constant reallocation now, no?

@theuni For m_send_buffer, m_recv_buffer, and m_recv_decode_buffer it's not unreasonable to e.g. permit a certain lingering memory usage. E.g. if the capacity is under 64 KiB, don't wipe.

I considered that before, but wasn't sure it'd be worth the discussion. We're already doing several allocations per P2P message already in various places, some of which will require more refactoring to get rid of, so one more or less probably doesn't matter that much.

For this PR, I wanted to just make it do what it was intending to do already.

Concept ACK.

I'm curious, does this change have an observable effect on peak memory usage?

@hebasto Not right now, as V2Transport isn't hooked up to anything. But once it is, without this PR, after sending a 4 MB message you'd permanently keep 4 MB allocated for that connection (and similarly for the receive side).

FWIW, shrink_to_fit:

... is a non-binding request to reduce capacity() to size(). It depends on the implementation whether the request is fulfilled.

@hebasto I'm aware. But it appears to work better in practice than v = std::vector<T>{};.

I guess we could also use the combination, v = std::vector<T>{}; v.shrink_to_fit();.

v.swap(std::vector<T>{}); is also an option, possibly followed by v.shrink_to_fit();.

v.swap(std::vector<T>{}); is also an option, possibly followed by v.shrink_to_fit();.

I was thinking just about the same :)

Replace those with v.clear(); v.shrink_to_fit();.

PR description needs updating?

@stickies-v

Found two more instances where ClearShrink seems appropriate:

• bitcoin/src/net_processing.cpp

  Line 2602 in adc0921

  headers = {};

The headers variable here is just a local variable that will go out of scope when the received headers are processed; there is no long-term memory usage we're concerned about. I believe the intent of the = {}; there is just to clear the vector (so headers don't get processed a second time).

• bitcoin/src/script/script.h

  Line 575 in adc0921

  void SetNull() { stack.clear(); stack.shrink_to_fit(); }

  (but probably not worth adding in this pull as it's consensus code)

Yeah, not going to touch that in this PR.

Question out of curiosity: does the C++17 standard guarantee that default-constructed (i.e. new and empty) std::vectors have an initial capacity of zero? If not (as e.g. suggested by https://stackoverflow.com/a/23415582 or https://tylerayoung.com/2020/08/20/default-capacity-growth-rate-of-c-stdvector/ -- not sure what C++ version specifically they are talking about though), then strictly speaking there'd be no guaranteed way to clear the memory that is not implementation-dependent, it seems.

I skimmed over the C++17 spec, but can't find anything about capacity of newly-constructed vectors. I don't think we need to care about this though; as that SO link says, I think it'd be nonsensical for an implementation to allocate anything for an empty newly-constructed vector.

utACK 3fcd7fc

Backported (partial) to 25.x in #28487.