feat(proto): expose max_transmit_segments and max_transmit_datagrams in TransportConfig

[poka-IT]

Description

Replace the hardcoded constants MAX_TRANSMIT_SEGMENTS = 10 and MAX_TRANSMIT_DATAGRAMS = 20 in noq::Connection::drive_transmit with two configurable fields on TransportConfig. Defaults are unchanged (10 / 20), so default behavior is preserved.

New API:

let mut t = TransportConfig::default();
t.max_transmit_segments(NonZeroUsize::new(40).unwrap());
t.max_transmit_datagrams(NonZeroUsize::new(80).unwrap());

This is an alternative to #636 (which simply bumps the constants to 40 / 80 globally). After feedback from @flub there, exposing the values as a runtime knob seems more defensible:

• No default-behavior change: multi-connection workloads keep their per-connection memory footprint.
• Single-connection bulk-transfer workloads opt in: applications such as ours (peer-to-peer VPN tunnel over QUIC) can call transport.max_transmit_segments(NonZeroUsize::new(40).unwrap()) and get the throughput / CPU-efficiency gains demonstrated in the perf(connection): raise MAX_TRANSMIT_SEGMENTS to 40 and MAX_TRANSMIT_DATAGRAMS to 80 #636 benches.
• Pattern-consistent: same shape as enable_segmentation_offload, congestion_controller_factory, etc.

Why a setter rather than a higher default

MAX_TRANSMIT_SEGMENTS = 10 is exactly equal to MIN_BURST_SIZE in noq-proto/src/connection/pacing.rs:162, the historical 10 was likely chosen to match the pacer's minimum burst. The pacer itself, however, already allows up to MAX_BURST_SIZE = 256 per burst, so even a setter value of 256 stays within the existing pacer envelope. CC and pacing remain authoritative; the constants are just an outer cap.

Trade-offs now self-documenting

• Per-connection memory: max_transmit_segments * current_mtu bytes pre-allocated in TransmitBuf per Connection (rustdoc on the setter spells this out).
• Per-iteration CPU budget vs scheduler fairness (see comment in drive_transmit referencing Congestion window grows without bound when CPU-bound quinn-rs/quinn#1126).

Breaking Changes

n/a, defaults preserved (10 / 20).

Notes & open questions

• Tests added in noq-proto/src/tests/mod.rs:
  • default_max_transmit_settings: defaults remain 10 / 20.
  • max_transmit_segments_setter_caps_batch_size: setter at 4 caps the observed GSO batch size.
• All existing noq-proto and noq lib tests still pass locally.
• If you'd prefer to land perf(connection): raise MAX_TRANSMIT_SEGMENTS to 40 and MAX_TRANSMIT_DATAGRAMS to 80 #636 (pure constant bump) instead of this, I'm happy to close this PR and continue there. They are not mutually exclusive: perf(connection): raise MAX_TRANSMIT_SEGMENTS to 40 and MAX_TRANSMIT_DATAGRAMS to 80 #636 could land on top of this by changing the Default values.

Bench data motivating both PRs (paired same-session, 3x CCX23 Hetzner, real network: intra-DC and inter-region EU) is in the discussion thread of #636.

Change checklist

• Self-review.
• Tests (defaults preserved + setter behavior).
• No breaking changes.

[dignifiedquire]

I like this direction, as it reflects reality better, that these values need to be different depending on actual usage.

[flub]

Yeah, I also think this is a good direction. Doesn't mean the other PR shouldn't happen, but let's do it on top of this one.

[poka-IT]

Thanks for the thorough review. All five points addressed in e0b77c47:

1. Newline between the two fields removed (grouped with enable_segmentation_offload).

2. Both accessors on Connection deleted. To keep the change minimal:

• max_transmit_segments is now clamped inside Connection::poll_transmit directly (via max_datagrams.min(self.config.max_transmit_segments)). The driver no longer has to read this value at all, the clamp follows the active config automatically. In a multi-transport future the clamp can be made per-path right where it lives.
• max_transmit_datagrams is still needed by noq::Connection::drive_transmit (it gates the loop, not the inner poll_transmit call). It is now read once at construction in noq::endpoint (from the ClientConfig / ServerConfig available at that call site) and cached on noq::State, never queried again from Connection. For the case where accept is called without a per-call server_config and the endpoint default is not reachable from noq, I fall back on TransportConfig::default().get_max_transmit_datagrams() — happy to refine if you'd prefer threading the endpoint default through proto::Endpoint::accept's return.

Two small read-only helpers got added to keep the privacy story clean: ClientConfig::get_transport_config() and TransportConfig::get_max_transmit_datagrams() (same get_ prefix as the existing EndpointConfig::get_max_udp_payload_size).

3. default_max_transmit_settings test removed.

4. Defaults extracted as DEFAULT_MAX_TRANSMIT_SEGMENTS / DEFAULT_MAX_TRANSMIT_DATAGRAMS at the top of noq-proto/src/config/transport.rs, used in Default::default().

5. Setter test rewritten using stats-delta: handshake stats are sampled before sending the stream, then again after, and the assertion is datagrams_delta <= ios_delta * CAP. Deterministic — no averaging.

Side-effect: the rustdoc on both setters got compacted, and I trimmed the rustdoc on the enable_segmentation_offload neighbour to match.

[poka-IT]

This latest commit simply changes the default value based on the benchmark results outlined in PR #636.
It can be reverted depending on what you decide.

[flub]

This latest commit simply changes the default value based on the benchmark results outlined in PR #636. It can be reverted depending on what you decide.

That's fine, we can do this in the same PR.

[poka-IT]

Thanks, all five addressed.

[flub]

This seems abandoned. Please try and find the right tradeoff of where to store the configs and how to expose it if re-opening.

[poka-IT]

This seems abandoned. Please try and find the right tradeoff of where to store the configs and how to expose it if re-opening.

I don't understand, everything has been done here since your latest review.

[divagant-martian]

@poka-IT thanks for the contribution. @flub is most likely referring to this comment, which went largely unaddressed.

The request was not only to change the code in certain way but to explore alternatives after the realization that both values might not go together, and to start a conversation about why the changes you decide to include in the code from that point, offer the better tradeoffs.

I understand the decision of closing this might be disappointing to you, so I'd like to offer the maintainer perspective. Without the proper analysis from the contributor, the review now comes down to designing the solution itself. This change is not currently on the top of the priority queue for us, but it would be a welcome one, provided a solution is properly evaluated and justified.

For future endeavors, the last commit also had CI failing with a test that might be transient. As far as I can tell there was no exploration of why that might be. The answer might easily be that it's unrelated but lack of addressing it gave us as well the impression that the PR had been abandoned.

Please feel welcome to contribute in the future and remember to keep these notes in mind