Skip to content

reduce buffer size of "unused" side of socket#4313

Closed
gregcusack wants to merge 2 commits into
anza-xyz:masterfrom
gregcusack:reduce-net-buffer-sizes
Closed

reduce buffer size of "unused" side of socket#4313
gregcusack wants to merge 2 commits into
anza-xyz:masterfrom
gregcusack:reduce-net-buffer-sizes

Conversation

@gregcusack
Copy link
Copy Markdown

@gregcusack gregcusack commented Jan 7, 2025

Follow Up to PR: #3929

Problem

We allocate large send and receive buffers for all sockets. However, not all sockets read and write equally from/to their buffers. In fact, 6 sockets are primarily Read and 2 are primarily Write. Meaning we are allocating memory that is never used.

Summary of Changes

  1. TPU write side of sockets are only QUIC control traffic. Set buffer size to 4 MB
  2. Broadcast and Repair read side of sockets are only QUIC control traffic. Set buffer size to $ MB

Services/Sockets
Read/Write

Gossip
RPC - TCP
Ip_echo - TCP
Repair
Repair_quic
Serve_repair
Serve_repair_quic
Ancestor_hashes_requests_quic
Ancestor_hashes_requests

Primarily Read
TVU
tvu_quic
Tpu
Tpu_forwards
Tpu_vote
Tpu_quic
Tpu_forwards_quic
Tpu_vote_quic

Primarily Write
Retransmitter
Broadcast

Follow Up PR:

  1. Make better sizing decisions for the sockets that don't need a full 128MB buffer

Comment thread gossip/src/cluster_info.rs
@gregcusack gregcusack force-pushed the reduce-net-buffer-sizes branch 2 times, most recently from 85a196a to e62193f Compare January 7, 2025 21:09
@gregcusack gregcusack marked this pull request as ready for review January 7, 2025 22:37
@gregcusack gregcusack requested a review from steviez January 8, 2025 00:16
@gregcusack gregcusack requested review from alexpyattaev and steviez and removed request for steviez March 13, 2025 16:33
Copy link
Copy Markdown

@steviez steviez left a comment

Choose a reason for hiding this comment

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

Ahh, this one slipped through the cracks. I doubt it will make any difference, but can you please rebase to tip as a precaution. I'm guessing your branch is on two-month-old-tip-of-master right now

@gregcusack gregcusack force-pushed the reduce-net-buffer-sizes branch from e62193f to 83a9a82 Compare March 13, 2025 19:07
@gregcusack
Copy link
Copy Markdown
Author

Ahh, this one slipped through the cracks. I doubt it will make any difference, but can you please rebase to tip as a precaution. I'm guessing your branch is on two-month-old-tip-of-master right now

rebased!

@gregcusack gregcusack requested a review from steviez March 13, 2025 19:09
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

testing on unstaked mnb node does not reveal any adverse impacts. will start on testnet, if all goes well I'll merge it in the morning.

alexpyattaev
alexpyattaev previously approved these changes Mar 13, 2025
Copy link
Copy Markdown

@alexpyattaev alexpyattaev left a comment

Choose a reason for hiding this comment

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

ok tested on staked node, TPU works fine, LGTM!

udp_config,
quic_config,
tpu_config,
tpu_reuseport_config,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is actually wrong, quic port should not use REUSEPORT variant. I suggest we merge this PR after #5832 , it will also become much smaller.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

oooh the current code is using reuseport. why do we not want to use reuseport here? and ya no prob waiting.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

because it allows it to bind on top of an already occupied port here =)

@steviez
Copy link
Copy Markdown

steviez commented Sep 8, 2025

Clearing out the inbox - are we still interested in pursuing this one @gregcusack ?

@gregcusack
Copy link
Copy Markdown
Author

Clearing out the inbox - are we still interested in pursuing this one @gregcusack ?

ahh yes we are. haven't gotten a chance to come back to this after recent refactor of all the socket libraries. we can close and i can create a new PR

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.

3 participants