fix(dot/network): fix memory allocations with sizedBufferPool

[qdm12]

Changes

• Use a slice instead of an array, since returning the array was effectively copying it

Tests

go test -benchmem -run=Benchmark_sizedBufferPool github.com/ChainSafe/gossamer/dot/network

Issues

• Related to Profile memory usage and describe plan of action to address issues #1936 and found by reviewing PR dot/network: create blockResponseBuf pool #1913

Primary Reviewer

• Anyone is welcome

[danforbes]

Is there any way to quantify the impact/improvement this has on the performance of the code?

[qdm12]

Is there any way to quantify the impact/improvement this has on the performance of the code?

Not really, although it is now significantly better, see benchmark results. Since that piece was used for all dot networking, it could either had a very bad impact or it would had been barely noticeable. Only way to find out is to run it 😄 (I mean it, and check memory usage over time, profiling won't help really here I think)

[danforbes]

Benchmark results are super quantifiable...thanks, @qdm12 🙏🏻

[qdm12]

@danforbes but let's not get too excited. Go's runtime is pretty efficient even with bad code, so there might not be any concrete difference sadly. Anyway, one can only hope 🌟 !

[codecov]

Codecov Report

Merging #1963 (ba211cf) into development (0bc4bf1) will increase coverage by 0.19%.
The diff coverage is 61.18%.

@@               Coverage Diff               @@
##           development    #1963      +/-   ##
===============================================
+ Coverage        59.94%   60.13%   +0.19%     
===============================================
  Files              185      193       +8     
  Lines            26343    26531     +188     
===============================================
+ Hits             15790    15954     +164     
- Misses            8677     8693      +16     
- Partials          1876     1884       +8     

Flag	Coverage Δ
unit-tests	60.13% <61.18%> (+0.19%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/gossamer/profile.go	0.00% <0.00%> (ø)
cmd/gossamer/toml_config.go	27.27% <0.00%> (ø)
dot/config.go	26.76% <0.00%> (-1.46%)	⬇️
dot/network/message.go	69.26% <0.00%> (-2.49%)	⬇️
dot/network/sync.go	5.26% <0.00%> (ø)
dot/network/utils.go	57.89% <0.00%> (ø)
dot/rpc/subscription/listeners.go	71.92% <0.00%> (ø)
dot/state/block.go	43.52% <0.00%> (-0.11%)	⬇️
dot/state/bloom.go	0.00% <0.00%> (ø)
dot/state/offline_pruner.go	0.00% <0.00%> (ø)
... and 96 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde989d...ba211cf. Read the comment docs.

[noot]

@qdm12 how does the CPU usage get affected by this (if any)? as the main point of the buffer pool was to prevent unnecessary malloc's which used a lot of CPU

[qdm12]

@noot the previous implementation makes sense but was bugged. It would copy the array on every call to get() so effectively it was useless (the same as declaring a new array on every call). See the benchmark results commented below the benchmark.

I benchmarked using an array pointer and a slice (which has a pointer to the underlying data), and both gave similar results, so I decided to go with the slice for simplicity.

Side question, did we notice a performance bottleneck in this part? We should avoid such performance improvements without identifying a bottleneck and making sure there is a visible difference of performance (don't get me wrong, I love optimising 😄). That pool being bugged, it must had been slower than the original code, and we still used it; my point is we should be more careful about such optimisation changes in the future 👍

[noot]

@qdm12 sorry, did you bench the CPU or just memory? I guess more ops in the same amount of time = improved CPU performance? just want to be sure I'm reading it correctly. and yes, this was created because previously we were allocating a new buffer every time we received a network message, which was causing gossamer to use 100% CPU on every core basically immediately after starting. the CPU usage improved dramatically after adding this

[qdm12]

did you bench the CPU or just memory

It does both.

I guess more ops in the same amount of time

Yes. You can see before it was using a lot more time (and memory) per operation (op).

the CPU usage improved dramatically after adding this ... which was causing gossamer to use 100% CPU on every core basically immediately after starting

That doesn't make sense, since the array was copied to another memory location on every get request. It might had been something else resolving the 100% cpu performance issue. I also doubt the GC (or malloc) would use 100% of even a single CPU core, even under high load. Also the network is magnitudes slower than memory bandwidth.
For science's sake 🥼, and once we have something easy to deploy changes, I would be curious to try reverting it and check CPU usage 😄

[noot]

@qdm12 I'm just going based off the profiling lol, it showed malloc using up all the CPU it could previously. I can try to find an old commit for you to try to profile if you'd like ;)

[noot]

[redacted] nevermind, let me test this more

[github-actions]

🎉 This PR is included in version 0.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