Improve *deduplicatingSlice.ingest performance
#4037
Conversation
I had to perform the initialization within the for loop as otherwise it always panics.
inkel
force-pushed
the
inkel/deduplicating-slice-ingest-improvements
branch
from
f0c1d89 to
e7472e0
Compare
There was a problem hiding this comment.
Thanks for sharing your work!
Not too sure if you are aware, but using -memprofile, you can also generate a profile of your micro benchmark to see how it performs line by line: (left is before this PR right is after)
.
I think the best benchmark test would, be to run pyroscope for 30 min with ingestion real profiles and compare its memory profiles before and after.
| @@ -279,6 +279,7 @@ func (s *deduplicatingSlice[M, K, H]) ingest(elems []M, rewriter *rewriter) { | |||
| if len(missing) > 0 { | |||
| s.lock.Lock() | |||
| posSlice := int64(len(s.slice)) | |||
| misSlice := make([]M, 0, len(missing)) | |||
There was a problem hiding this comment.
Rather than using the extra slice you can also just grow the underlying slice, you might overgrow it but you can get rid of misSlice entirely.
| misSlice := make([]M, 0, len(missing)) | |
| s.slice = slices.Grow(s.slice, len(missing)) |
There was a problem hiding this comment.
Ah, that's probably even better! I'll refactor to use this and see what the benchmark says.
There was a problem hiding this comment.
Did you mean slices.GrowLen instead? Anyway, I've tested calling this function and then comparing both results and got this:
$ benchstat bench.pw-writeprofilesymbols-misslice.log bench.pw-writeprofilesymbols-slicesgrowlen.log
goos: darwin
goarch: arm64
pkg: github.com/grafana/pyroscope/pkg/phlaredb/symdb
cpu: Apple M1 Pro
│ bench.pw-writeprofilesymbols-misslice.log │ bench.pw-writeprofilesymbols-slicesgrowlen.log │
│ sec/op │ sec/op vs base │
PartitionWriter_WriteProfileSymbols-10 504.0µ ± 1% 515.7µ ± 1% +2.34% (p=0.001 n=20)
│ bench.pw-writeprofilesymbols-misslice.log │ bench.pw-writeprofilesymbols-slicesgrowlen.log │
│ B/op │ B/op vs base │
PartitionWriter_WriteProfileSymbols-10 600.8Ki ± 0% 714.7Ki ± 0% +18.95% (p=0.000 n=20)
│ bench.pw-writeprofilesymbols-misslice.log │ bench.pw-writeprofilesymbols-slicesgrowlen.log │
│ allocs/op │ allocs/op vs base │
PartitionWriter_WriteProfileSymbols-10 2.176k ± 0% 2.206k ± 0% +1.38% (p=0.000 n=20)
Surprisingly, growing s.slice is actually slower and consumes more memory 🤔
There was a problem hiding this comment.
No GrowLen is not the same function, as it will take the total length of slices, rather than how much more space should be available, you either need to adapt the size of that or import origslices "slices" and use slices.Grow(xx)
| @@ -254,7 +254,7 @@ func (s *deduplicatingSlice[M, K, H]) Size() uint64 { | |||
|
|
|||
| func (s *deduplicatingSlice[M, K, H]) ingest(elems []M, rewriter *rewriter) { | |||
| var ( | |||
| rewritingMap = make(map[int64]int64) | |||
There was a problem hiding this comment.
Good find, this will give us a right sized map every time.
Ah, yes! I used it locally and that's how I arrived to the
Ooh this would definitely be The Dream™. I'm going to apply your suggestion for refactoring the benchmark and then see how it affects the results I have. Thanks for the review so far! 🫶 |
Thank you @simonswine for the suggestion! Co-authored-by: Christian Simon <simon@swine.de>
By doing this we avoid having to regrow the map, which improves CPU and memory consumption.
Instead of checking `s.slice` on each call to append and regrow if necessary, create a temporary slice with the maximum possible capacity, and append that slice to `s.slice` outside the for loop, effectively checking and regrowing `s.slice` only once. This further improves CPU and memory consumption.
|
@simonswine applying your changes to the benchmark it runs ~13% faster with the original code, so I'm going to reorder the commits and run the benchmarks again to get more realistic results. I'll update the PR accordingly. Thanks! |
inkel
force-pushed
the
inkel/deduplicating-slice-ingest-improvements
branch
from
95b0bec to
44cf7d9
Compare
|
I've found another thing that could be pre-initialized with a size that greatly affects the performance, and is the |
|
🤔 there's a failing test on CI, IIUC due to a context cancelled. I've run |
Yes some tests are flaky right now, sorry about that |
|
No worries! |
As in #4033 in the past hackathon I checked optimizations on Pyroscope ingester, and found a couple low hanging fruits that could speed up
*deduplicatingSlice.ingestmethod.The first thing I did was adding a benchmark for
*PartitionWriter.WriteProfileSymbolswhich was showing as one of the methods consuming most CPU and memory. Armed with this, I eventually found out that*deduplicatingSlice.ingeststand out as one of the methods that could be improved in a relatively easy way.The first was creating the
rewritingMapwith the most capacity it could hold. This already showed some gains in ~7% CPU and ~8% bytes/op reduction.Next thing I did was noticing that when iterating over missing elements we were calling
appendons.sliceevery time there was an element to add. By creating a temporary slice with an initial capacity oflen(missing)and size 0 and appending to that one, the runtime doesn't have to regrow the slice (if needed) on every iteration, and it only checks capacity and regrowss.sliceif needed outside the loop. This results in additional CPU and memory savings:Overall, with all these changes applied, the benchmark comparison is close to 20% CPU and 15% memory: