Reuse float histogram objects #13276

fpetkovski · 2023-12-11T07:19:55Z

This commit builds upon #13215, and reduces memory for querying native histogram by reusing existing HPoint instances.

Benchmarks show ~23% improvement in performance and ~45% reduction in memory usage. Queries without rate are not affected yet. To make improvements there we will need to change the iterator interface and I will do that in a follow up PR.

benchstat main.out new.out                                                                    
name                         old time/op    new time/op    delta
NativeHistograms/sum-8          521ms ± 2%     522ms ± 3%     ~     (p=1.000 n=5+5)
NativeHistograms/sum_rate-8     1.11s ± 3%     0.85s ± 8%  -23.15%  (p=0.008 n=5+5)

name                         old alloc/op   new alloc/op   delta
NativeHistograms/sum-8          415MB ± 0%     414MB ± 0%     ~     (p=0.548 n=5+5)
NativeHistograms/sum_rate-8    1.16GB ± 0%    0.64GB ± 0%  -44.94%  (p=0.008 n=5+5)

name                         old allocs/op  new allocs/op  delta
NativeHistograms/sum-8          5.16M ± 0%     5.16M ± 0%     ~     (p=0.841 n=5+5)
NativeHistograms/sum_rate-8     21.1M ± 0%     10.3M ± 0%  -51.26%  (p=0.008 n=5+5)

This commit reduces the memory needed to query native histogram objects by reusing existing HPoint instances. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

My PR for adding quantile_over_time support increased allocations in the matrix selector due to creating a new []float64 slice for each call to the aligner function. This commit resolves that issue by reusing the same slice, and applies the trick in prometheus/prometheus#13276 to further drive down memory usage. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

* Reduce allocations in matrix selector My PR for adding quantile_over_time support increased allocations in the matrix selector due to creating a new []float64 slice for each call to the aligner function. This commit resolves that issue by reusing the same slice, and applies the trick in prometheus/prometheus#13276 to further drive down memory usage. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix histograms Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> --------- Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

beorn7

Looks good in general. Just two smallish nits.

beorn7 · 2023-12-12T16:43:08Z

promql/engine.go

+				if n := len(floats); n < cap(floats) {
+					floats = floats[:n+1]
+					floats[n].T, floats[n].F = t, f
+				} else {
+					floats = append(floats, FPoint{T: t, F: f})
+				}


I see the reason for the cap hack above for histograms (so that we can get a previously allocated histogram for re-use), but with float64, we don't need to re-use anything allocated.

Isn't append under the hood doing the same thing as you have implemented in the 1st branch of the if clause, provided the capacity is sufficient? Do you see any difference in the benchmark if you don't do this change?

I thought we could provide a consistent way to resize the buffer for both floats and histograms, and also save an allocation for FPoint. However, benchmarks show 0% difference so reverting the change makes sense.

beorn7 · 2023-12-12T16:49:33Z

storage/buffer.go

 	return it.t, it.h
 }

-func (it *sampleRingIterator) AtFloatHistogram() (int64, *histogram.FloatHistogram) {
+func (it *SampleRingIterator) AtFloatHistogram(fh *histogram.FloatHistogram) (int64, *histogram.FloatHistogram) {


Now that SampleRingIterator doesn't implement chunkenc.Iterator anymore, it would be good to have doc comments. At least for this method, that now has an additional parameter.

I've added comments on all methods. I also removed Seek and Err because they were never called. Also Seek was poorly implemented and always returned chunkenc.ValNone. Let me know if I should revert this change.

Yead, Seek was only implemented to implement the chunkenc.Iterator interface. But as proven by your change, we don't need that.

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

beorn7

Thank you very much.

In prometheus#13276 we started reusing float histogram objects to reduce allocations in PromQL. That PR introduces a bug where histogram pointers gets copied to the beginning of the histograms slice, but are still kept in the end of the slice. When a new histogram is read at the last element, it can overwrite a previous element because the pointer is the same. This commit fixes the issue by moving outdated points to the end of the slice so that we don't end up with duplicate pointers in the same buffer. In other words, the slice gets rotated so that old objects can get reused. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

In prometheus#13276 we started reusing float histogram objects to reduce allocations in PromQL. That PR introduces a bug where histogram pointers gets copied to the beginning of the histograms slice, but are still kept in the end of the slice. When a new histogram is read into the last element, it can overwrite a previous element because the pointer is the same. This commit fixes the issue by moving outdated points to the end of the slice so that we don't end up with duplicate pointers in the same buffer. In other words, the slice gets rotated so that old objects can get reused. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

This commit updates Prometheus to commit d0c2d9c which contains the loser-tree based postings merge. Note that we cannot update to latest main since there is currently a bug in PromQL when querying native histograms introduced by prometheus/prometheus#13276. This issue should be fixed by prometheus/prometheus#13289. This commit also updates the Thanos PromQL engine to latest main. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

* Update prometheus to d0c2d9c This commit updates Prometheus to commit d0c2d9c which contains the loser-tree based postings merge. Note that we cannot update to latest main since there is currently a bug in PromQL when querying native histograms introduced by prometheus/prometheus#13276. This issue should be fixed by prometheus/prometheus#13289. This commit also updates the Thanos PromQL engine to latest main. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix handler_test.go Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Fix manager_test.go Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> * Use empty registry for file discovery Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com> --------- Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

This commit optimizes queries against native histograms by reusing histogram objects for windowing functions. The same optimization was applied in Prometheus in prometheus/prometheus#13276 and prometheus/prometheus#13289.

This commit optimizes queries against native histograms by reusing histogram objects for windowing functions. The same optimization was applied in Prometheus in prometheus/prometheus#13276 and prometheus/prometheus#13289. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski requested a review from roidelapluie as a code owner December 11, 2023 07:19

Reuse float histogram objects

e2a9f8a

This commit reduces the memory needed to query native histogram objects by reusing existing HPoint instances. Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the reuse-float-histograms branch from 1719b3c to e2a9f8a Compare December 11, 2023 07:25

Export SampleRingIterator

48df9fc

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

fpetkovski force-pushed the reuse-float-histograms branch from 9fb3899 to 48df9fc Compare December 11, 2023 10:18

fpetkovski mentioned this pull request Dec 11, 2023

Reduce allocations in matrix selector thanos-io/promql-engine#356

Merged

beorn7 self-requested a review December 12, 2023 10:05

beorn7 requested changes Dec 12, 2023

View reviewed changes

fpetkovski added 2 commits December 13, 2023 08:30

Add comment on SampleRingIterator

bb8363d

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

Add comment on SampleRingIterator methods

ea356c4

Signed-off-by: Filip Petkovski <filip.petkovsky@gmail.com>

beorn7 approved these changes Dec 13, 2023

View reviewed changes

beorn7 merged commit 775de1a into prometheus:main Dec 13, 2023
24 checks passed

fpetkovski mentioned this pull request Dec 14, 2023

Fix reusing float histograms #13289

Merged

fpetkovski mentioned this pull request Dec 14, 2023

Update prometheus to d0c2d9c thanos-io/thanos#6978

Merged

2 tasks

fpetkovski mentioned this pull request Dec 27, 2023

Optimize native histograms thanos-io/promql-engine#362

Merged

bboreham mentioned this pull request Jan 29, 2024

Cut v2.50.0-rc.0 #13465

Merged

chenrui333 mentioned this pull request Feb 22, 2024

prometheus 2.50.0 Homebrew/homebrew-core#163774

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reuse float histogram objects #13276

Reuse float histogram objects #13276

fpetkovski commented Dec 11, 2023 •

edited

beorn7 left a comment

beorn7 Dec 12, 2023

fpetkovski Dec 13, 2023

beorn7 Dec 12, 2023

fpetkovski Dec 13, 2023

beorn7 Dec 13, 2023

beorn7 left a comment

Reuse float histogram objects #13276

Reuse float histogram objects #13276

Conversation

fpetkovski commented Dec 11, 2023 • edited

beorn7 left a comment

Choose a reason for hiding this comment

beorn7 Dec 12, 2023

Choose a reason for hiding this comment

fpetkovski Dec 13, 2023

Choose a reason for hiding this comment

beorn7 Dec 12, 2023

Choose a reason for hiding this comment

fpetkovski Dec 13, 2023

Choose a reason for hiding this comment

beorn7 Dec 13, 2023

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

fpetkovski commented Dec 11, 2023 •

edited