Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Use more cache friendly implementations #1164

Merged
merged 1 commit into from
Dec 20, 2018
Merged

Use more cache friendly implementations #1164

merged 1 commit into from
Dec 20, 2018

Conversation

shanson7
Copy link
Collaborator

@shanson7 shanson7 commented Dec 5, 2018

While debugging slow groupByTags performance I noticed that a lot of the time was in math.IsNaN. This function should be fast, so I figured it was due to cpu cache line misses. Some of the functions were easy to make more cache friendly, so I did.

benchmark                                              old ns/op     new ns/op     delta
BenchmarkSeriesAggregateSum10k_100NoNulls-8            4520815       1746490       -61.37%
BenchmarkSeriesAggregateSum10k_100WithNulls-8          4458155       1744894       -60.86%
BenchmarkSeriesAggregateMax10k_100NoNulls-8            3263282       1772731       -45.68%
BenchmarkSeriesAggregateMax10k_100WithNulls-8          3288302       1777941       -45.93%
BenchmarkSeriesAggregateMin10k_100NoNulls-8            3417546       1737670       -49.15%
BenchmarkSeriesAggregateMin10k_100WithNulls-8          3526632       1759587       -50.11%
BenchmarkSeriesAggregateMultiply10k_100NoNulls-8       2998636       1293014       -56.88%
BenchmarkSeriesAggregateMultiply10k_100WithNulls-8     2987237       1290399       -56.80%
BenchmarkSeriesAggregateDiff10k_100NoNulls-8           4737603       1951431       -58.81%
BenchmarkSeriesAggregateDiff10k_100WithNulls-8         4656263       1942149       -58.29%
BenchmarkSeriesAggregateRange10k_100NoNulls-8          6827062       3519037       -48.45%
BenchmarkSeriesAggregateRange10k_100WithNulls-8        6853572       3535418       -48.41%

benchmark                                              old MB/s     new MB/s     speedup
BenchmarkSeriesAggregateSum10k_100NoNulls-8            2654.39      6870.92      2.59x
BenchmarkSeriesAggregateSum10k_100WithNulls-8          2691.70      6877.21      2.55x
BenchmarkSeriesAggregateMax10k_100NoNulls-8            3677.28      6769.22      1.84x
BenchmarkSeriesAggregateMax10k_100WithNulls-8          3649.30      6749.38      1.85x
BenchmarkSeriesAggregateMin10k_100NoNulls-8            3511.29      6905.80      1.97x
BenchmarkSeriesAggregateMin10k_100WithNulls-8          3402.68      6819.78      2.00x
BenchmarkSeriesAggregateMultiply10k_100NoNulls-8       4001.82      9280.63      2.32x
BenchmarkSeriesAggregateMultiply10k_100WithNulls-8     4017.09      9299.45      2.31x
BenchmarkSeriesAggregateDiff10k_100NoNulls-8           2532.93      6149.33      2.43x
BenchmarkSeriesAggregateDiff10k_100WithNulls-8         2577.17      6178.72      2.40x
BenchmarkSeriesAggregateRange10k_100NoNulls-8          1757.71      3410.02      1.94x
BenchmarkSeriesAggregateRange10k_100WithNulls-8        1750.91      3394.22      1.94x

@shanson7 shanson7 mentioned this pull request Dec 6, 2018
Merged
@Dieterbe
Copy link
Contributor

So in master, we iterate, for each point in time, over all series and the output series.
In this PR, we iterate, for each input series, over all points for the input series and the output series

It makes sense that the latter could be faster, though it's a suprise to me why a profile would show time spent in math.IsNaN() though. did you mean time is spent on the math.IsNaN(p) line, or within math.IsNaN() itself? i would expect the former, not the latter.

Dieterbe added a commit that referenced this pull request Dec 20, 2018
@Dieterbe
BenchmarkAggregate: also test with 2 input series, and test using Min
fa2b2f0 
so that we can better see the effect of
#1164
@Dieterbe
Copy link
Contributor

Dieterbe commented Dec 20, 2018 •
edited
Loading

I decided to run my own benchmarks using https://godoc.org/golang.org/x/perf/cmd/benchstat
on my laptop which has all kinds of scheduling and cpu isolation tweaks.

for i in 1 2 3 4 5 ; do taskset --cpu-list 5 go test ./... -run '^$' -test.benchmem -short -bench '^Bench.*Aggregate' . ; done

to get a more complete picture. This includes Avg, Median, and Stdev which received no, or minor updates.
And min,max,sum,multiply,diff,range are the ones that received the mentioned update (range being a special case, piggybacking on the min/max gains)

I was also curious wether the number of input series affects the performance delta,
e.g. are there pathalogical cases where we may see a slowdown?
This is something we can test via the BenchmarkAggregate* functions which test a wider variety
of inputs for a given aggregator function.
You can see my branch at https://github.com/grafana/metrictank/tree/cacheFriendly, which is your PR but preceeded
by a commit which changed BenchmarkAggregate to use Min rather than Avg and includes a case to test with 2 input series

The results speak for themselves I think.
I'm happy to report that for single series there is no difference, and for >1 series there's an increasing speedup, although you need tens of series for it to really kick into gear.

name                                     old time/op    new time/op    delta
Aggregate10k_1NoNulls                       868ns ± 1%     866ns ± 1%      ~     (p=0.286 n=5+5)
Aggregate10k_2NoNulls                       230µs ± 4%     215µs ± 5%    -6.51%  (p=0.016 n=5+5)
Aggregate10k_10NoNulls                      460µs ± 2%     434µs ± 3%    -5.70%  (p=0.008 n=5+5)
Aggregate10k_100NoNulls                    6.45ms ± 2%    3.08ms ± 1%   -52.26%  (p=0.008 n=5+5)
Aggregate10k_1000NoNulls                    167ms ± 2%      31ms ± 1%   -81.42%  (p=0.008 n=5+5)
Aggregate10k_1SomeSeriesHalfNulls           868ns ± 1%     869ns ± 1%      ~     (p=0.841 n=5+5)
Aggregate10k_2SomeSeriesHalfNulls           230µs ± 2%     209µs ± 4%    -9.12%  (p=0.008 n=5+5)
Aggregate10k_10SomeSeriesHalfNulls          455µs ± 1%     435µs ± 3%    -4.25%  (p=0.008 n=5+5)
Aggregate10k_100SomeSeriesHalfNulls        6.38ms ± 1%    3.13ms ± 2%   -50.96%  (p=0.008 n=5+5)
Aggregate10k_1000SomeSeriesHalfNulls        165ms ± 5%      31ms ± 2%   -81.14%  (p=0.008 n=5+5)
Aggregate10k_1AllSeriesHalfNulls            870ns ± 0%     872ns ± 1%      ~     (p=0.571 n=5+5)
Aggregate10k_2AllSeriesHalfNulls            224µs ± 4%     205µs ± 3%    -8.82%  (p=0.008 n=5+5)
Aggregate10k_10AllSeriesHalfNulls           431µs ± 1%     380µs ± 3%   -11.90%  (p=0.008 n=5+5)
Aggregate10k_100AllSeriesHalfNulls         5.97ms ± 0%    2.63ms ± 3%   -55.87%  (p=0.008 n=5+5)
Aggregate10k_1000AllSeriesHalfNulls         164ms ± 2%      26ms ± 3%   -84.05%  (p=0.008 n=5+5)
SeriesAggregateAvg10k_100NoNulls           6.21ms ± 1%    6.14ms ± 2%      ~     (p=0.222 n=5+5)
SeriesAggregateAvg10k_100WithNulls         6.20ms ± 1%    6.14ms ± 2%      ~     (p=0.222 n=5+5)
SeriesAggregateSum10k_100NoNulls           8.38ms ± 1%    2.98ms ± 2%   -64.46%  (p=0.008 n=5+5)
SeriesAggregateSum10k_100WithNulls         8.39ms ± 1%    2.98ms ± 2%   -64.44%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100NoNulls           6.21ms ± 1%    2.93ms ± 2%   -52.86%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100WithNulls         6.21ms ± 1%    2.92ms ± 2%   -53.06%  (p=0.008 n=5+5)
SeriesAggregateMin10k_100NoNulls           6.21ms ± 1%    2.90ms ± 0%   -53.35%  (p=0.016 n=5+4)
SeriesAggregateMin10k_100WithNulls         6.22ms ± 2%    2.92ms ± 3%   -52.99%  (p=0.008 n=5+5)
SeriesAggregateMultiply10k_100NoNulls      5.51ms ± 1%    2.34ms ± 4%   -57.57%  (p=0.008 n=5+5)
SeriesAggregateMultiply10k_100WithNulls    5.51ms ± 1%    2.33ms ± 5%   -57.68%  (p=0.008 n=5+5)
SeriesAggregateMedian10k_100NoNulls        30.4ms ± 0%    30.4ms ± 1%      ~     (p=1.000 n=5+5)
SeriesAggregateMedian10k_100WithNulls      30.5ms ± 1%    30.5ms ± 1%      ~     (p=1.000 n=5+5)
SeriesAggregateDiff10k_100NoNulls          8.89ms ± 2%    3.31ms ± 2%   -62.82%  (p=0.008 n=5+5)
SeriesAggregateDiff10k_100WithNulls        8.90ms ± 1%    3.31ms ± 2%   -62.82%  (p=0.008 n=5+5)
SeriesAggregateStddev10k_100NoNulls        12.9ms ± 1%    12.7ms ± 2%      ~     (p=0.421 n=5+5)
SeriesAggregateStddev10k_100WithNulls      12.9ms ± 1%    12.7ms ± 2%      ~     (p=0.095 n=5+5)
SeriesAggregateRange10k_100NoNulls         12.5ms ± 2%     5.9ms ± 2%   -52.65%  (p=0.008 n=5+5)
SeriesAggregateRange10k_100WithNulls       12.5ms ± 2%     5.9ms ± 1%   -52.73%  (p=0.008 n=5+5)

name                                     old speed      new speed      delta
Aggregate10k_1NoNulls                     138GB/s ± 1%   139GB/s ± 0%      ~     (p=0.310 n=5+5)
Aggregate10k_2NoNulls                    1.04GB/s ± 4%  1.12GB/s ± 4%    +6.97%  (p=0.016 n=5+5)
Aggregate10k_10NoNulls                   2.61GB/s ± 2%  2.77GB/s ± 3%    +6.05%  (p=0.008 n=5+5)
Aggregate10k_100NoNulls                  1.86GB/s ± 2%  3.90GB/s ± 1%  +109.43%  (p=0.008 n=5+5)
Aggregate10k_1000NoNulls                  719MB/s ± 2%  3871MB/s ± 1%  +438.25%  (p=0.008 n=5+5)
Aggregate10k_1SomeSeriesHalfNulls         138GB/s ± 1%   138GB/s ± 1%      ~     (p=0.841 n=5+5)
Aggregate10k_2SomeSeriesHalfNulls        1.04GB/s ± 2%  1.15GB/s ± 4%   +10.07%  (p=0.008 n=5+5)
Aggregate10k_10SomeSeriesHalfNulls       2.64GB/s ± 1%  2.76GB/s ± 3%    +4.47%  (p=0.008 n=5+5)
Aggregate10k_100SomeSeriesHalfNulls      1.88GB/s ± 1%  3.84GB/s ± 2%  +103.96%  (p=0.008 n=5+5)
Aggregate10k_1000SomeSeriesHalfNulls      726MB/s ± 5%  3847MB/s ± 2%  +429.86%  (p=0.008 n=5+5)
Aggregate10k_1AllSeriesHalfNulls          138GB/s ± 0%   137GB/s ± 1%      ~     (p=0.690 n=5+5)
Aggregate10k_2AllSeriesHalfNulls         1.07GB/s ± 4%  1.17GB/s ± 3%    +9.69%  (p=0.008 n=5+5)
Aggregate10k_10AllSeriesHalfNulls        2.78GB/s ± 1%  3.16GB/s ± 3%   +13.53%  (p=0.008 n=5+5)
Aggregate10k_100AllSeriesHalfNulls       2.01GB/s ± 0%  4.56GB/s ± 3%  +126.68%  (p=0.008 n=5+5)
Aggregate10k_1000AllSeriesHalfNulls       730MB/s ± 2%  4576MB/s ± 3%  +526.98%  (p=0.008 n=5+5)
SeriesAggregateAvg10k_100NoNulls         1.93GB/s ± 1%  1.95GB/s ± 2%      ~     (p=0.222 n=5+5)
SeriesAggregateAvg10k_100WithNulls       1.94GB/s ± 1%  1.96GB/s ± 2%      ~     (p=0.222 n=5+5)
SeriesAggregateSum10k_100NoNulls         1.43GB/s ± 1%  4.03GB/s ± 2%  +181.43%  (p=0.008 n=5+5)
SeriesAggregateSum10k_100WithNulls       1.43GB/s ± 1%  4.02GB/s ± 2%  +181.28%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100NoNulls         1.93GB/s ± 1%  4.10GB/s ± 2%  +112.12%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100WithNulls       1.93GB/s ± 1%  4.12GB/s ± 2%  +113.04%  (p=0.008 n=5+5)
SeriesAggregateMin10k_100NoNulls         1.93GB/s ± 1%  4.14GB/s ± 0%  +114.33%  (p=0.016 n=5+4)
SeriesAggregateMin10k_100WithNulls       1.93GB/s ± 2%  4.11GB/s ± 3%  +112.74%  (p=0.008 n=5+5)
SeriesAggregateMultiply10k_100NoNulls    2.18GB/s ± 1%  5.13GB/s ± 4%  +135.82%  (p=0.008 n=5+5)
SeriesAggregateMultiply10k_100WithNulls  2.18GB/s ± 1%  5.15GB/s ± 5%  +136.44%  (p=0.008 n=5+5)
SeriesAggregateMedian10k_100NoNulls       394MB/s ± 0%   394MB/s ± 1%      ~     (p=1.000 n=5+5)
SeriesAggregateMedian10k_100WithNulls     393MB/s ± 1%   393MB/s ± 1%      ~     (p=1.000 n=5+5)
SeriesAggregateDiff10k_100NoNulls        1.35GB/s ± 2%  3.63GB/s ± 2%  +168.99%  (p=0.008 n=5+5)
SeriesAggregateDiff10k_100WithNulls      1.35GB/s ± 1%  3.63GB/s ± 2%  +169.02%  (p=0.008 n=5+5)
SeriesAggregateStddev10k_100NoNulls       931MB/s ± 1%   942MB/s ± 2%      ~     (p=0.421 n=5+5)
SeriesAggregateStddev10k_100WithNulls     931MB/s ± 1%   945MB/s ± 2%      ~     (p=0.095 n=5+5)
SeriesAggregateRange10k_100NoNulls        961MB/s ± 2%  2030MB/s ± 2%  +111.22%  (p=0.008 n=5+5)
SeriesAggregateRange10k_100WithNulls      962MB/s ± 2%  2036MB/s ± 1%  +111.51%  (p=0.008 n=5+5)

name                                     old alloc/op   new alloc/op   delta
Aggregate10k_1NoNulls                        304B ± 0%      304B ± 0%      ~     (all equal)
Aggregate10k_2NoNulls                       799kB ± 0%     799kB ± 0%    +0.00%  (p=0.040 n=5+5)
Aggregate10k_10NoNulls                      800kB ± 0%     800kB ± 0%      ~     (p=0.238 n=4+5)
Aggregate10k_100NoNulls                     809kB ± 0%     809kB ± 0%    -0.01%  (p=0.008 n=5+5)
Aggregate10k_1000NoNulls                    931kB ± 0%     904kB ± 0%    -2.89%  (p=0.000 n=4+5)
Aggregate10k_1SomeSeriesHalfNulls            304B ± 0%      304B ± 0%      ~     (all equal)
Aggregate10k_2SomeSeriesHalfNulls           799kB ± 0%     799kB ± 0%      ~     (p=0.921 n=5+5)
Aggregate10k_10SomeSeriesHalfNulls          800kB ± 0%     800kB ± 0%      ~     (p=1.000 n=5+5)
Aggregate10k_100SomeSeriesHalfNulls         809kB ± 0%     809kB ± 0%    -0.01%  (p=0.008 n=5+5)
Aggregate10k_1000SomeSeriesHalfNulls        931kB ± 0%     904kB ± 0%    -2.89%  (p=0.000 n=5+4)
Aggregate10k_1AllSeriesHalfNulls             304B ± 0%      304B ± 0%      ~     (all equal)
Aggregate10k_2AllSeriesHalfNulls            799kB ± 0%     799kB ± 0%      ~     (p=1.000 n=5+5)
Aggregate10k_10AllSeriesHalfNulls           800kB ± 0%     800kB ± 0%      ~     (p=0.714 n=5+5)
Aggregate10k_100AllSeriesHalfNulls          809kB ± 0%     809kB ± 0%    -0.01%  (p=0.000 n=4+5)
Aggregate10k_1000AllSeriesHalfNulls         931kB ± 0%     904kB ± 0%    -2.89%  (p=0.029 n=4+4)
SeriesAggregateAvg10k_100NoNulls             819B ± 0%      819B ± 0%      ~     (all equal)
SeriesAggregateAvg10k_100WithNulls           819B ± 0%      819B ± 0%      ~     (all equal)
SeriesAggregateSum10k_100NoNulls             819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateSum10k_100WithNulls           819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100NoNulls             819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateMax10k_100WithNulls           819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateMin10k_100NoNulls             819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateMin10k_100WithNulls           819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateMultiply10k_100NoNulls        546B ± 0%      163B ± 0%   -70.15%  (p=0.000 n=5+4)
SeriesAggregateMultiply10k_100WithNulls      546B ± 0%      163B ± 0%   -70.15%  (p=0.000 n=5+4)
SeriesAggregateMedian10k_100NoNulls         324kB ± 0%     324kB ± 0%      ~     (all equal)
SeriesAggregateMedian10k_100WithNulls       324kB ± 0%     324kB ± 0%      ~     (all equal)
SeriesAggregateDiff10k_100NoNulls            819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateDiff10k_100WithNulls          819B ± 0%      327B ± 0%   -60.07%  (p=0.008 n=5+5)
SeriesAggregateStddev10k_100NoNulls         165kB ± 0%       2kB ± 0%   -99.01%  (p=0.008 n=5+5)
SeriesAggregateStddev10k_100WithNulls       165kB ± 0%       2kB ± 0%   -99.01%  (p=0.008 n=5+5)
SeriesAggregateRange10k_100NoNulls          329kB ± 0%     164kB ± 0%   -50.08%  (p=0.000 n=5+4)
SeriesAggregateRange10k_100WithNulls        329kB ± 0%     164kB ± 0%   -50.08%  (p=0.008 n=5+5)

