Skip to content
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

lib: window-based flow control #1040

Merged
merged 1 commit into from
Oct 1, 2021
Merged

lib: window-based flow control #1040

merged 1 commit into from
Oct 1, 2021

Conversation

junhochoi
Copy link
Contributor

This PR reimplments #529 which tried to implement a QUIC flow
control specified in https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing

It's basically window-based approach where window size is determined
by current receiving speed.

Since lot of things changed from #529, I tried to minimize the
changes in the existing code. Most of the logic is in src/flowcontrol.rs
and it will replace max_data and max_data_next logic.

@junhochoi junhochoi requested a review from a team as a code owner September 10, 2021 04:20
@junhochoi
Copy link
Contributor Author

junhochoi commented Sep 10, 2021

This PR mostly improves client download performance.

[2021-09-10T04:42:46.167704000Z INFO  quiche_apps::common] 1/1 response(s) received in 2.514392853s, closing...

this branch: quiche-client download 100MB file: 1.328s

[[2021-09-10T05:03:12.352234000Z INFO  quiche_apps::common] 1/1 response(s) received in 1.328804393s, closing...

For comparison, h2load with quic patch on the same client:

$ h2load --npn-list h3-29 -n 1 https://server:8443/100m.dat
finished in 1.19s, 0.84 req/s, 84.23MB/s

@junhochoi junhochoi force-pushed the 1374.fc_update branch 3 times, most recently from 9d4cf63 to 4f1b5fc Compare September 10, 2021 15:37
@junhochoi junhochoi changed the base branch from master to need_fma September 10, 2021 22:29
@junhochoi junhochoi force-pushed the 1374.fc_update branch 2 times, most recently from 012601a to 92e025a Compare September 12, 2021 05:39
Base automatically changed from need_fma to master September 13, 2021 11:59
@junhochoi junhochoi force-pushed the 1374.fc_update branch 2 times, most recently from e24c972 to a937363 Compare September 22, 2021 22:26
src/lib.rs Show resolved Hide resolved
@@ -49,6 +52,12 @@ const SEND_BUFFER_SIZE: usize = 5;
#[cfg(not(test))]
const SEND_BUFFER_SIZE: usize = 4096;

// The default size of the receiver stream flow control window.
const DEFAULT_STREAM_WINDOW: u64 = 32 * 1024;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this default override the configuration value set by the application in e.g. set_initial_max_stream_data_bidi_local() and set_initial_max_stream_data_bidi_remote()?

Maybe instead we need to take into account those values. And that would mean we need separate flow control behaviour for bidi streams initiated locally, bidi streams initiated remotely, and uni stream initiated remotely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEFAULT_{CONNECTION,STREAM}_WINDOW is a different value from initial stream/connection max_data. in the flowcontrol.rs there is window variable is limited by those default values and (so, max_data will be consumed + window). Since those are hardcoded, it's a good idea to make it configurable, but don't be confused with max_data transport parameter which defines initial limit only and nothing to do with window size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added config.set_max_connection_window() and config.set_max_stream_window() API to set the max window size of the connection and the stream.

Copy link
Member

@ghedo ghedo Sep 29, 2021

Choose a reason for hiding this comment

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

I'm still confused. In the code we have cmp::min(max_data, DEFAULT_STREAM_WINDOW), which means tha if the application sets a lower initial stream data value (i.e. max_data) than the "default" window value and a lower max window value, we will just ignore the application-set values and simply use DEFAULT_STREAM_WINDOW as the initial window size. And later, when autotune is done, the new window will just be max_window.

So it's still not very clear to me what the point of this constant is. Why not just use max_data without doing the cmp::min()?

Copy link
Member

Choose a reason for hiding this comment

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

And while we are at it, why even do autotuning at all, and not just use max_window from the start?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I just realized I was probably confusing min with max 🤦 ...

Copy link
Contributor Author

@junhochoi junhochoi Sep 29, 2021

Choose a reason for hiding this comment

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

cmp::min() is used for flow control window initialization when the max_data (whether it's connection or stream) is too small (e.g. tests) in such case, having a small window will make sense and it's helpful for passing most of existing tests without a large modification (I only changed 2 tests). In practive it doesn't have much meaning.

Copy link
Contributor Author

@junhochoi junhochoi Sep 29, 2021

Choose a reason for hiding this comment

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

And while we are at it, why even do autotuning at all, and not just use max_window from the start?

Interesting idea! I think the reason to have a flow control window is to properly manage receiver buffer size. If it's too high each connection/stream might need more memory even for a small BDP connection and MAX_DATA update might not be delivered in time.

I see some of implementation use a very high max_data in the beginning (e.g. h2load + quic's initial max_data is 2^30-1 and window size is 2^26-1, but it's a test tool). In practical use most of them takes windowed approach - I believe it's better to tune the window size dynamically.

src/lib.rs Show resolved Hide resolved
src/flowcontrol.rs Show resolved Hide resolved
src/flowcontrol.rs Show resolved Hide resolved
src/flowcontrol.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@junhochoi junhochoi force-pushed the 1374.fc_update branch 3 times, most recently from 9a1be42 to ace1b0d Compare September 29, 2021 06:20
This PR reimplments #529 which tried to implement a QUIC flow
control specified in https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing

It's basically window-based approach where window size is determined
by current receiving speed.

Since lot of things changed from #529, I tried to minimize the
changes in the existing code. Most of the logic is in src/flowcontrol.rs
and it will replace max_data and max_data_next logic.

To set up maximum window size, config.set_max_connection_window() and
config.set_max_stream_window() API is added.
@ghedo ghedo merged commit e79ea93 into master Oct 1, 2021
@ghedo ghedo deleted the 1374.fc_update branch October 1, 2021 10:37
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