Skip to content

feat: allow configuring the DATA frame overhead threshold - #950

Open
tatsuiman wants to merge 1 commit into
hyperium:masterfrom
tatsuiman:configurable-data-frame-overhead-threshold
Open

feat: allow configuring the DATA frame overhead threshold#950
tatsuiman wants to merge 1 commit into
hyperium:masterfrom
tatsuiman:configurable-data-frame-overhead-threshold

Conversation

@tatsuiman

Copy link
Copy Markdown

Motivation

data_frame_budget is configurable, but the payload length below which a DATA frame is
charged against it is a fixed 256 bytes (DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD). A peer
that legitimately receives many small DATA frames — a server-sent event stream whose events
are tens to low hundreds of bytes each is the ordinary case — is charged on every frame,
and today has no way to say "frames this small are normal here" other than by raising the
budget.

Raising the budget does not express it cleanly, because whether such a stream survives
depends on the connection window as well as the frame size. A frame of payload L < T
consumes T - L until the application reads it, and the number that can sit unread is
bounded by the connection window W, so the worst-case simultaneous charge is
W * (T - L) / L. With the Auto budget B = max(W / 2, 25_600), for W >= 51_200 the
stream avoids the budget exactly when

W * (T - L) / L  <=  W / 2      <=>      2T <= 3L      <=>      L >= (2/3) * T

W cancels — which is the improvement Auto already made. But the residual cutoff is
(2/3) * 256, about 171 bytes, and it is a property of T, not something the budget
can move. Any budget value picked to get under it is back to being a number derived from
W, so it silently becomes wrong the day someone widens the window.

Making T itself configurable removes both dependencies. Frames at or above T are
never charged at all, so setting T at or below the smallest payload the peer legitimately
receives makes the accounting independent of both the frame size and the window.

Solution

The addition is deliberately symmetric with data_frame_budget:

  • a field and a data_frame_overhead_threshold() setter on both server::Builder and
    client::Builder, right next to data_frame_budget();
  • plumbed through proto::Config into streams::Config;
  • Counts reads the threshold from config instead of the constant, in both
    record_data_frame and release_data_frame.

The default is unchanged. Anyone who does not call the setter gets
DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD (256) and byte-identical behaviour. 0 is rejected
with a panic, since it would disable the charge for every payload size.

Lowering the threshold does not weaken the empty-DATA-frame limit. Empty frames are
counted against MAX_RECV_EMPTY_DATA_FRAMES and never touch this budget, so the number a
peer may send is identical for every threshold. That is asserted rather than assumed, since
it is the property someone reviewing a lowered threshold will want to check first.

Tests

Four tests next to the existing budget tests, covering both directions:

  • a_lowered_threshold_stops_charging_frames_at_or_above_it — the point of the setting;
  • a_lowered_threshold_still_charges_frames_below_it — shortening the charged range must
    not turn the charge off for the sizes that remain inside it;
  • released_frames_replenish_at_the_configured_threshold — record and release must use the
    same threshold, or the budget drifts over the life of the connection;
  • the_threshold_does_not_change_the_empty_data_frame_limit — checked across
    [1, 16, 256, 1024].

cargo test --lib passes (446 tests), cargo fmt --check is clean, and cargo clippy --all-features reports the same warnings as master (no new ones).

I am happy to adjust the naming, the doc wording, or the panic-vs-Result choice for 0 to
whatever fits the crate's conventions.

`data_frame_budget` is configurable, but the payload length below which a
DATA frame is charged against it is a fixed 256 bytes. A peer that
legitimately receives many small DATA frames — a server-sent event stream
whose events are tens to low hundreds of bytes each is the ordinary case —
is charged on every frame, and cannot express "frames this small are normal
here" other than by raising the budget.

Raising the budget does not express it cleanly, because whether such a
stream survives depends on the connection window as well as the frame size.
Frames of payload `L < T` each consume `T - L` until the application reads
them, and the number that can sit unread is bounded by the connection window
`W`, so the worst-case simultaneous charge is `W * (T - L) / L`. With the
default `Auto` budget `B = max(W / 2, 25_600)`, for `W >= 51_200` the stream
avoids the budget only when

    W * (T - L) / L <= W / 2   <=>   2T <= 3L   <=>   L >= (2/3) * T

`W` cancels, which is the improvement `Auto` already made — but the residual
cutoff is `(2/3) * 256`, about 171 bytes, and it is a property of `T`, not
something the budget can move. Any budget a user picks to get under it is
back to being a number derived from `W`.

Making `T` itself configurable removes both dependencies: frames at or above
`T` are never charged at all, so setting `T` at or below the smallest payload
the peer legitimately receives makes the accounting independent of both the
frame size and the window, and it stays correct if someone later widens the
window.

The addition is symmetric with `data_frame_budget`: a field on both builders,
plumbed through `proto::Config` into `streams::Config` and `Counts`, which
now reads the threshold from config instead of the constant. The default is
unchanged (`DEFAULT_DATA_FRAME_OVERHEAD_THRESHOLD`, 256), so behaviour for
anyone who does not call the setter is byte-identical. `0` is rejected,
since it would disable the charge for every payload size.

Lowering the threshold does not weaken the empty-DATA-frame limit: empty
frames are counted against `MAX_RECV_EMPTY_DATA_FRAMES` and never touch this
budget, so the number a peer may send is the same for every threshold. That
is asserted rather than assumed.

Four tests are added next to the existing budget tests: frames at or above a
lowered threshold stop being charged, frames below it are still charged (and
still exhaust the budget), record and release use the same threshold so the
budget does not drift, and the empty-frame limit is identical across
thresholds.
@seanmonstar

Copy link
Copy Markdown
Member

Thanks for the write-up!

Though, I'm not sure adjusting the overhead threshold is something that should be configurable. It's meant to represent internally how much extra cost in resources holding onto a frame takes, especially when it doesn't other "cost" the window size. For instance, if we can do some work in the future to reduce the memory layout of Event, it would internally mean less cost (and we adjust internally too).

Why is adjusting the budget itself not sufficient?

@tatsuiman

Copy link
Copy Markdown
Author

Thanks for taking a look.

Raising the budget does work numerically — the part I couldn't make safe is that the value you have to pick is a function of the connection window.

For a payload L < T each unread frame holds T - L, and the number that can sit unread is bounded by the connection window W, so the worst case is W * (T - L) / L. Surviving on budget alone needs B >= W * (T - L) / L. With Auto's B = max(W / 2, 25_600) that reduces to L >= (2/3) * T, about 171 bytes; below that, the required B grows with W. So a budget chosen this way is only correct for the window it was computed against — widen the window later, or let a peer's SETTINGS move it, and the same constant silently becomes too small. Nothing fails loudly: the connection just starts dying under traffic it used to carry.

The other reason is granularity. Raising B relaxes the accounting for every frame size at once, including the smallest ones the limit exists to catch. Lowering T only shortens the range that is charged at all and leaves everything below it charged exactly as before — which is what the [1, 16, 256, 1024] test is asserting about the empty-DATA-frame limit.

On T being an internal cost model: I agree, and I don't have a strong claim that applications should be tuning it. What I actually need is for the accounting not to depend on W. If you'd rather keep T private, something like a budget variant parameterised by the smallest payload the application expects would work just as well for me — the app states L, h2 does the W * (T - L) / L arithmetic internally, and T never leaves the crate. Happy to rework it that way, or to close this if you'd rather solve it differently.

One caveat I should state plainly: I have not characterised the payload-size distribution of the streams involved, so I'm not claiming a specific typical size — only that they can fall below the (2/3) * T cutoff.

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