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

pull: Lower max concurrent HTTP requests to 2 #3065

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

I believe this may help re-enabling the libcurl low-speed limit checks in the case where we have a full 8 HTTP requests outstanding with the remote server, but it's actually throttling us.

In practice, we really don't need more than two. Experimenting with this using the builtin Go HTTP server, this actually seems to increase performance; but I get pretty wild timing variations overall.

I believe this may help re-enabling the libcurl low-speed limit
checks in the case where we have a full 8 HTTP requests outstanding
with the remote server, but it's actually throttling us.

In practice, we really don't need more than two.  Experimenting
with this using the builtin Go HTTP server, this actually seems
to increase performance; but I get pretty wild timing variations
overall.
@cgwalters
Copy link
Member Author

Ah yes of relevance here, this looks like the librepo equivalent: https://github.com/rpm-software-management/librepo/blob/27cb3114ccce72814df305aa7342ee5bc88e3ac0/librepo/handle.h#L72

@jmarrero
Copy link
Member

jmarrero commented Oct 3, 2023

This looks good to me, we can also just make it configurable in the future if for some reason this becomes an issue.

@dbnicholson
Copy link
Member

This seems like a pretty significant change. We've been operating the soup backend with 8 concurrent requests for years. Do you have any data to back up this change?

@jmarrero jmarrero requested review from jmarrero and removed request for jmarrero October 3, 2023 01:26
Copy link
Member

@jmarrero jmarrero left a comment

Choose a reason for hiding this comment

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

let's have the above discussion before merging.

@cgwalters
Copy link
Member Author

This seems like a pretty significant change. We've been operating the soup backend with 8 concurrent requests for years.

One thing to bear in mind here is that in practice the important thing is to keep the "pipeline" full - and we're still going to be doing that in general.

Do you have any data to back up this change?

As I said in the commit message when using locally, it overall improves performance - although this is with libcurl.

What I think would be really helpful is to set up a CI/test flow that better reproduces some "real world" conditions without actually hitting up the internet.

we can also just make it configurable in the future if for some reason this becomes an issue.

Yes agree, we should make this configurable.

@cgwalters cgwalters marked this pull request as draft October 3, 2023 14:06
@dbnicholson
Copy link
Member

This seems like a pretty significant change. We've been operating the soup backend with 8 concurrent requests for years.

One thing to bear in mind here is that in practice the important thing is to keep the "pipeline" full - and we're still going to be doing that in general.

Can you explain that? What do "in practice" and "in general" mean in this context?

@cgwalters
Copy link
Member Author

We'll make this part of #2843

@cgwalters cgwalters closed this Oct 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants