drop vAddrToSend after sending big addr message #8154

kazcw commented at 10:17 PM on June 6, 2016: contributor

We send a newly-accepted peer a 1000-entry addr message, and then only use vAddrToSend for small messages. Deallocate vAddrToSend after it's been used for the big message to save about 40 kB per connected inbound peer.

drop vAddrToSend after sending big addr message

We send a newly-accepted peer a 1000-entry addr message, and then only use
vAddrToSend for small messages. Deallocate vAddrToSend after it's been used for
the big message to save about 40 kB per connected inbound peer.

d3d02d5145

in src/main.cpp:None in a89232c268 outdated

5712 | @@ -5713,6 +5713,9 @@ bool SendMessages(CNode* pto)
5713 |              pto->vAddrToSend.clear();
5714 |              if (!vAddr.empty())
5715 |                  pto->PushMessage(NetMsgType::ADDR, vAddr);
5716 | +            // we only send the big addr message once
5717 | +            if (pto->vAddrToSend.capacity() > 40)
5718 | +                pto->vAddrToSend = {};

sipa commented at 10:28 PM on June 6, 2016:

I don't think this frees up space.

To do so, I think you need the amazing idiom std::vector<CAdress>().swap(pto->vAddrToSend). Since c++11 you can also use pto->vAddrToSend.shrink_to_fit() as a non-binding request.

kazcw commented at 10:59 PM on June 6, 2016:

Wow, is the temporary-swap still necessary? I was so happy thinking we left that behind with C++98. I think a move assignment would work -- for compatible allocators its time complexity is documented as constant in the size of the other vector, which is only possible if it adopts the other vector's storage.

I'm inclined to go with the straightforward shrink_to_fit. It's non-binding specifically to allow for optimizations, and no sane implementation would refuse to deallocate 40kB as an "optimization"...

sipa commented at 12:50 AM on June 7, 2016:

I just tested. Your old code indeed would not reduce capacity for me. However pto->vAddrToSend = std::vector<CAddress>{} does reduce it to 0. I wonder what the difference is between a type name being present before the {} or not.

sipa commented at 12:51 AM on June 7, 2016:

Oh, I think that without type name, the initializer list assignment is called: (3) here: http://en.cppreference.com/w/cpp/container/vector/operator%3D

kazcw commented at 2:15 AM on June 7, 2016:

Yeah, I wrote it that way thinking it would construct a temporary from the initializer list and then move-assign from the rvalue temporary. That's what it would do, if assigning from an initializer list weren't overridden to provide subtly different behaviour... but of course the standard library conveniently has an override for everything :P

kazcw force-pushed on Jun 6, 2016

jonasschnelli added the label P2P on Jun 7, 2016

sipa commented at 2:14 PM on June 7, 2016: member

utACK d3d02d51453943bfe3a9edb944eb48f9f1e01aca

laanwj commented at 6:12 AM on June 9, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8154/commits/d3d02d51453943bfe3a9edb944eb48f9f1e01aca

laanwj merged this on Jun 9, 2016

laanwj closed this on Jun 9, 2016

laanwj referenced this in commit 1445835bd3 on Jun 9, 2016

codablock referenced this in commit 435dcdd3dd on Sep 16, 2017

codablock referenced this in commit e581611310 on Sep 19, 2017

codablock referenced this in commit aa6cb48bfa on Dec 22, 2017

andvgal referenced this in commit d919d2040d on Jan 6, 2019

MarkLTZ referenced this in commit e8e96e9c25 on Apr 28, 2019

MarcoFalke locked this on Sep 8, 2021