Skip to content
This repository has been archived by the owner on Feb 1, 2023. It is now read-only.

Fix unbounded buffer #7

Closed
wants to merge 1 commit into from
Closed

Conversation

cannium
Copy link

@cannium cannium commented Aug 29, 2018

In the original code, buffer toProvide is unbounded, also it could possibly leak memory because reslice(toProvide = toProvide[1:]) will not cause the array under the hood to be GCed.

@cannium cannium changed the title Fix unbounded memory buffer Fix unbounded buffer Aug 29, 2018
@whyrusleeping
Copy link
Member

So, this is intended. It used to do what this PR does, but that just results in unbounded growth of goroutines, which are wayyyyyyyyy more expensive than an item in the array.

Really, we just need to separate providing from bitswap altogether.

cc @magik6k @Stebalien

@cannium
Copy link
Author

cannium commented Aug 30, 2018

I'm confused about the "unbounded goroutine" part... Intuitively, a buffer only slows the creation of new goroutines, but overall load pressure remains. I might need a little more digging of the code and come back to the discussion.

@Stebalien
Copy link
Member

Ah, I think the issue here is that we don't want to block the newBlocks channel. That will back up the entire system.

If you're feeling adventurous, you could try using a slice-backed deque (that's the correct datastructure but, unfortunately, we're using go so you'll have to write your own).

As @whyrusleeping says, the correct solution here is to not buffer these in-memory at all and provide out-of-band.

@Stebalien
Copy link
Member

Actually, I've been informed that this does not, in fact leak too much memory. That is, it may (temporarily) leak a few CIDs but as we shift off the front and append to the back, go will reallocate the slice and GC the unused portion. That is, when we append(slice, element) while at capacity, go will reallocate the slice but will not allocate space for (or copy) the elements that we've "shifted" off the front of slice.

@cannium
Copy link
Author

cannium commented Sep 4, 2018

@Stebalien You're right. But it's still unbounded and can grow indefinitely. If needed, you could close this pull request for now. I think I might need to think with a big picture in mind as instructed.

@Stebalien
Copy link
Member

You're right, it can. However, in this particular case, we send blocks on this channel when we receive them from the network. If we didn't buffer here, we'd end up piling up all the goroutines handling these new blocks (see Bitswap.ReceiveMessage).

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