dot/network: create blockResponseBuf pool

[zhuber6]

Changes

• Created pool for blockResponseBuf and updated message size to work for both pools.

Tests

go test ./dot/network

Issues

-Fixes #1858

Primary Reviewer

• @noot

[noot]

can you change this back? BlockResponseMessages are significantly larger than all other messages, so other messages won't use this much space, and this will increase the memory usage unneedlessly I think

[zhuber6]

Okay I appreciate the feedback, I wasn't quite sure of the repercussions of increasing the size that much. I will see if I can have one buffer pool handler but different sizes. Thanks

[noot]

remove

[noot]

I'd rather leave the maxMessageSize as it is and create another buffer pool that deals with [maxBlockResponseSize]byte buffers. unfortunately this will involve copy-pasting the existing buffer pool code and changing the size afaik, maybe there's a nicer way to do that but not sure

[qdm12]

@noot why do we have this sizedBufferPool? Can't we just use a sync.Pool? Lock contention wise, the sync.Pool scales much better than using channels or mutexes.

[noot]

@qdm12 cause stuff gets garbage collected from sync.Pool :( and we don't want to be re-malloc-ing at runtime, as it's very CPU intensive

[qdm12]

@noot @zjhuber is there a point in doing this? We were using a single shared byte slice as buffer and it was working fine right? Unless the implementation was wrong before and multiple threads were accessing that buffer erroneously, then yes a sizedBufferPool makes sense. What do you thinK?

[qdm12]

nit just some small simplifications 😉

Suggested change

	var blockResponseBufPool *sizedBufferPool
	if cfg.noPreAllocate {
	blockResponseBufPool = &sizedBufferPool{
	c: make(chan *[maxMessageSize]byte, cfg.MinPeers*3),
	}
	} else {
	blockResponseBufPool = newSizedBufferPool(cfg.MinPeers*3, cfg.MaxPeers*3)
	}
	poolSize := 3 * cfg.MaxPeers
	var preAlloc int
	if !cfg.noPreAllocate {
	preAlloc = 3 * cfg.MaxPeers
	}
	blockResponseBufPool := newSizedBufferPool(preAlloc, poolSize)

[noot]

@qdm12 honestly the code works fine as it is with the 1 buffer used for block responses, so I don't know if this is necessary. it would only be needed for the case when we're syncing at the tip and have multiple requests going and want to receive them all concurrently, but I don't think that even is needed as they aren't processed concurrently

[noot]

gonna close this for now as I don't actually think it's needed. sorry about that!