make QUIC tpu QOS parameters configurable #4170

lijunwangs · 2024-12-19T09:29:12Z

Problem

Making QUIC TPU QOS parameters configurable so that it is easier to tweak performance and do bench marks.

Summary of Changes

CLI is enabled to pass the following:

--tpu-max-connections-per-peer
--max-tpu-staked-connections
--max-tpu-unstaked-connections
--max-fwd-staked-connections
--max-fwd-unstaked-connections

They are set to hidden unless forced to be shown.

Elevate QuicServerParams configuration to ValidatorTpuConfig as field to configure
tpu, tpu forward and vote quic server parameters.

Move the public default parameters to quic from nonblocking::quic to encapsulate better.

Fixes #

lijunwangs · 2024-12-19T09:32:26Z

core/src/tpu.rs

@@ -54,9 +51,6 @@ use {
    tokio::sync::mpsc::Sender as AsyncSender,
 };

-// allow multiple connections for NAT and any open/close overlap
-pub const MAX_QUIC_CONNECTIONS_PER_PEER: usize = 8;


moved to quic.rs in streamer

this symbol needs reexported from here to avoid breaking api. also add #[deprecated(...)] annotation plz

lijunwangs · 2024-12-19T09:33:07Z

streamer/src/nonblocking/quic.rs

@@ -87,14 +87,6 @@ const CONNECTION_CLOSE_REASON_TOO_MANY: &[u8] = b"too_many";
 const CONNECTION_CLOSE_CODE_INVALID_STREAM: u32 = 5;
 const CONNECTION_CLOSE_REASON_INVALID_STREAM: &[u8] = b"invalid_stream";

-/// Limit to 250K PPS


move to ../quic.rs

these similarly need reexported here with deprecation annotations

streamer/src/quic.rs

validator/src/main.rs

alessandrod

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

core/src/tpu.rs

core/src/validator.rs

validator/src/cli.rs

lijunwangs · 2024-12-22T23:33:06Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:

We never promised solana-streamer to a public interface as others like solana-sdk, solana-client, solana-geyser-interface. The interfaces are made public to share among agave's crates. We have done such refactoring before. But I agree with should interface cleaner and stable if we can, hope this refactoring actually make it more stable in the future... :)
We are renaming some of the constant from MAX_*** to DEFAULT_MAX_** to connote they are configurable anyway which requires a change of the names.
streamer::nonblocking is pure internal implementation should have been hidden and use streamer::quic as the only interface for quic related server interactions.

lijunwangs · 2025-01-03T21:34:18Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:

We never promised solana-streamer to a public interface as others like solana-sdk, solana-client, solana-geyser-interface. The interfaces are made public to share among agave's crates. We have done such refactoring before. But I agree with should interface cleaner and stable if we can, hope this refactoring actually make it more stable in the future... :)

We are renaming some of the constant from MAX_*** to DEFAULT_MAX_** to connote they are configurable anyway which requires a change of the names.

streamer::nonblocking is pure internal implementation should have been hidden and use streamer::quic as the only interface for quic related server interactions.

Hi @alessandrod, just a gentle nudge, can you please take another look at this PR? Thanks

alessandrod · 2025-01-07T02:49:16Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:
Hi @alessandrod, just a gentle nudge, can you please take another look at this PR? Thanks

