Skip to content

tsdb: more efficient sorting of postings read from WAL at startup#10500

Merged
codesome merged 2 commits intoprometheus:mainfrom
bboreham:improve-postings-sort
Mar 30, 2022
Merged

tsdb: more efficient sorting of postings read from WAL at startup#10500
codesome merged 2 commits intoprometheus:mainfrom
bboreham:improve-postings-sort

Conversation

@bboreham
Copy link
Member

This was something I noticed in passing.

Benchmark stats:

name                                                old time/op    new time/op    delta
MemPostings_ensureOrder/many_values_per_label-4        568ms ± 2%     567ms ± 2%      ~     (p=0.690 n=5+5)
MemPostings_ensureOrder/few_values_per_label-4         649ms ± 0%     659ms ± 5%      ~     (p=0.190 n=4+5)
MemPostings_ensureOrder/few_refs_per_label_value-4    28.5ms ± 3%    21.5ms ± 2%   -24.53%  (p=0.008 n=5+5)

name                                                old alloc/op   new alloc/op   delta
MemPostings_ensureOrder/many_values_per_label-4       24.0MB ± 0%     0.0MB ± 1%   -99.91%  (p=0.008 n=5+5)
MemPostings_ensureOrder/few_values_per_label-4        24.0MB ± 0%     0.0MB ± 0%   -99.90%  (p=0.016 n=5+4)
MemPostings_ensureOrder/few_refs_per_label_value-4    24.0MB ± 0%     0.0MB ± 6%  -100.00%  (p=0.008 n=5+5)

name                                                old allocs/op  new allocs/op  delta
MemPostings_ensureOrder/many_values_per_label-4        1.00M ± 0%     0.00M ± 0%  -100.00%  (p=0.008 n=5+5)
MemPostings_ensureOrder/few_values_per_label-4         1.00M ± 0%     0.00M ±12%  -100.00%  (p=0.008 n=5+5)
MemPostings_ensureOrder/few_refs_per_label_value-4     1.00M ± 0%     0.00M ± 0%  -100.00%  (p=0.008 n=5+5)

Most of the benefit comes from avoiding slice-to-interface allocation, by pulling the seriesRefSlice out of the loop, so the compiler doesn't allocate a new one on the heap every time.

The second commit to use a pointer type in the Pool reduces allocations two more orders of magnitude: this is almost invisible in benchmark timings after the first commit, but I couldn't leave it with a staticcheck warning :-)

Removed a comment that said fixing this has a performance penalty: not borne out by benchmarks.

Since benchstat rounded the numbers, the first commit goes to 987 allocs/iter, and the second to 9.

This is pulling the `seriesRefSlice` out of the loop, so the compiler
doesn't allocate a new one on the heap every time.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
As noted by staticcheck, Pool prefers the objects in the pool to have
pointer type. This is a little more fiddly to code, but avoids
allocation of a wrapper object every time a slice is put into the pool.

Removed a comment that said fixing this has a performance penalty: not
borne out by benchmarks.

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
@bboreham bboreham requested a review from codesome as a code owner March 29, 2022 19:21
Copy link
Member

@metalmatze metalmatze left a comment

Choose a reason for hiding this comment

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

Fantastic, as always! 💯

@codesome codesome merged commit 2c1be4d into prometheus:main Mar 30, 2022
@roidelapluie
Copy link
Member

/prombench v2.34.0

@prombot
Copy link
Contributor

prombot commented Apr 12, 2022

⏱️ Welcome to Prometheus Benchmarking Tool. ⏱️

Compared versions: PR-10500 and v2.34.0

After successful deployment, the benchmarking metrics can be viewed at:

Other Commands:
To stop benchmark: /prombench cancel
To restart benchmark: /prombench restart v2.34.0

@roidelapluie
Copy link
Member

roidelapluie commented Apr 13, 2022

This PR has the memory leak apparently. I am checking if it is the first one.

@bboreham
Copy link
Member Author

It seems to take several hours to kick in, which is interesting if it's caused by a change that only runs during startup.

image

@roidelapluie
Copy link
Member

/prombench cancel

@prombot
Copy link
Contributor

prombot commented Apr 14, 2022

Benchmark cancel is in progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants