fix(godeltaprof): fix mutex tests on gotip #108

korniltsev · 2024-05-22T10:54:45Z

Fixes #103

This PR fixes an issue when a heap/mutex profile has a record with equal stack (by accumulating values for duplicate stacks before delta computation).
The issue was found in tests for mutex(golang started to produce two profile records for the same mutex ( see the linked issue for more info))
I'm not sure if it is possible to trigger the issue for heap.

The PR turned up big because of refactored test helpers for reuse. I can split PR into 2 or 3 smaller ones if needed.

This PR also drops support for go16, go17 as I wanted to use generics in prof/map.go

goos: darwin
goarch: arm64
pkg: github.com/grafana/pyroscope-go/godeltaprof/compat
                                      │   old.txt   │               new.txt                │
                                      │   sec/op    │    sec/op      vs base               │
HeapCompression-8                       691.1µ ± 0%    729.8µ ± 65%   +5.59% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   640.6µ ± 1%    693.8µ ±  9%   +8.30% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   643.2µ ± 1%    690.3µ ±  1%   +7.33% (p=0.002 n=6)
HeapDelta-8                             76.26µ ± 1%   113.92µ ±  2%  +49.38% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         48.98µ ± 1%    81.11µ ±  2%  +65.59% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         49.03µ ± 1%    81.14µ ±  1%  +65.49% (p=0.002 n=6)
geomean                                 193.3µ         253.0µ        +30.87%

                                      │   old.txt    │               new.txt               │
                                      │     B/op     │     B/op      vs base               │
HeapCompression-8                       974.1Ki ± 0%   973.9Ki ± 0%   -0.03% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   974.6Ki ± 0%   974.3Ki ± 0%   -0.02% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   974.6Ki ± 0%   974.3Ki ± 0%   -0.03% (p=0.002 n=6)
HeapDelta-8                              333.00 ± 0%     53.00 ± 0%  -84.08% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8          312.00 ± 0%     30.00 ± 0%  -90.38% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8          312.00 ± 0%     30.00 ± 0%  -90.38% (p=0.002 n=6)
geomean                                 17.42Ki        5.874Ki       -66.28%

                                      │  old.txt   │              new.txt              │
                                      │ allocs/op  │ allocs/op   vs base               │
HeapCompression-8                       788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
HeapDelta-8                             2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
geomean                                 39.70        28.05       -29.33%

So it looks like the cost of deduplication is about 5% of overall time(including compression), which is not that bad.

korniltsev · 2024-05-22T11:03:33Z

yey, green light \M/

korniltsev · 2024-05-27T11:42:21Z

goos: darwin
goarch: arm64
pkg: github.com/grafana/pyroscope-go/godeltaprof/compat
                                      │   old.txt   │              new.txt               │
                                      │   sec/op    │   sec/op     vs base               │
HeapCompression-8                       715.6µ ± 1%   570.0µ ± 1%  -20.35% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   676.0µ ± 0%   544.5µ ± 1%  -19.45% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   681.8µ ± 1%   543.8µ ± 1%  -20.25% (p=0.002 n=6)
HeapDelta-8                             78.44µ ± 1%   97.20µ ± 2%  +23.92% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         54.43µ ± 1%   80.91µ ± 1%  +48.65% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         54.02µ ± 1%   81.12µ ± 2%  +50.16% (p=0.002 n=6)
geomean                                 205.8µ        218.1µ        +5.96%

                                      │   old.txt    │               new.txt               │
                                      │     B/op     │     B/op      vs base               │
HeapCompression-8                       965.7Ki ± 0%   969.5Ki ± 0%   +0.39% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   965.7Ki ± 0%   969.1Ki ± 0%   +0.35% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   965.6Ki ± 0%   969.1Ki ± 0%   +0.36% (p=0.002 n=6)
HeapDelta-8                              333.00 ± 0%     50.00 ± 2%  -84.98% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8          313.00 ± 0%     30.00 ± 0%  -90.42% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8          313.00 ± 0%     30.00 ± 0%  -90.42% (p=0.002 n=6)
geomean                                 17.36Ki        5.802Ki       -66.57%

                                      │  old.txt   │              new.txt              │
                                      │ allocs/op  │ allocs/op   vs base               │
HeapCompression-8                       788.0 ± 0%   782.0 ± 0%   -0.76% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   788.0 ± 0%   782.0 ± 0%   -0.76% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   788.0 ± 0%   782.0 ± 0%   -0.76% (p=0.002 n=6)
HeapDelta-8                             2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
geomean                                 39.70        27.96       -29.56%

The delta bench became slower by 20-50%
What I don't understand is why compression test became faster 20% 🤷

This reverts commit 5a760bf.

korniltsev · 2024-05-28T08:34:19Z

What I don't understand is why compression test became faster 20% 🤷

Ok, I understand now: the benchmarks were using different randomized testdata. Fixed.

goos: darwin
goarch: arm64
pkg: github.com/grafana/pyroscope-go/godeltaprof/compat
                                      │   old.txt   │               new.txt                │
                                      │   sec/op    │    sec/op      vs base               │
HeapCompression-8                       691.1µ ± 0%    729.8µ ± 65%   +5.59% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   640.6µ ± 1%    693.8µ ±  9%   +8.30% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   643.2µ ± 1%    690.3µ ±  1%   +7.33% (p=0.002 n=6)
HeapDelta-8                             76.26µ ± 1%   113.92µ ±  2%  +49.38% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         48.98µ ± 1%    81.11µ ±  2%  +65.59% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         49.03µ ± 1%    81.14µ ±  1%  +65.49% (p=0.002 n=6)
geomean                                 193.3µ         253.0µ        +30.87%

                                      │   old.txt    │               new.txt               │
                                      │     B/op     │     B/op      vs base               │
HeapCompression-8                       974.1Ki ± 0%   973.9Ki ± 0%   -0.03% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   974.6Ki ± 0%   974.3Ki ± 0%   -0.02% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   974.6Ki ± 0%   974.3Ki ± 0%   -0.03% (p=0.002 n=6)
HeapDelta-8                              333.00 ± 0%     53.00 ± 0%  -84.08% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8          312.00 ± 0%     30.00 ± 0%  -90.38% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8          312.00 ± 0%     30.00 ± 0%  -90.38% (p=0.002 n=6)
geomean                                 17.42Ki        5.874Ki       -66.28%

                                      │  old.txt   │              new.txt              │
                                      │ allocs/op  │ allocs/op   vs base               │
HeapCompression-8                       788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
MutexCompression/ScalerMutexProfile-8   788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
MutexCompression/ScalerBlockProfile-8   788.0 ± 0%   787.0 ± 0%   -0.13% (p=0.002 n=6)
HeapDelta-8                             2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerMutexProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
MutexDelta/ScalerBlockProfile-8         2.000 ± 0%   1.000 ± 0%  -50.00% (p=0.002 n=6)
geomean                                 39.70        28.05       -29.33%

So it looks like the cost of deduplication is about 5% of overall time(including compression), which is not that bad.

godeltaprof/internal/pprof/delta_heap.go

kolesnikovae

LGTM. Nice job!

korniltsev force-pushed the korniltsev/gotip-fix-mutex-tests branch from 6c4afd9 to 5c31860 Compare May 23, 2024 08:06

make tests a bit more reusable

65a479d

korniltsev force-pushed the korniltsev/gotip-fix-mutex-tests branch from 5c31860 to 65a479d Compare May 24, 2024 06:08

korniltsev added 22 commits May 24, 2024 15:50

add more tests for allocations

f6edf8e

comment

25a686e

more logs

aba8908

more logs

6f40c50

fix test

9b1889b

more logs

d752721

more logs

50f78aa

diff

07915a5

diff

68de7a4

no generics

1f5f735

revert dependencies

536859c

todo

318d9ab

todo

2c422ec

fix test

849451b

improve test, remove internals tests

f4d5924

fix mutex

4676604

fix heap

c2caa4e

make mprof map generic

28e67be

go work sync

ed47413

rm 16 17 support

9f94b26

make go/mod

10617e8

fix test 18 19

abbab38

korniltsev marked this pull request as ready for review May 24, 2024 17:25

korniltsev requested a review from a team as a code owner May 24, 2024 17:25

korniltsev added 2 commits May 27, 2024 18:09

tiny optimization

525c0e3

remove unused fields i profMap

10ed1f0

korniltsev added 5 commits May 28, 2024 13:55

maybe fix bench reproducibility

5a760bf

Revert "maybe fix bench reproducibility"

ec7c2f0

This reverts commit 5a760bf.

fix BenchmarkHeapDelta and BenchmarkHeapCompression determinism

3d4f5ca

fix BenchmarkMutexDelta and BenchmarkMutexCompression determinism

912e33e

save a bit of memory

864a4c5

fix gotip compilation again

1596d12

kolesnikovae reviewed Jun 3, 2024

View reviewed changes

godeltaprof/internal/pprof/delta_heap.go Outdated Show resolved Hide resolved

godeltaprof/internal/pprof/delta_heap.go Outdated Show resolved Hide resolved

godeltaprof/internal/pprof/delta_heap.go Outdated Show resolved Hide resolved

korniltsev added 2 commits June 3, 2024 18:56

use r.Stack(), add comments avoid blockSize multiplications

eb3517f

add more comments

560126f

kolesnikovae approved these changes Jun 4, 2024

View reviewed changes

korniltsev merged commit 181f95a into main Jun 4, 2024
7 checks passed

korniltsev deleted the korniltsev/gotip-fix-mutex-tests branch June 4, 2024 05:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(godeltaprof): fix mutex tests on gotip #108

fix(godeltaprof): fix mutex tests on gotip #108

korniltsev commented May 22, 2024 •

edited

Loading

korniltsev commented May 22, 2024

korniltsev commented May 27, 2024

korniltsev commented May 28, 2024 •

edited

Loading

kolesnikovae left a comment

fix(godeltaprof): fix mutex tests on gotip #108

fix(godeltaprof): fix mutex tests on gotip #108

Conversation

korniltsev commented May 22, 2024 • edited Loading

korniltsev commented May 22, 2024

korniltsev commented May 27, 2024

korniltsev commented May 28, 2024 • edited Loading

kolesnikovae left a comment

Choose a reason for hiding this comment

korniltsev commented May 22, 2024 •

edited

Loading

korniltsev commented May 28, 2024 •

edited

Loading