Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

use io.CopyBuffer with explicitly allocated buffers #69

Merged
merged 4 commits into from
Apr 8, 2019
Merged

Conversation

vyzo
Copy link
Contributor

@vyzo vyzo commented Apr 7, 2019

io.Copy uses a 32KB buffer, which as pprof indicates results in significant memory usage.
This changes to using io.CopyBuffer directly with 4KB buffers.

@vyzo vyzo requested review from Stebalien and raulk April 7, 2019 12:28
@ghost ghost assigned vyzo Apr 7, 2019
@ghost ghost added the status/in-progress In progress label Apr 7, 2019
io.Copy uses a 32KB buffer, which pprof indicates result in significant memory usage.
@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

The 4096B buffer LGTM as in practice the max MTU for Ethernet networks is 1500B. I think we disable Nagle (I could be mistaken), so we can do with smaller buffers. But my assumptions could be wrong.

However, I think we would benefit from using a sync.Pool here to further curtail memory thrashing and GC pressure. WDYT?

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Yeah, we could use a sync.Pool, steb made the same comment. Does it ever return memory? I am worried about hoarding memory when we scale up.

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

reading the docs, it explicitly mentioned that pooled objects may be gc'ed which is nice.

@raulk
Copy link
Member

raulk commented Apr 7, 2019

Yeah, it holds weak references, so unused elements are elegible for collection by the GC :-)

@vyzo
Copy link
Contributor Author

vyzo commented Apr 7, 2019

Added a pool.

relay.go Outdated Show resolved Hide resolved
relay.go Outdated
@@ -85,6 +87,11 @@ func NewRelay(ctx context.Context, h host.Host, upgrader *tptu.Upgrader, opts ..
incoming: make(chan *Conn),
relays: make(map[peer.ID]struct{}),
liveHops: make(map[peer.ID]map[peer.ID]int),
bufPool: sync.Pool{
New: func() interface{} {
return make([]byte, 4096)
Copy link
Member

Choose a reason for hiding this comment

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

So, welcome to go. Converting a []byte to an interface{} allocates. Let's just use https://github.com/libp2p/go-buffer-pool (https://godoc.org/github.com/libp2p/go-buffer-pool#Get)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, that's nuts.
Yeah, I'll use go-buffer-pool.

@raulk raulk requested a review from Stebalien April 8, 2019 12:17
@vyzo vyzo merged commit e3f9b56 into master Apr 8, 2019
@vyzo vyzo deleted the feat/copy-buffer branch April 8, 2019 16:03
@ghost ghost removed the status/in-progress In progress label Apr 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants