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.

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

receive/rule: Fixed Segfault issue; Added tests & benchmarks for TSDB Store, fixed multitsdb benchmarks. #3046

Merged
merged 1 commit into from
Aug 25, 2020

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Aug 18, 2020

Fixed #3013

Also:

  • Fixed other quite big issue with reusing chunk slice.
  • Fixed framing - previously it was wrongly sending single-chunk frames, taking
    huge amount of time.

Fix: We deletage closer now to ensure multitsdb operate on valid data.

Signed-off-by: Bartlomiej Plotka [email protected]

@bwplotka bwplotka force-pushed the segfault-fix branch 8 times, most recently from fb84117 to 522d759 Compare August 21, 2020 09:06
@bwplotka bwplotka marked this pull request as ready for review August 21, 2020 09:06
@bwplotka
Copy link
Member Author

PTAL - this should be ready to go. It's blocking 0.15 release.

cc @krasi-georgiev

@metalmatze
Copy link
Contributor

Generally looking good, not too familiar with the surrounding code though.

Copy link
Member

@GiedriusS GiedriusS left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits in line.

pkg/store/tsdb.go Outdated Show resolved Hide resolved
@@ -241,7 +220,7 @@ func TestServerSeries(t testutil.TB, store storepb.StoreServer, cases ...*Series
srv := NewSeriesServer(context.Background())
testutil.Ok(t, store.Series(c.Req, srv))
testutil.Equals(t, len(c.ExpectedWarnings), len(srv.Warnings), "%v", srv.Warnings)
testutil.Equals(t, len(c.ExpectedSeries), len(srv.SeriesSet))
//testutil.Equals(t, len(c.ExpectedSeries), len(srv.SeriesSet))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Important bug, we should have it and uncomment, thanks!

pkg/store/tsdb.go Outdated Show resolved Hide resolved
pkg/store/tsdb.go Show resolved Hide resolved
@bwplotka bwplotka force-pushed the segfault-fix branch 2 times, most recently from 8d3e1b3 to 267ee04 Compare August 24, 2020 13:58
@bwplotka
Copy link
Member Author

Looking on different frame limits it's really close... However actually now it shows 1MB being bit better one...

bash ../thanos/_dev/benchmarks/cmp.sh noframelimit 1mblimit
Comparing '/home/bwplotka/Repos/thanos/_dev/benchmarks/results/BenchmarkTSDBStoreSeries/noframelimit/2020-08-24-T17-39-13.bench.out' with (new) '/home/bwplotka/Repos/thanos/_dev/benchmarks/results/BenchmarkTSDBStoreSeries/1mblimit/2020-08-24-T17-31-18.bench.out'
name                                                                                                    old time/op    new time/op    delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       6.24s ± 0%     6.73s ± 0%   +7.79%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      804ms ± 0%     684ms ± 0%  -15.02%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     426ms ± 0%     390ms ± 0%   -8.50%

name                                                                                                    old alloc/op   new alloc/op   delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12      2.16GB ± 0%    2.16GB ± 0%   -0.01%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      363MB ± 0%     362MB ± 0%   -0.25%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     601MB ± 0%     591MB ± 0%   -1.62%

name                                                                                                    old allocs/op  new allocs/op  delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       43.0M ± 0%     43.0M ± 0%   -0.00%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      4.43M ± 0%     4.43M ± 0%   -0.01%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12      518k ± 0%      519k ± 0%   +0.20%

@bwplotka
Copy link
Member Author

Another one:

name                                                                                                    old time/op    new time/op    delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       6.80s ± 0%     6.73s ± 0%   -1.08%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      970ms ± 0%     684ms ± 0%  -29.54%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     461ms ± 0%     390ms ± 0%  -15.55%

name                                                                                                    old alloc/op   new alloc/op   delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12      2.16GB ± 0%    2.16GB ± 0%   -0.00%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      365MB ± 0%     362MB ± 0%   -0.62%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     603MB ± 0%     591MB ± 0%   -1.93%

name                                                                                                    old allocs/op  new allocs/op  delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       43.0M ± 0%     43.0M ± 0%   -0.00%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      4.43M ± 0%     4.43M ± 0%   -0.02%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12      518k ± 0%      519k ± 0%   +0.19%

@bwplotka bwplotka force-pushed the segfault-fix branch 2 times, most recently from 4922fbd to a693a66 Compare August 25, 2020 10:28
@bwplotka
Copy link
Member Author

OK, extensive tests shows really not much difference, so sticking to 1MB limit as it was previously (:

Comparing '/home/bwplotka/Repos/thanos/_dev/benchmarks/results/BenchmarkTSDBStoreSeries/noframelimit/2020-08-25-T12-07-03.bench.out' with (new) '/home/bwplotka/Repos/thanos/_dev/benchmarks/results/BenchmarkTSDBStoreSeries/1mblimit/2020-08-25-T11-45-07.bench.out'
name                                                                                                    old time/op    new time/op    delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       6.28s ± 0%     6.04s ± 0%  -3.96%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      814ms ± 0%     790ms ± 0%  -2.97%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     425ms ± 0%     427ms ± 0%  +0.48%

name                                                                                                    old alloc/op   new alloc/op   delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12      2.15GB ± 0%    2.15GB ± 0%  -0.00%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      365MB ± 0%     365MB ± 0%  -0.20%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12     612MB ± 0%     599MB ± 0%  -2.01%

name                                                                                                    old allocs/op  new allocs/op  delta
TSDBStoreSeries/1000000SeriesWith1Samples/3_blocks_and_one_WAL_with_1_samples,_250000_series_each-12       43.0M ± 0%     43.0M ± 0%  +0.00%
TSDBStoreSeries/100000SeriesWith100Samples/3_blocks_and_one_WAL_with_25_samples,_25000_series_each-12      4.43M ± 0%     4.43M ± 0%  -0.01%
TSDBStoreSeries/1SeriesWith10000000Samples/3_blocks_and_one_WAL_with_2500000_samples,_1_series_each-12      519k ± 0%      520k ± 0%  +0.18%

@bwplotka
Copy link
Member Author

This is without marshalling - will do marshalling later on

…Store, fixed multitsdb benchmarks.

Fixed #3013

Also:
* Fixed other quite big issue with reusing chunk slice.
* Fixed framing - previously it was wrongly sending single-chunk frames, taking
huge amount of time.

Fix: We deletage closer now to ensure multitsdb operate on valid data.

Signed-off-by: Bartlomiej Plotka <[email protected]>

# Conflicts:
#	pkg/store/tsdb_test.go
#	pkg/testutil/testutil.go
@bwplotka bwplotka merged commit 04a7f54 into master Aug 25, 2020
@bwplotka bwplotka deleted the segfault-fix branch August 25, 2020 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

receive/ruler: Segfault due to chunk bytes reuse.
3 participants