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

Compute the expected body length to reduce heap allocations #1773

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

ebfull
Copy link
Contributor

@ebfull ebfull commented Feb 18, 2021

Motivation

Unnecessary allocation(s) in the external protocol codec implementation.

Solution

Let's precompute the expected body length of a network message and reserve this capacity in the provided buffer. Also compute the checksum in-place.

Review

This review is a low priority.

Follow Up

Add a size estimator #1774
Proptest our message implementation #27
Add benchmarks for network messages #1775

@zfnd-bot zfnd-bot bot added this to In progress in 🦓 Feb 18, 2021
@teor2345 teor2345 self-requested a review February 18, 2021 20:48
@teor2345 teor2345 added A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup P-Low labels Feb 18, 2021
teor2345
teor2345 previously approved these changes Feb 18, 2021
Copy link
Collaborator

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

This change removes a Vec allocation for every peer message. But the exact body length calculation adds an extra serialization. (I'm not sure if the functions called by our serializations do any extra allocations internally. And I don't know how much of the redundant FakeWriter serializations will be removed by the optimiser. But our serialization code does try to write directly to the Writer.)

Overall, allocations are more expensive than serialization, particularly if the allocation incurs a page fault. So this should provide a small performance improvement. (And a bit of code cleanup.)

We should follow up this PR with a size estimator and a benchmark, but that's on us :-)

Co-authored-by: teor <teor@riseup.net>
@teor2345
Copy link
Collaborator

The macOS testnet CI hung without any logs, #1222 should fix that, and #1140 will get us better diagnostics when that happens.

I thought we'd found all the places where Zebra just hung without any output, but maybe we've missed one.

@ebfull
Copy link
Contributor Author

ebfull commented Feb 18, 2021

@teor2345 Yeah, I was originally writing out size estimators for each message, but eventually due to all the nested structures it started to look like... the serialization code. So I figured this would be as efficient modulo some pointer arithmetic (which probably won't get optimized out, but meh)

@teor2345
Copy link
Collaborator

@ebfull yeah it's possible there's no good design for size estimation - or we'd have to rewrite all out serialisation code using a visitor pattern. So we might never end up doing that ticket.

Doubling the size of all our serialisation code wouldn't be great for performance either.

Copy link
Collaborator

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

The latest change just applies my comment suggestion.

@teor2345
Copy link
Collaborator

We should re-run CI after #1789 merges, then merge this PR once it passes.

@teor2345 teor2345 closed this Feb 19, 2021
🦓 automation moved this from In progress to Done Feb 19, 2021
@teor2345 teor2345 reopened this Feb 19, 2021
🦓 automation moved this from Done to Review in progress Feb 19, 2021
@teor2345 teor2345 enabled auto-merge (squash) February 19, 2021 22:15
@teor2345 teor2345 merged commit b7fddbd into ZcashFoundation:main Feb 19, 2021
🦓 automation moved this from Review in progress to Done Feb 19, 2021
@ebfull ebfull deleted the compute-expected-body-len branch February 19, 2021 22:23
@teor2345 teor2345 mentioned this pull request Feb 23, 2021
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-cleanup Category: This is a cleanup
Projects
No open projects
🦓
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants