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

avoid unnecessary async copies in broadcast #3830

Merged
merged 1 commit into from
Jul 1, 2022
Merged

avoid unnecessary async copies in broadcast #3830

merged 1 commit into from
Jul 1, 2022

Conversation

arnetheduck
Copy link
Member

No description provided.

try: snappy.encode(uncompressed)
except InputTooLarge:
raiseAssert "More than 4gb? not likely.."
proc broadcast(node: Eth2Node, topic: string, msg: seq[byte]):
Copy link
Contributor

@zah zah Jun 30, 2022

Choose a reason for hiding this comment

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

Is the claim that this PR is reducing the number of copies? There will be still some copying here (both topic and msg will be copied)

Copy link
Member Author

Choose a reason for hiding this comment

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

it avoids the (extremely slow) copy of msg via genericAssign as well as the lifetime extension of keeping copies of the unserialized object and uncompessed in the closure environment

Copy link
Contributor

Choose a reason for hiding this comment

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

We can perhaps start optimizing away even the string and seq copies like the ones here by using more ref parameters.

Copy link
Member Author

@arnetheduck arnetheduck Jul 1, 2022

Choose a reason for hiding this comment

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

not worth it imo, this is by far not the biggest culprit (most copying happens in libp2p which would require major changes) - this was just a small drive-by fix that doesn't change semantics or introduce extra mutability like ref would - it gets rid of the extra try/catch layers that also were introducing overhead and noise (no longer needed thanks to the new snappy impl)

@github-actions
Copy link

Unit Test Results

     12 files  ±0     857 suites  ±0   1h 0m 35s ⏱️ - 2m 48s
1 712 tests ±0  1 660 ✔️ ±0    52 💤 ±0  0 ±0 
9 944 runs  ±0  9 816 ✔️ ±0  128 💤 ±0  0 ±0 

Results for commit 47ab7bf. ± Comparison against base commit 24c435a.

@zah zah merged commit 6a3bd89 into unstable Jul 1, 2022
@zah zah deleted the encode-copy branch July 1, 2022 14:48
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