Skip to content

network: remove GossipNode.BroadcastArray #6281

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 4 commits into from
Mar 19, 2025

Conversation

cce
Copy link
Contributor

@cce cce commented Mar 17, 2025

Summary

In #1966 the methods BroadcastArray and RelayArray were added to the GossipNode interface. (The idea was to break up long messages like proposals into a sequence of smaller messages, to allow canceling a proposal while it was still being sent, as soon as a better one was observed.) However they were never used, and have no callers. They require every (tag, message) pair, to be wrapped in two single-element []protocol.Tag and [][]byte slices. This PR attempts to see if it is straightforward (and more performant) to go back to single-message sending, without the slice wrappers.

Review recommended with whitespace hidden.

Test Plan

Updated TestWebsocketNetworkCancel and TestPreparePeerData for missing BroadcastArray. Existing unit & e2e test should ensure that networking is still working with single messages.

@cce cce marked this pull request as ready for review March 17, 2025 22:13
@cce cce requested a review from algorandskiy March 17, 2025 22:13
Copy link

codecov bot commented Mar 17, 2025

Codecov Report

Attention: Patch coverage is 95.65217% with 2 lines in your changes missing coverage. Please review.

Project coverage is 51.66%. Comparing base (272ec02) to head (9db0a1c).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
network/wsNetwork.go 90.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6281      +/-   ##
==========================================
+ Coverage   51.63%   51.66%   +0.02%     
==========================================
  Files         647      647              
  Lines       86907    86874      -33     
==========================================
+ Hits        44876    44883       +7     
+ Misses      39161    39126      -35     
+ Partials     2870     2865       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@cce cce requested review from jannotti and gmalouf March 17, 2025 23:22
@gmalouf
Copy link
Contributor

gmalouf commented Mar 18, 2025

We still have an idea on our board to split up the block proposal - does this make that harder, or just revisit when we get to it?

@jannotti
Copy link
Contributor

We still have an idea on our board to split up the block proposal - does this make that harder, or just revisit when we get to it?

I think if we want to go back, we could retain the single broadcast method, but implement it with a sync.Pool for the singleton slices.

Though it makes me wonder: Are we sure this was even allocating? I think Go can allocate small slices on the stack if they're just fed into another function and don't escape. I might be imagining that.

@algorandskiy
Copy link
Contributor

Though it makes me wonder: Are we sure this was even allocating? I think Go can allocate small slices on the stack if they're just fed into another function and don't escape. I might be imagining that.

It does escape at least in Broadcast():

network/wsNetwork.go:364:19: make([][]byte, 1) escapes to heap
network/wsNetwork.go:366:18: make([]protocol.Tag, 1) escapes to heap

@cce
Copy link
Contributor Author

cce commented Mar 18, 2025

I was thinking if we want to split up the block proposal (and support canceling mid-stream), we could add special handling in readLoop/writeLoop for handling large PP-tagged messages. When @algorandskiy added proposal compression, he added special handing for PP tag messages to the readLoop (to decompress them) and we have other special handling of tags in readLoop and writeLoop as well (for large message caching and other things).

IMO, to implement proposal splitting/canceling, it could be more clear to have a single large message with a single PP tag show up to be sent to the writeLoop, rather than a sequence of split-up messages of different tag types that were split up by the sender or inside of agreement.

@algorandskiy algorandskiy merged commit 55011f9 into algorand:master Mar 19, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants