Set max concurrent uni streams accordingly -- do not over allocate open uni streams #1060

lijunwangs · 2024-04-25T21:41:06Z

Problem

We can over allocate max concurrent open uni streams than the total streams allowed within a throttle window.
For example, an unstaked node might be eligible for 2 uni streams per throttle window, but we might allocate the 128 concurrent uni streams for it.

This has two problems: allocating resources on the server side unnecessarily and allow the client to open concurrent uni streams which will be throttled on server side, more timeout error on client side and more load on the server side.

Summary of Changes

Do not allow more open concurrent uni streams than the one permitted in a throttle window.

Fixes #

pgarg66

Can we split the new functionality in its own struct, and add unit tests for the math?

…re uni streams than necessary

codecov-commenter · 2024-05-09T00:15:09Z

Codecov Report

Attention: Patch coverage is 97.29730% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 82.1%. Comparing base (2bc026d) to head (9b5c06c).
Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #1060     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         893      893             
  Lines      236600   236677     +77     
=========================================
+ Hits       194429   194451     +22     
- Misses      42171    42226     +55

lijunwangs · 2024-05-09T01:50:29Z

Can we split the new functionality in its own struct, and add unit tests for the math?

Done.

pgarg66 · 2024-05-09T16:19:25Z

streamer/src/nonblocking/quic.rs