I'm on board (i don't care about not breaking these "accidental" APIs), but you need to get @t-nelson on board too since he blocked a similar PR a few months ago (the one consolidating arguments from stephen? Can't seem to find it now)

lijunwangs · 2025-01-07T05:59:03Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:
Hi @alessandrod, just a gentle nudge, can you please take another look at this PR? Thanks

I'm on board (i don't care about not breaking these "accidental" APIs), but you need to get @t-nelson on board too since he blocked a similar PR a few months ago (the one consolidating arguments from stephen? Can't seem to find it now)

I think it is about this PR #3328 which is already merged -- my take from that conversation is streamer is not really a public crate. I added @t-nelson to review.

lijunwangs · 2025-01-08T16:17:58Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:
Hi @alessandrod, just a gentle nudge, can you please take another look at this PR? Thanks

I'm on board (i don't care about not breaking these "accidental" APIs), but you need to get @t-nelson on board too since he blocked a similar PR a few months ago (the one consolidating arguments from stephen? Can't seem to find it now)

I think it is about this PR #3328 which is already merged -- my take from that conversation is streamer is not really a public crate. I added @t-nelson to review.

Hi @t-nelson do you have any concerns regarding this? Need to wrap this PR up. Thanks.

lijunwangs · 2025-01-09T23:47:46Z

This looks good, however it breaks API but renaming/moving some pub items. I don't care, but in the past people have complained about this so might be worth trying to not do those changes?

Yes -- I'have thought about that. But I took the action to consolidate the public defaults in various places to streamer::quic. For a few reasons:
Hi @alessandrod, just a gentle nudge, can you please take another look at this PR? Thanks

I'm on board (i don't care about not breaking these "accidental" APIs), but you need to get @t-nelson on board too since he blocked a similar PR a few months ago (the one consolidating arguments from stephen? Can't seem to find it now)

I think it is about this PR #3328 which is already merged -- my take from that conversation is streamer is not really a public crate. I added @t-nelson to review.

Hi @t-nelson do you have any concerns regarding this? Need to wrap this PR up. Thanks.

I checked PR #3328 's commit: 6319db8c7a8938a7e63e6d698a04670be3de0576 is not yet released: v2.2.x. We get in these refactoring changes together in one release to reduce churns. cc @sakridge @alessandrod @t-nelson

lijunwangs commented Dec 19, 2024

View reviewed changes

streamer/src/quic.rs Show resolved Hide resolved

lijunwangs commented Dec 20, 2024

View reviewed changes

validator/src/main.rs Show resolved Hide resolved

lijunwangs requested review from sakridge and alessandrod December 20, 2024 16:20

alessandrod reviewed Dec 21, 2024

View reviewed changes

core/src/tpu.rs Outdated Show resolved Hide resolved

core/src/validator.rs Outdated Show resolved Hide resolved

validator/src/cli.rs Outdated Show resolved Hide resolved

lijunwangs force-pushed the configure_quic_qos branch from 88323a3 to 470dfcf Compare December 22, 2024 23:04

lijunwangs force-pushed the configure_quic_qos branch from 470dfcf to 1ea71ce Compare January 7, 2025 02:10

lijunwangs requested a review from t-nelson January 7, 2025 05:49

lijunwangs force-pushed the configure_quic_qos branch from 1ea71ce to a8cc8e9 Compare January 7, 2025 07:44

lijunwangs added 9 commits January 11, 2025 12:45

make QUIC tpu QOS parameters configurable

aba559e

Use max_connections_per_ipaddr_per_min

c1621c9

set max_unstaked_connections to 0 for tpu-fwd and vote in testing

8037404

fixed some clippy complaint

30267bc

missing max-streams-per-ms

9c91cef

missing tpu_max_streams_per_ms

cefd3e6

vote does not accept unstaked connections

3e74ee7

Addressed some feedback from Alessandro

a22ceba

re-export some constants moved/renamed and mark them deprecated

133db69

lijunwangs force-pushed the configure_quic_qos branch from a8cc8e9 to 133db69 Compare January 11, 2025 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make QUIC tpu QOS parameters configurable #4170

make QUIC tpu QOS parameters configurable #4170

lijunwangs commented Dec 19, 2024 •

edited

Loading

lijunwangs Dec 19, 2024

t-nelson Jan 11, 2025

lijunwangs Jan 11, 2025

lijunwangs Dec 19, 2024

t-nelson Jan 11, 2025

lijunwangs Jan 11, 2025

alessandrod left a comment

lijunwangs commented Dec 22, 2024 •

edited

Loading

lijunwangs commented Jan 3, 2025

alessandrod commented Jan 7, 2025

lijunwangs commented Jan 7, 2025 •

edited

Loading

lijunwangs commented Jan 8, 2025

lijunwangs commented Jan 9, 2025

make QUIC tpu QOS parameters configurable #4170

Are you sure you want to change the base?

make QUIC tpu QOS parameters configurable #4170

Conversation

lijunwangs commented Dec 19, 2024 • edited Loading

Problem

Summary of Changes

lijunwangs Dec 19, 2024

Choose a reason for hiding this comment

t-nelson Jan 11, 2025

Choose a reason for hiding this comment

lijunwangs Jan 11, 2025

Choose a reason for hiding this comment

lijunwangs Dec 19, 2024

Choose a reason for hiding this comment

t-nelson Jan 11, 2025

Choose a reason for hiding this comment

lijunwangs Jan 11, 2025

Choose a reason for hiding this comment

alessandrod left a comment

Choose a reason for hiding this comment

lijunwangs commented Dec 22, 2024 • edited Loading

lijunwangs commented Jan 3, 2025

alessandrod commented Jan 7, 2025

lijunwangs commented Jan 7, 2025 • edited Loading

lijunwangs commented Jan 8, 2025

lijunwangs commented Jan 9, 2025

lijunwangs commented Dec 19, 2024 •

edited

Loading

lijunwangs commented Dec 22, 2024 •

edited

Loading

lijunwangs commented Jan 7, 2025 •

edited

Loading