Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Switch to sync.Map for cached event stats #29

Merged
merged 3 commits into from
Jul 20, 2023
Merged

Switch to sync.Map for cached event stats #29

merged 3 commits into from
Jul 20, 2023

Conversation

axw
Copy link
Member

@axw axw commented Jul 20, 2023

This removes the need for exclusive locking to update the cached event stats in Aggregate* methods.

The cperformance changes are negligible, which is expected. I think CombinedMetricsEncoding-16 has more variance than the benchstat output lets on -- in another run it was faster than main, but with a wider range.

goos: linux
goarch: amd64
pkg: github.com/elastic/apm-aggregation/aggregators
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics     
                            │ /tmp/main.txt │            /tmp/pr.txt             │
                            │    sec/op     │    sec/op     vs base              │
AggregateCombinedMetrics       8.373µ ±  6%   8.262µ ±  2%       ~ (p=0.485 n=6)
AggregateCombinedMetrics-8     6.322µ ± 11%   6.393µ ± 10%       ~ (p=0.699 n=6)
AggregateCombinedMetrics-16    6.610µ ±  9%   6.338µ ± 10%       ~ (p=0.937 n=6)
AggregateBatchSerial           35.43µ ±  4%   35.46µ ±  7%       ~ (p=0.937 n=6)
AggregateBatchSerial-8         33.41µ ± 16%   34.42µ ± 12%       ~ (p=1.000 n=6)
AggregateBatchSerial-16        32.73µ ±  5%   33.73µ ± 10%       ~ (p=0.818 n=6)
AggregateBatchParallel         35.08µ ±  6%   34.27µ ±  6%  -2.31% (p=0.026 n=6)
AggregateBatchParallel-8       43.49µ ± 11%   41.08µ ± 15%       ~ (p=0.394 n=6)
AggregateBatchParallel-16      45.19µ ± 12%   42.66µ ± 20%       ~ (p=0.485 n=6)
CombinedMetricsEncoding        4.936µ ±  4%   4.986µ ±  4%       ~ (p=0.485 n=6)
CombinedMetricsEncoding-8      5.064µ ±  5%   5.296µ ±  7%       ~ (p=0.589 n=6)
CombinedMetricsEncoding-16     5.106µ ±  3%   5.370µ ±  3%  +5.17% (p=0.002 n=6)
CombinedMetricsDecoding        6.496µ ±  7%   6.660µ ±  8%       ~ (p=0.818 n=6)
CombinedMetricsDecoding-8      6.939µ ±  7%   7.083µ ±  7%       ~ (p=0.589 n=6)
CombinedMetricsDecoding-16     6.745µ ±  9%   6.795µ ±  7%       ~ (p=0.853 n=6)
CombinedMetricsToBatch         510.5µ ±  3%   513.7µ ±  2%       ~ (p=0.240 n=6)
CombinedMetricsToBatch-8       527.7µ ±  4%   526.6µ ±  3%       ~ (p=0.818 n=6)
CombinedMetricsToBatch-16      527.1µ ±  6%   525.6µ ±  3%       ~ (p=0.818 n=6)
EventToCombinedMetrics         3.231µ ±  3%   3.228µ ±  2%       ~ (p=1.000 n=6)
EventToCombinedMetrics-8       3.401µ ± 10%   3.421µ ±  4%       ~ (p=0.699 n=6)
EventToCombinedMetrics-16      3.514µ ±  7%   3.482µ ±  6%       ~ (p=0.699 n=6)
geomean                        17.88µ         17.91µ        +0.16%

                            │  /tmp/main.txt  │              /tmp/pr.txt              │
                            │      B/op       │     B/op       vs base                │
AggregateCombinedMetrics      3.474Ki ±  7%     3.467Ki ±  1%       ~ (p=0.784 n=6)
AggregateCombinedMetrics-8    3.207Ki ± 10%     3.278Ki ± 17%       ~ (p=0.589 n=6)
AggregateCombinedMetrics-16   2.892Ki ± 22%     2.921Ki ± 20%       ~ (p=0.818 n=6)
AggregateBatchSerial          24.02Ki ±  0%     23.93Ki ±  0%       ~ (p=0.102 n=6)
AggregateBatchSerial-8        24.00Ki ±  0%     24.09Ki ±  1%       ~ (p=0.180 n=6)
AggregateBatchSerial-16       24.00Ki ±  0%     23.98Ki ±  0%       ~ (p=1.000 n=6)
AggregateBatchParallel        23.95Ki ±  0%     23.96Ki ±  0%       ~ (p=0.894 n=6)
AggregateBatchParallel-8      23.79Ki ±  2%     24.01Ki ±  0%       ~ (p=0.485 n=6)
AggregateBatchParallel-16     23.87Ki ±  1%     24.02Ki ±  3%       ~ (p=0.394 n=6)
CombinedMetricsEncoding         0.000 ±   ?       0.000 ±   ?       ~ (p=1.000 n=6)
CombinedMetricsEncoding-8       0.000 ±  0%       0.000 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-16      0.000 ±  0%       0.000 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding       10.01Ki ±  0%     10.01Ki ±  0%       ~ (p=0.636 n=6)
CombinedMetricsDecoding-8     10.01Ki ±  0%     10.01Ki ±  0%       ~ (p=0.242 n=6)
CombinedMetricsDecoding-16    10.01Ki ±  0%     10.01Ki ±  0%       ~ (p=0.242 n=6)
CombinedMetricsToBatch        37.66Ki ±  0%     37.66Ki ±  0%       ~ (p=1.000 n=6)
CombinedMetricsToBatch-8      37.66Ki ±  0%     37.66Ki ±  0%       ~ (p=1.000 n=6)
CombinedMetricsToBatch-16     37.66Ki ±  0%     37.66Ki ±  0%       ~ (p=1.000 n=6)
EventToCombinedMetrics        4.188Ki ±  0%     4.188Ki ±  0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-8      4.188Ki ±  0%     4.188Ki ±  0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-16     4.188Ki ±  0%     4.188Ki ±  0%       ~ (p=1.000 n=6) ¹
geomean                                     ²                  +0.21%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │ /tmp/main.txt │             /tmp/pr.txt             │
                            │   allocs/op   │  allocs/op   vs base                │
AggregateCombinedMetrics      28.00 ±  7%     28.00 ±  4%       ~ (p=1.000 n=6)
AggregateCombinedMetrics-8    26.00 ±  8%     26.50 ± 13%       ~ (p=0.537 n=6)
AggregateCombinedMetrics-16   24.50 ± 14%     24.50 ± 14%       ~ (p=1.000 n=6)
AggregateBatchSerial          112.0 ±  0%     112.0 ±  0%       ~ (p=1.000 n=6) ¹
AggregateBatchSerial-8        112.0 ±  0%     112.0 ±  0%       ~ (p=1.000 n=6) ¹
AggregateBatchSerial-16       112.0 ±  0%     112.0 ±  0%       ~ (p=1.000 n=6) ¹
AggregateBatchParallel        112.0 ±  0%     112.0 ±  0%       ~ (p=1.000 n=6) ¹
AggregateBatchParallel-8      111.0 ±  4%     112.0 ±  1%       ~ (p=0.182 n=6)
AggregateBatchParallel-16     111.0 ±  3%     112.0 ±  4%       ~ (p=0.545 n=6)
CombinedMetricsEncoding       0.000 ±  0%     0.000 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-8     0.000 ±  0%     0.000 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-16    0.000 ±  0%     0.000 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding       40.00 ±  0%     40.00 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding-8     40.00 ±  0%     40.00 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding-16    40.00 ±  0%     40.00 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch        308.0 ±  0%     308.0 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-8      308.0 ±  0%     308.0 ±  0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-16     308.0 ±  0%     308.0 ±  0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics        17.00 ±  0%     17.00 ±  0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-8      17.00 ±  0%     17.00 ±  0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-16     17.00 ±  0%     17.00 ±  0%       ~ (p=1.000 n=6) ¹
geomean                                   ²                +0.18%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@axw axw requested a review from a team as a code owner July 20, 2023 09:18
@axw
Copy link
Member Author

axw commented Jul 20, 2023

Benchmarks are from before I fixed releasing back to the pool, but it should not make a difference.

@carsonip
Copy link
Member

Benchmark:

goos: linux
goarch: amd64
pkg: github.com/elastic/apm-aggregation/aggregators
cpu: 11th Gen Intel(R) Core(TM) i7-11800H @ 2.30GHz
                            │ bench-out/syncmap-cached-stats-before │ bench-out/syncmap-cached-stats-after │
                            │                sec/op                 │    sec/op      vs base               │
AggregateCombinedMetrics-16                             3.350µ ± 3%    3.352µ ± 19%       ~ (p=0.841 n=10)
AggregateBatchSerial-16                                 15.68µ ± 3%    16.04µ ±  3%       ~ (p=0.190 n=10)
AggregateBatchParallel-16                               16.72µ ± 4%    17.24µ ±  3%       ~ (p=0.105 n=10)
CombinedMetricsEncoding-16                              5.066µ ± 3%    5.021µ ±  2%  -0.91% (p=0.035 n=10)
CombinedMetricsDecoding-16                              4.505µ ± 0%    4.500µ ±  2%       ~ (p=0.542 n=10)
CombinedMetricsToBatch-16                               409.2µ ± 2%    407.8µ ±  2%       ~ (p=0.853 n=10)
EventToCombinedMetrics-16                               1.977µ ± 1%    1.979µ ±  1%       ~ (p=0.811 n=10)
geomean                                                 10.71µ         10.78µ        +0.59%

                            │ bench-out/syncmap-cached-stats-before │ bench-out/syncmap-cached-stats-after  │
                            │                 B/op                  │     B/op      vs base                 │
AggregateCombinedMetrics-16                          3.709Ki ± 1%     3.707Ki ± 0%       ~ (p=1.000 n=10)
AggregateBatchSerial-16                              24.01Ki ± 1%     23.93Ki ± 1%       ~ (p=0.315 n=10)
AggregateBatchParallel-16                            23.97Ki ± 0%     23.97Ki ± 0%       ~ (p=0.912 n=10)
CombinedMetricsEncoding-16                             0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsDecoding-16                           10.01Ki ± 0%     10.01Ki ± 0%       ~ (p=0.628 n=10)
CombinedMetricsToBatch-16                            37.66Ki ± 0%     37.66Ki ± 0%       ~ (p=1.000 n=10)
EventToCombinedMetrics-16                            4.188Ki ± 0%     4.188Ki ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                           ²                 -0.06%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │ bench-out/syncmap-cached-stats-before │ bench-out/syncmap-cached-stats-after │
                            │               allocs/op               │  allocs/op   vs base                 │
AggregateCombinedMetrics-16                            29.00 ± 0%      29.00 ± 0%       ~ (p=1.000 n=10) ¹
AggregateBatchSerial-16                                112.0 ± 0%      112.0 ± 0%       ~ (p=1.000 n=10) ¹
AggregateBatchParallel-16                              112.0 ± 0%      112.0 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsEncoding-16                             0.000 ± 0%      0.000 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsDecoding-16                             40.00 ± 0%      40.00 ± 0%       ~ (p=1.000 n=10) ¹
CombinedMetricsToBatch-16                              308.0 ± 0%      308.0 ± 0%       ~ (p=1.000 n=10)
EventToCombinedMetrics-16                              17.00 ± 0%      17.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                                           ²                +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@axw
Copy link
Member Author

axw commented Jul 20, 2023

Again, I think the differences are just noise.

goos: linux
goarch: amd64
pkg: github.com/elastic/apm-aggregation/aggregators
cpu: AMD Ryzen 7 PRO 5850U with Radeon Graphics     
                            │ /tmp/main.txt │             /tmp/pr.txt             │
                            │    sec/op     │    sec/op     vs base               │
AggregateCombinedMetrics       8.983µ ±  8%   8.837µ ±  9%        ~ (p=0.485 n=6)
AggregateCombinedMetrics-8     7.248µ ±  3%   6.648µ ±  9%        ~ (p=0.065 n=6)
AggregateCombinedMetrics-16    7.192µ ±  7%   6.913µ ±  6%        ~ (p=0.093 n=6)
AggregateBatchSerial           24.84µ ±  6%   25.09µ ± 15%        ~ (p=0.699 n=6)
AggregateBatchSerial-8         20.52µ ± 10%   19.87µ ± 20%        ~ (p=0.937 n=6)
AggregateBatchSerial-16        19.42µ ± 10%   18.71µ ± 10%        ~ (p=0.818 n=6)
AggregateBatchParallel         23.48µ ± 13%   21.95µ ±  3%        ~ (p=0.132 n=6)
AggregateBatchParallel-8       22.65µ ± 14%   21.38µ ±  4%        ~ (p=0.065 n=6)
AggregateBatchParallel-16      22.31µ ± 19%   21.08µ ± 16%        ~ (p=0.240 n=6)
CombinedMetricsEncoding        4.988µ ±  6%   4.809µ ±  2%        ~ (p=0.485 n=6)
CombinedMetricsEncoding-8      4.933µ ±  6%   4.820µ ±  6%        ~ (p=0.485 n=6)
CombinedMetricsEncoding-16     4.890µ ±  3%   4.951µ ±  7%        ~ (p=0.132 n=6)
CombinedMetricsDecoding        5.946µ ±  3%   6.197µ ± 12%   +4.21% (p=0.041 n=6)
CombinedMetricsDecoding-8      6.565µ ±  9%   6.606µ ±  7%        ~ (p=0.818 n=6)
CombinedMetricsDecoding-16     6.304µ ± 11%   6.942µ ± 11%  +10.14% (p=0.041 n=6)
CombinedMetricsToBatch         528.1µ ±  2%   535.2µ ±  1%        ~ (p=0.093 n=6)
CombinedMetricsToBatch-8       550.8µ ± 15%   546.0µ ±  2%        ~ (p=0.485 n=6)
CombinedMetricsToBatch-16      552.8µ ±  4%   546.6µ ±  6%        ~ (p=0.485 n=6)
EventToCombinedMetrics         1.199µ ±  3%   1.239µ ±  4%   +3.34% (p=0.017 n=6)
EventToCombinedMetrics-8       1.263µ ± 12%   1.273µ ±  2%        ~ (p=0.818 n=6)
EventToCombinedMetrics-16      1.371µ ± 27%   1.263µ ±  3%   -7.88% (p=0.002 n=6)
geomean                        13.50µ         13.28µ         -1.58%

                            │ /tmp/main.txt  │             /tmp/pr.txt              │
                            │      B/op      │     B/op      vs base                │
AggregateCombinedMetrics      4.711Ki ± 3%     4.552Ki ± 7%  -3.38% (p=0.041 n=6)
AggregateCombinedMetrics-8    4.483Ki ± 5%     4.621Ki ± 4%       ~ (p=0.485 n=6)
AggregateCombinedMetrics-16   4.626Ki ± 8%     4.414Ki ± 6%       ~ (p=0.310 n=6)
AggregateBatchSerial          11.52Ki ± 2%     11.41Ki ± 2%       ~ (p=0.394 n=6)
AggregateBatchSerial-8        11.43Ki ± 1%     11.51Ki ± 2%       ~ (p=0.394 n=6)
AggregateBatchSerial-16       11.53Ki ± 1%     11.53Ki ± 2%       ~ (p=0.699 n=6)
AggregateBatchParallel        11.45Ki ± 3%     11.37Ki ± 2%       ~ (p=0.485 n=6)
AggregateBatchParallel-8      11.55Ki ± 1%     11.62Ki ± 3%       ~ (p=0.699 n=6)
AggregateBatchParallel-16     11.59Ki ± 2%     11.52Ki ± 3%       ~ (p=0.394 n=6)
CombinedMetricsEncoding         0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-8       0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-16      0.000 ±  ?       0.000 ± 0%       ~ (p=1.000 n=6)
CombinedMetricsDecoding       10.01Ki ± 0%     10.01Ki ± 0%       ~ (p=0.545 n=6)
CombinedMetricsDecoding-8     10.01Ki ± 0%     10.01Ki ± 0%       ~ (p=1.000 n=6)
CombinedMetricsDecoding-16    10.01Ki ± 0%     10.01Ki ± 0%       ~ (p=0.892 n=6)
CombinedMetricsToBatch        37.66Ki ± 0%     37.66Ki ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-8      37.66Ki ± 0%     37.66Ki ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-16     37.66Ki ± 0%     37.66Ki ± 0%       ~ (p=1.000 n=6)
EventToCombinedMetrics          80.00 ± 0%       80.00 ± 0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-8        80.00 ± 0%       80.00 ± 0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-16       80.00 ± 0%       80.00 ± 0%       ~ (p=1.000 n=6) ¹
geomean                                    ²                 -0.29%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                            │ /tmp/main.txt │            /tmp/pr.txt             │
                            │   allocs/op   │ allocs/op   vs base                │
AggregateCombinedMetrics       38.00 ± 3%     36.50 ± 4%       ~ (p=0.080 n=6)
AggregateCombinedMetrics-8     36.50 ± 4%     37.00 ± 3%       ~ (p=0.602 n=6)
AggregateCombinedMetrics-16    37.00 ± 5%     35.50 ± 7%       ~ (p=0.407 n=6)
AggregateBatchSerial           79.00 ± 1%     79.00 ± 0%       ~ (p=1.000 n=6)
AggregateBatchSerial-8         79.00 ± 3%     79.00 ± 3%       ~ (p=1.000 n=6)
AggregateBatchSerial-16        79.00 ± 0%     79.00 ± 0%       ~ (p=1.000 n=6) ¹
AggregateBatchParallel         79.00 ± 4%     79.00 ± 3%       ~ (p=1.000 n=6)
AggregateBatchParallel-8       79.00 ± 0%     79.00 ± 0%       ~ (p=1.000 n=6) ¹
AggregateBatchParallel-16      79.00 ± 3%     79.00 ± 4%       ~ (p=1.000 n=6)
CombinedMetricsEncoding        0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-8      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsEncoding-16     0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding        40.00 ± 0%     40.00 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding-8      40.00 ± 0%     40.00 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsDecoding-16     40.00 ± 0%     40.00 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch         308.0 ± 0%     308.0 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-8       308.0 ± 0%     308.0 ± 0%       ~ (p=1.000 n=6) ¹
CombinedMetricsToBatch-16      308.0 ± 0%     308.0 ± 0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics         6.000 ± 0%     6.000 ± 0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-8       6.000 ± 0%     6.000 ± 0%       ~ (p=1.000 n=6) ¹
EventToCombinedMetrics-16      6.000 ± 0%     6.000 ± 0%       ~ (p=1.000 n=6) ¹
geomean                                   ²               -0.32%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@axw axw merged commit a2be42e into main Jul 20, 2023
4 checks passed
@axw axw deleted the syncmap-cached-stats branch July 20, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carsonip carsonip carsonip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@axw @carsonip