+    ) -> u64 {
+        let max_streams_per_throttle_window =
+            ema.available_load_capacity_in_throttling_duration(peer_type, total_stake);
+        (UniStreamQosUtil::compute_max_allowed_uni_streams(peer_type, total_stake) as u64)


nit: UniStreamQosUtil::compute_max_allowed_uni_streams could be replaced with Self::compute_max_allowed_uni_streams

pgarg66 · 2024-05-09T16:26:39Z

streamer/src/nonblocking/quic.rs

+
+    /// Given the max_streams_per_throttling_interval, derive the streams per throttle window.
+    /// Do not allow concurrent streams more than the max streams per throttle window.
+    pub fn max_concurrent_uni_streams_per_throttling_interval(


Do we really need a function for this? It's just a wrapper on min. Why not directly use min() where we are calling this?

I think this makes the design goal more explicit and easier to do test

pgarg66 · 2024-05-09T16:38:30Z

streamer/src/nonblocking/quic.rs

@@ -811,8 +844,15 @@ async fn handle_connection(
        stats.total_streams.load(Ordering::Relaxed),
        stats.total_connections.load(Ordering::Relaxed),
    );
+    connection.set_max_concurrent_uni_streams(max_uni_streams);
+    if let Some(receive_window) = receive_window {


Any benefit of moving receive_window setting to this function?

It encapsulates better to put QOS related config for connections in one place (receive window and max concurrent uni streams.

pgarg66 · 2024-05-09T16:47:25Z

streamer/src/nonblocking/quic.rs

@@ -856,6 +896,20 @@ async fn handle_connection(
                            sleep(throttle_duration).await;
                        }
                    }
+                    let max_concurrent_uni_streams =


nit: max_concurrent_uni_streams is very overloaded here. Can we simplify the code? Maybe we if use min() instead of UniStreamQosUtil::max_concurrent_uni_streams_per_throttling_interval(), the code could be compressed and simplified.

I renamed the variables a little to clarify

pgarg66 · 2024-05-09T16:48:27Z

Some nits. Otherwise the logic looks good to me.
Let's wait for @alessandrod to also take a look.

pgarg66

LGTM. Please give @alessandrod a chance to look at it before merging.

lijunwangs · 2024-05-09T22:11:33Z

@alessandrod -- this is the formal change for the experiments we have done in '3gG' where we hard coded concurrent streams to 1 for unstaked connection based on the calculation of max allowed streams count per throttle window. This change makes the stream count limit to take the minimum of (original max current uni streams per stake, max allowed streams per throttle window).

alessandrod · 2024-05-13T14:45:51Z

Oops sorry I had missed this! I'll take a look in the morning

lijunwangs · 2024-05-17T17:43:53Z

Oops sorry I had missed this! I'll take a look in the morning

Hi @alessandrod , any further comments on this PR? I'd like to wrap it up

alessandrod · 2024-05-18T00:15:46Z

From what I understand, I don't think this code is needed, adds complexity and probably some round trips to communicate the new limit to peers. (And locking and wakeups of the connection task, but admittedly those should be infrequent). I could be wrong of course, but I haven't seen any plausible explanation of why this is needed (or multiple streams are needed to begin with).

lijunwangs · 2024-05-18T03:09:21Z

From what I understand, I don't think this code is needed, adds complexity and probably some round trips to communicate the new limit to peers. (And locking and wakeups of the connection task, but admittedly those should be infrequent). I could be wrong of course, but I haven't seen any plausible explanation of why this is needed (or multiple streams are needed to begin with).

Can you clarify why it is not needed? I think you would agree over allocate is bad. If you are questioning why we have multiple streams in the first place. As I mentioned it is based on the thinking of reducing head of line issue and 2. for better performance using parallelism. And I mentioned in the slack channel that 1 stream vs the current default there is at least 3 times of difference in bench-tps test. Your point of multiple stream may cause fragmentations among the stream is a valid point. But it is orthogonal to this PR. I am not making it worse. If you are proposing just change stream count to 1 for everything, I am thinking it is too drastic change. Please explicit if you are proposing something different.

alessandrod · 2024-05-18T05:28:12Z

From what I understand, I don't think this code is needed, adds complexity and probably some round trips to communicate the new limit to peers. (And locking and wakeups of the connection task, but admittedly those should be infrequent). I could be wrong of course, but I haven't seen any plausible explanation of why this is needed (or multiple streams are needed to begin with).

Can you clarify why it is not needed? I think you would agree over allocate is bad.

Over allocate what? From what I understand - and again I could be wrong - we're not allocating anything more on the server side whether we allow 1 stream or 1000 streams. The max streams limit is a protocol limit that is communicated to the client. The server doesn't pre-reserve anything about it. Just the client will stop opening streams once it runs out of streams. Stream id tracking is done in the client, it requires no synchronization with the server. We have one task per connection. We read one stream at a time - not in parallel: we pop the next stream from the connection and process it. What gets over allocated?

The thing that changes how much the server allocates is the receive window, and we already bound that.

If you are questioning why we have multiple streams in the first place. As I mentioned it is based on the thinking of reducing head of line issue and 2. for better performance using parallelism.

I thought we agreed on slack that there's no HOL issue? If you don't agree, can you explain to me where the HOL issue is exactly?

And can you explain where the parallelism is, if the server has one task per connection and pops one stream at a time? How is parallelism increased exactly?

And I mentioned in the slack channel that 1 stream vs the current default there is at least 3 times of difference in bench-tps test. Your point of multiple stream may cause fragmentations among the stream is a valid point. But it is orthogonal to this PR. I am not making it worse.

You are adding code that I don't think is necessary. More code is always bad: more complexity and more bugs. In that sense, it's worse. Obviously I can be wrong, but I'd like to know how I'm wrong if you want me to approve the PR.

If you are proposing just change stream count to 1 for everything, I am thinking it is too drastic change. Please explicit if you are proposing something different.

I'm not proposing to set streams to 1 in the context of this PR. I do think we should do it, but as you said it's orthogonal to this PR and likely requires fixes in the client before we can do it.

lijunwangs · 2024-07-15T19:39:42Z

From what I understand, I don't think this code is needed, adds complexity and probably some round trips to communicate the new limit to peers. (And locking and wakeups of the connection task, but admittedly those should be infrequent). I could be wrong of course, but I haven't seen any plausible explanation of why this is needed (or multiple streams are needed to begin with).

Can you clarify why it is not needed? I think you would agree over allocate is bad.

Over allocate what? From what I understand - and again I could be wrong - we're not allocating anything more on the server side whether we allow 1 stream or 1000 streams. The max streams limit is a protocol limit that is communicated to the client. The server doesn't pre-reserve anything about it. Just the client will stop opening streams once it runs out of streams. Stream id tracking is done in the client, it requires no synchronization with the server. We have one task per connection. We read one stream at a time - not in parallel: we pop the next stream from the connection and process it. What gets over allocated?

Lijun: To follow up. I mentioned in the PR description:
This has two problems: allocating resources on the server side unnecessarily and allow the client to open more concurrent uni streams which will be throttled on server side, more timeout error on client side and more load on the server side. My simple intuition was: if we allow to max N streams in a throttle window, do not allow the max concurrent open streams to be more than N, as the ones exceeding that will be throttled anyway. Open uni streams does require allocating resources to maintain the states.

lijunwangs requested review from pgarg66 and alessandrod April 26, 2024 00:52

lijunwangs force-pushed the do_not_over_allocate_streams branch 2 times, most recently from 00d93ce to 1f6108a Compare April 26, 2024 08:05

pgarg66 reviewed May 3, 2024

View reviewed changes

lijunwangs added 4 commits May 8, 2024 15:29

Set max concurrent uni streams accordingly -- do not over allocate mo…

69f0d55

…re uni streams than necessary

Refector the code and added unit test as per comments from Pankaj

bd7f4eb

A clippy issue

10d1ad5

fmt code

d21dab8

lijunwangs force-pushed the do_not_over_allocate_streams branch from 1c5fcf3 to d21dab8 Compare May 8, 2024 22:29

pgarg66 reviewed May 9, 2024

View reviewed changes

lijunwangs added 3 commits May 9, 2024 10:19

fmt code

6147668

fmt code

d3365eb

simplify code a little

9b5c06c

pgarg66 approved these changes May 9, 2024

View reviewed changes

lijunwangs requested a review from sakridge May 18, 2024 03:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set max concurrent uni streams accordingly -- do not over allocate open uni streams #1060

Set max concurrent uni streams accordingly -- do not over allocate open uni streams #1060

lijunwangs commented Apr 25, 2024

pgarg66 left a comment

codecov-commenter commented May 9, 2024 •

edited

Loading

lijunwangs commented May 9, 2024

pgarg66 May 9, 2024

lijunwangs May 9, 2024

pgarg66 May 9, 2024

lijunwangs May 9, 2024

pgarg66 May 9, 2024

lijunwangs May 9, 2024

pgarg66 May 9, 2024

lijunwangs May 9, 2024

pgarg66 commented May 9, 2024

pgarg66 left a comment

lijunwangs commented May 9, 2024

alessandrod commented May 13, 2024

lijunwangs commented May 17, 2024

alessandrod commented May 18, 2024

lijunwangs commented May 18, 2024

alessandrod commented May 18, 2024 •

edited

Loading

lijunwangs commented Jul 15, 2024

Set max concurrent uni streams accordingly -- do not over allocate open uni streams #1060

Are you sure you want to change the base?

Set max concurrent uni streams accordingly -- do not over allocate open uni streams #1060

Conversation

lijunwangs commented Apr 25, 2024

Problem

Summary of Changes

pgarg66 left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 9, 2024 • edited Loading

Codecov Report

lijunwangs commented May 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgarg66 commented May 9, 2024

pgarg66 left a comment

Choose a reason for hiding this comment

lijunwangs commented May 9, 2024

alessandrod commented May 13, 2024

lijunwangs commented May 17, 2024

alessandrod commented May 18, 2024

lijunwangs commented May 18, 2024

alessandrod commented May 18, 2024 • edited Loading

lijunwangs commented Jul 15, 2024

codecov-commenter commented May 9, 2024 •

edited

Loading

alessandrod commented May 18, 2024 •

edited

Loading