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

Improve Ingester Record Insertion: Slice -> Map #681

Merged
merged 16 commits into from
Apr 30, 2021

Conversation

joe-elliott
Copy link
Member

@joe-elliott joe-elliott commented Apr 30, 2021

What this PR does:

  • Adds additional logging around CompleteBlock
  • Drops the slice insertion in favor of using a map in the appender. This is showing a nice increase in spans/cpu internally:

image

and a decrease in time spent in the garbage collector:

image

A skiplist was also experimented with but local benchmarking showed it to be signficantly slower than the map. WIth or without records indicates whether or not the .Records() method was called in the benchmark. In our ops environment we are cutting blocks with roughly 200k records.

// map - with .Records 
BenchmarkAppender100-8     	   12934	     93423 ns/op	   21107 B/op	     214 allocs/op
BenchmarkAppender1000-8    	    1201	    984313 ns/op	  271893 B/op	    2035 allocs/op
BenchmarkAppender10000-8   	     100	  10653973 ns/op	 2316755 B/op	   20217 allocs/op
BenchmarkAppender200000-8   	       5	 247647939 ns/op	42067766 B/op	  407959 allocs/op
BenchmarkAppender500000-8   	       2	 770413231 ns/op	133247456 B/op	 1018929 allocs/op

// map - no .Records 
BenchmarkAppender100-8     	   15434	     77022 ns/op	   16980 B/op	     212 allocs/op
BenchmarkAppender1000-8    	    1490	    812231 ns/op	  230952 B/op	    2033 allocs/op
BenchmarkAppender10000-8   	     148	   8038403 ns/op	 1915067 B/op	   20214 allocs/op
BenchmarkAppender200000-8   	       7	 163810728 ns/op	34075778 B/op	  407998 allocs/op
BenchmarkAppender500000-8   	       3	 448853118 ns/op	113293720 B/op	 1019105 allocs/op

// skiplist - with .Records
BenchmarkAppender100-8     	    7124	    164523 ns/op	   35992 B/op	     707 allocs/op
BenchmarkAppender1000-8    	     706	   1856481 ns/op	  301532 B/op	    7007 allocs/op
BenchmarkAppender10000-8   	      64	  18889760 ns/op	 2948812 B/op	   70007 allocs/op
BenchmarkAppender200000-8   	       2	 594963092 ns/op	58828964 B/op	 1400007 allocs/op
BenchmarkAppender500000-8   	       1	1837878628 ns/op	147040840 B/op	 3500009 allocs/op

// skiplist - no .Records
BenchmarkAppender100-8     	    7400	    160864 ns/op	   31902 B/op	     706 allocs/op
BenchmarkAppender1000-8    	     662	   1643509 ns/op	  260568 B/op	    7006 allocs/op
BenchmarkAppender10000-8   	      66	  18660791 ns/op	 2547250 B/op	   70006 allocs/op
BenchmarkAppender200000-8   	       2	 555868270 ns/op	50813524 B/op	 1400006 allocs/op
BenchmarkAppender500000-8   	       1	1795158977 ns/op	127053896 B/op	 3500014 allocs/op

Which issue(s) this PR fixes:
Fixes #591

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
This reverts commit 97c95f5.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
This reverts commit 02b024b.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
This reverts commit 470d5a9.
This reverts commit 6247a77.
Signed-off-by: Joe Elliott <number101010@gmail.com>
Signed-off-by: Joe Elliott <number101010@gmail.com>
Copy link
Contributor

@annanay25 annanay25 left a comment

Choose a reason for hiding this comment

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

lgtm, nice!

Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Copy link
Member Author

Benchstat output of Record vs *Record:

name              old time/op    new time/op    delta
Appender100-8        101µs ±16%     106µs ±28%     ~     (p=0.400 n=10+9)
Appender1000-8      1.11ms ±28%    1.17ms ±32%     ~     (p=0.353 n=10+10)
Appender10000-8     12.2ms ±21%    12.4ms ±15%     ~     (p=0.481 n=10+10)
Appender200000-8     308ms ± 2%     274ms ±17%  -10.79%  (p=0.006 n=8+9)
Appender500000-8     1.10s ± 5%     0.86s ±26%  -21.73%  (p=0.000 n=10+9)
name              old alloc/op   new alloc/op   delta
Appender100-8       18.7kB ± 0%    21.1kB ± 0%  +12.83%  (p=0.000 n=9+10)
Appender1000-8       247kB ± 0%     272kB ± 0%  +10.01%  (p=0.000 n=10+10)
Appender10000-8     2.08MB ± 0%    2.32MB ± 0%  +11.54%  (p=0.000 n=10+10)
Appender200000-8    37.3MB ± 0%    42.1MB ± 0%  +12.88%  (p=0.000 n=10+10)
Appender500000-8     121MB ± 0%     133MB ± 0%   +9.89%  (p=0.000 n=10+10)
name              old allocs/op  new allocs/op  delta
Appender100-8          314 ± 0%       214 ± 0%  -31.85%  (p=0.000 n=10+10)
Appender1000-8       3.04k ± 0%     2.04k ± 0%  -32.95%  (p=0.000 n=10+10)
Appender10000-8      30.2k ± 0%     20.2k ± 0%  -33.09%  (p=0.000 n=10+9)
Appender200000-8      608k ± 0%      408k ± 0%  -32.89%  (p=0.000 n=10+10)
Appender500000-8     1.52M ± 0%     1.02M ± 0%  -32.92%  (p=0.000 n=10+10)

Record is faster and has less total allocations, but allocates more total memory.

@joe-elliott joe-elliott merged commit ab662b3 into grafana:main Apr 30, 2021
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.

Improve Ingester Record Insertion
3 participants