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

Increase read buffer size to reduce poll system calls #66

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

aarshkshah1992
Copy link
Contributor

No description provided.

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Mar 10, 2021

@hsanjuan Merging this small change. Please let me know if there are any objections and I'll fix them in a later PR.

@aarshkshah1992 aarshkshah1992 merged commit 8592544 into master Mar 10, 2021
@aarshkshah1992 aarshkshah1992 deleted the feat/buffer-size branch March 10, 2021 03:35
@@ -150,7 +151,7 @@ func (rt *RoundTripper) RoundTrip(r *http.Request) (*http.Response, error) {
}
}()

resp, err := http.ReadResponse(bufio.NewReader(conn), r)
resp, err := http.ReadResponse(bufio.NewReaderSize(conn, network.MessageSizeMax), r)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@aarshkshah1992 The 4MB here seems like an arbitrary value?. If so, I don't fully see the need to bring it from core/network (it's an indirection that just hides the actual number).

So why 4 MB? I don't expect the average http response to be 4MB in my usecases. Do you in you usecases?

Also would this means that handling 10 concurrent streams is going to need 40MB of memory allocated rather than 4? That would not seem a good thing when running on small devices or VPCs with 512MB of ram.

Copy link

Choose a reason for hiding this comment

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

this should be user configurable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aarshkshah1992 I have reverted the commit. Next time, please do not directly merge and release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hsanjuan Sorry ! My bad.

@aschmahmann aschmahmann mentioned this pull request Aug 23, 2021
62 tasks
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