-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add initial prototype of http over libp2p #2218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, except for the content length issue. Maybe creating a net.Listener
isn't that bad after all, at least we wouldn't have to deal that much with HTTP internals.
var bufWriterPool = sync.Pool{ | ||
New: func() any { | ||
return bufio.NewWriterSize(nil, 4<<10) | ||
}, | ||
} | ||
|
||
var bufReaderPool = sync.Pool{ | ||
New: func() any { | ||
return bufio.NewReaderSize(nil, 4<<10) | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably doesn't matter (and definitely not for a prototype): What about creating a new reader / writer, but taking the underlying the byte slice from our shared buffer pool? That's an additional alloc for the bufio.{Reader/Writer}
, but we get better byte slice reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We get to reuse the whole thing here which is probably net better for the GC.
closing this in favor of #2438 |
This doesn't handle all the edge cases (like expect continue) and trailers. Maybe less code than implementing a net.Listener? At least it's one goroutine.
Might switch to a net.Listener later.