Skip to content

avoid redundant memcopy in node_buffer::get_nodes #23

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

Merged
merged 2 commits into from
Aug 22, 2016

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Aug 21, 2016

Instead of heap allocating a string and copying nodes into, this patch makes node_buffer::get_nodes() return 2 ranges of bytes to be copied into the packet. This avoids heap allocations entirely and saves at least one memcopy for all node data.

@arvidn
Copy link
Contributor Author

arvidn commented Aug 21, 2016

@arvidn arvidn force-pushed the node-buffer_optimization branch from cbfcd51 to 0b11328 Compare August 22, 2016 17:01
@arvidn
Copy link
Contributor Author

arvidn commented Aug 22, 2016

{
std::string ret;

if (m_buffer.size() < num_nodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comparison should be <=. No point in returning two ranges if they're going to comprise the entire buffer.

@ssiloti
Copy link
Contributor

ssiloti commented Aug 22, 2016

lgtm

@arvidn arvidn merged commit 1222e75 into bittorrent:master Aug 22, 2016
@arvidn arvidn deleted the node-buffer_optimization branch August 22, 2016 18:18
@arvidn
Copy link
Contributor Author

arvidn commented Aug 23, 2016

actually, changing that comparison operator made the unit test fail. I'll fix that in-place in the master branch (since I already merged it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants