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

Http Client benchmark #388

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Http Client benchmark #388

wants to merge 32 commits into from

Conversation

TingDaoK
Copy link
Contributor

@TingDaoK TingDaoK commented Aug 26, 2022

  • Initial build of our http client benchmark
  • Test using a local host, that using our http client to connect to the host and collect how many requests are made during a certain time.
  • To run:

ISSUE FOUND & FIXED

  • We update the window for connection for each data frame received, which really slows us down for frequent small chunk of data frames receiving.

    • We fixed it by only update the connection window whenever it drops to 50% of the max.
    • Same issue may happens to streams windows
    • Or the padding of the connection window.
  • Providing tiny increments to flow control in WINDOW_UPDATE frames can cause a sender to generate a large number of DATA frames. from here

    • Is the client's responsibility to make sure not doing this? Even if the user do manual window update and doing small window update?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@TingDaoK TingDaoK changed the title Canary Http Client benchmark Aug 26, 2022
@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging 4cd7338 into f81ee94 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging f445de0 into f81ee94 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging f86a9f7 into f81ee94 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Sep 12, 2022

This pull request introduces 1 alert when merging ca0c7c5 into f81ee94 - view on LGTM.com

new alerts:

  • 1 for Variable defined multiple times

@TingDaoK TingDaoK marked this pull request as ready for review September 12, 2022 23:53
bin/canary/CMakeLists.txt Outdated Show resolved Hide resolved
bin/canary/CMakeLists.txt Outdated Show resolved Hide resolved
bin/canary/main.c Outdated Show resolved Hide resolved
@@ -1762,6 +1767,8 @@ static void s_handler_installed(struct aws_channel_handler *handler, struct aws_
aws_linked_list_push_back(
&connection->thread_data.outgoing_frames_queue, &connection_window_update_frame->node);
connection->thread_data.window_size_self += initial_window_update_size;
/* For automatic window management, we only update connectio windows when it droped blow 50% of MAX. */
connection->thread_data.window_size_self_dropped_threshold = AWS_H2_WINDOW_UPDATE_MAX / 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pull this magic number into a constant?

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