Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drop vAddrToSend after sending big addr message #8154

Merged
merged 1 commit into from Jun 9, 2016

Conversation

@kazcw
Copy link
Contributor

commented Jun 6, 2016

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.

@sipa

View changes

src/main.cpp Outdated
@@ -5713,6 +5713,9 @@ bool SendMessages(CNode* pto)
pto->vAddrToSend.clear();
if (!vAddr.empty())
pto->PushMessage(NetMsgType::ADDR, vAddr);
// we only send the big addr message once
if (pto->vAddrToSend.capacity() > 40)
pto->vAddrToSend = {};

This comment has been minimized.

Copy link
@sipa

sipa Jun 6, 2016

Member

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.

This comment has been minimized.

Copy link
@kazcw

kazcw Jun 6, 2016

Author Contributor

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"...

This comment has been minimized.

Copy link
@sipa

sipa Jun 7, 2016

Member

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.

This comment has been minimized.

Copy link
@sipa

sipa Jun 7, 2016

Member

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

This comment has been minimized.

Copy link
@kazcw

kazcw Jun 7, 2016

Author Contributor

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

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.

@kazcw kazcw force-pushed the kazcw:drop-vAddrToSend branch to d3d02d5 Jun 6, 2016

@jonasschnelli jonasschnelli added the P2P label Jun 7, 2016

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 7, 2016

utACK d3d02d5

@laanwj

This comment has been minimized.

Copy link
Member

commented Jun 9, 2016

utACK d3d02d5

@laanwj laanwj merged commit d3d02d5 into bitcoin:master Jun 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jun 9, 2016
Merge #8154: drop vAddrToSend after sending big addr message
d3d02d5 drop vAddrToSend after sending big addr message (Kaz Wesley)
codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017
Merge bitcoin#8154: drop vAddrToSend after sending big addr message
d3d02d5 drop vAddrToSend after sending big addr message (Kaz Wesley)
andvgal added a commit to energicryptocurrency/energi that referenced this pull request Jan 6, 2019
Merge bitcoin#8154: drop vAddrToSend after sending big addr message
d3d02d5 drop vAddrToSend after sending big addr message (Kaz Wesley)
LitecoinZ added a commit to litecoinz-project/litecoinz that referenced this pull request Apr 28, 2019
Cherry-picked from upstream PRs:
bitcoin/bitcoin#8136: Log/report in 10% steps during VerifyDB
bitcoin/bitcoin#8154: drop vAddrToSend after sending big addr message
@LitecoinZ LitecoinZ referenced this pull request Apr 30, 2019
53 of 77 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.