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.

Sign up for GitHub

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

Jump to bottom

Increase read buffer size to reduce poll system calls #66

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Increase read buffer size to reduce poll system calls #66

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion p2phttp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import (
"net"
"net/http"

"github.com/libp2p/go-libp2p-core/network"
gostream "github.com/libp2p/go-libp2p-gostream"

"github.com/libp2p/go-libp2p-core/host"
Expand Down Expand Up @@ -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.

if err != nil {
return resp, err
}
Expand Down