name                                     old allocs/op  new allocs/op  delta
Aggregate10k_1NoNulls                        8.00 ± 0%      8.00 ± 0%      ~     (all equal)
Aggregate10k_2NoNulls                        27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_10NoNulls                       27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_100NoNulls                      28.0 ± 0%      27.0 ± 0%    -3.57%  (p=0.008 n=5+5)
Aggregate10k_1000NoNulls                      227 ± 0%        67 ± 0%   -70.48%  (p=0.008 n=5+5)
Aggregate10k_1SomeSeriesHalfNulls            8.00 ± 0%      8.00 ± 0%      ~     (all equal)
Aggregate10k_2SomeSeriesHalfNulls            27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_10SomeSeriesHalfNulls           27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_100SomeSeriesHalfNulls          28.0 ± 0%      27.0 ± 0%    -3.57%  (p=0.008 n=5+5)
Aggregate10k_1000SomeSeriesHalfNulls          227 ± 0%        67 ± 0%   -70.48%  (p=0.008 n=5+5)
Aggregate10k_1AllSeriesHalfNulls             8.00 ± 0%      8.00 ± 0%      ~     (all equal)
Aggregate10k_2AllSeriesHalfNulls             27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_10AllSeriesHalfNulls            27.0 ± 0%      27.0 ± 0%      ~     (all equal)
Aggregate10k_100AllSeriesHalfNulls           28.0 ± 0%      27.0 ± 0%      ~     (p=0.079 n=4+5)
Aggregate10k_1000AllSeriesHalfNulls           227 ± 0%        67 ± 0%   -70.48%  (p=0.008 n=5+5)
SeriesAggregateAvg10k_100NoNulls             0.00           0.00           ~     (all equal)
SeriesAggregateAvg10k_100WithNulls           0.00           0.00           ~     (all equal)
SeriesAggregateSum10k_100NoNulls             0.00           0.00           ~     (all equal)
SeriesAggregateSum10k_100WithNulls           0.00           0.00           ~     (all equal)
SeriesAggregateMax10k_100NoNulls             0.00           0.00           ~     (all equal)
SeriesAggregateMax10k_100WithNulls           0.00           0.00           ~     (all equal)
SeriesAggregateMin10k_100NoNulls             0.00           0.00           ~     (all equal)
SeriesAggregateMin10k_100WithNulls           0.00           0.00           ~     (all equal)
SeriesAggregateMultiply10k_100NoNulls        0.00           0.00           ~     (all equal)
SeriesAggregateMultiply10k_100WithNulls      0.00           0.00           ~     (all equal)
SeriesAggregateMedian10k_100NoNulls         10.0k ± 0%     10.0k ± 0%      ~     (all equal)
SeriesAggregateMedian10k_100WithNulls       10.0k ± 0%     10.0k ± 0%      ~     (all equal)
SeriesAggregateDiff10k_100NoNulls            0.00           0.00           ~     (all equal)
SeriesAggregateDiff10k_100WithNulls          0.00           0.00           ~     (all equal)
SeriesAggregateStddev10k_100NoNulls          1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
SeriesAggregateStddev10k_100WithNulls        1.00 ± 0%      0.00       -100.00%  (p=0.008 n=5+5)
SeriesAggregateRange10k_100NoNulls           2.00 ± 0%      1.00 ± 0%   -50.00%  (p=0.008 n=5+5)
SeriesAggregateRange10k_100WithNulls         2.00 ± 0%      1.00 ± 0%   -50.00%  (p=0.008 n=5+5)

Dieterbe
Dieterbe reviewed Dec 20, 2018
View reviewed changes
expr/seriesaggregators.go
mins := make([]schema.Point, 0, len(in[0].Datapoints))

crossSeriesMax(in, &maxes)
crossSeriesMax(in, out)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever reuse of slices too :)

@Dieterbe
Copy link
Contributor

I took down my branch as we don't really need it anymore.
but for posterity, my commit was

commit fa2b2f03e0882582a589a19c5cc62f2b1a1e570d (master)
Author: Dieter Plaetinck <dieter@grafana.com>
Date:   Thu Dec 20 10:50:09 2018 +0100

    BenchmarkAggregate: also test with 2 input series, and test using Min
    
    so that we can better see the effect of
    https://github.com/grafana/metrictank/pull/1164

diff --git a/expr/func_aggregate_test.go b/expr/func_aggregate_test.go
index e52d97b8..c40c34bf 100644
--- a/expr/func_aggregate_test.go
+++ b/expr/func_aggregate_test.go
@@ -272,6 +272,9 @@ func testAggregate(name, agg string, in [][]models.Series, out models.Series, t
 func BenchmarkAggregate10k_1NoNulls(b *testing.B) {
        benchmarkAggregate(b, 1, test.RandFloats10k, test.RandFloats10k)
 }
+func BenchmarkAggregate10k_2NoNulls(b *testing.B) {
+       benchmarkAggregate(b, 2, test.RandFloats10k, test.RandFloats10k)
+}
 func BenchmarkAggregate10k_10NoNulls(b *testing.B) {
        benchmarkAggregate(b, 10, test.RandFloats10k, test.RandFloats10k)
 }
@@ -285,6 +288,9 @@ func BenchmarkAggregate10k_1000NoNulls(b *testing.B) {
 func BenchmarkAggregate10k_1SomeSeriesHalfNulls(b *testing.B) {
        benchmarkAggregate(b, 1, test.RandFloats10k, test.RandFloatsWithNulls10k)
 }
+func BenchmarkAggregate10k_2SomeSeriesHalfNulls(b *testing.B) {
+       benchmarkAggregate(b, 2, test.RandFloats10k, test.RandFloatsWithNulls10k)
+}
 func BenchmarkAggregate10k_10SomeSeriesHalfNulls(b *testing.B) {
        benchmarkAggregate(b, 10, test.RandFloats10k, test.RandFloatsWithNulls10k)
 }
@@ -298,6 +304,9 @@ func BenchmarkAggregate10k_1000SomeSeriesHalfNulls(b *testing.B) {
 func BenchmarkAggregate10k_1AllSeriesHalfNulls(b *testing.B) {
        benchmarkAggregate(b, 1, test.RandFloatsWithNulls10k, test.RandFloatsWithNulls10k)
 }
+func BenchmarkAggregate10k_2AllSeriesHalfNulls(b *testing.B) {
+       benchmarkAggregate(b, 2, test.RandFloatsWithNulls10k, test.RandFloatsWithNulls10k)
+}
 func BenchmarkAggregate10k_10AllSeriesHalfNulls(b *testing.B) {
        benchmarkAggregate(b, 10, test.RandFloatsWithNulls10k, test.RandFloatsWithNulls10k)
 }
@@ -324,7 +333,7 @@ func benchmarkAggregate(b *testing.B, numSeries int, fn0, fn1 func() []schema.Po
        b.ResetTimer()
        var err error
        for i := 0; i < b.N; i++ {
-               f := NewAggregateConstructor("average", crossSeriesAvg)()
+               f := NewAggregateConstructor("min", crossSeriesMin)()
                avg := f.(*FuncAggregate)
                avg.in = append(avg.in, NewMock(input))
                results, err = f.Exec(make(map[Req][]models.Series))

Dieterbe
Dieterbe approved these changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@Dieterbe Dieterbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work Sean 🎉

@Dieterbe Dieterbe merged commit e8ef35f into grafana:master Dec 20, 2018
@shanson7
Copy link
Collaborator Author

though it's a suprise to me why a profile would show time spent in math.IsNaN() though. did you mean time is spent on the math.IsNaN(p) line

math.IsNaN got inlined, so the profile just showed it as math.IsNaN (inline) which is likely due to that being the first place that p was actually used.

I was also curious wether the number of input series affects the performance delta,
e.g. are there pathalogical cases where we may see a slowdown?

Definitely. If the number of series is small enough, then they can all fit in cache and there is no issue with the old code.

@Dieterbe
Copy link
Contributor

right, to be more clear, I was looking for cases where the new code would be slower, but i couldn't find such case.

@Dieterbe Dieterbe added this to the vnext milestone Feb 11, 2019
@shanson7 shanson7 deleted the cacheFriendly branch March 6, 2019 14:27
@Dieterbe Dieterbe mentioned this pull request May 28, 2019
Closed
@shanson7 shanson7 mentioned this pull request Mar 27, 2020
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Dieterbe Dieterbe Dieterbe approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@shanson7 @Dieterbe