tsdb: stop saving a copy of last 4 samples in memSeries#11296
tsdb: stop saving a copy of last 4 samples in memSeries#11296codesome merged 5 commits intoprometheus:mainfrom
Conversation
|
Sadly the unit tests say that the race is not avoided. I will look some more. |
|
I don't think my change to My change assumes the caller never tries to read the last byte if it's unsafe to do. So, in |
c67f1ab to
4b030a6
Compare
|
I have updated this PR to take a copy of the last byte in |
Because the data is stored as a bit-stream, the last byte in the stream could change if the stream is appended to after an Iterator is obtained. Copy the last byte when the Iterator is created, so we don't have to read it later. Clarify in comments that concurrent Iterator and Appender are allowed, but the chunk must not be modified while an Iterator is created. (This was already the case, in order to copy the bstream slice header.) Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
This extra copy of the last 4 samples was introduced to avoid a race condition between reading the last byte of the chunk and writing to it. But now we have fixed that by having `bstreamReader` copy the last byte, we don't need to copy the last 4 samples. This change saves 56 bytes per series, which is very worthwhile when you have millions or tens of millions of series. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Previous changes have left this code duplicating some lines; pull them out to a separate function and tidy up. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
c698b83 to
c222cd4
Compare
pracucci
left a comment
There was a problem hiding this comment.
Good job, Bryan! I double checking the locking when appending samples to the chunk and creating the iterator, and I can't find any issue 👏
The behaviour has changed so chunk iterators are only wrapped when transaction isolation requires them to stop short of the end. This makes tests fail which are checking the type. Tests should check the observable behaviour, not the type. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
c222cd4 to
b844d29
Compare
Signed-off-by: Ganesh Vernekar <ganeshvern@gmail.com>
This extra copy of the last 4 samples was introduced to avoid a race condition between reading the last byte of the chunk and writing to it.
However this race condition was fixed in f42ed03, so we don't need the copy any more.EDIT: But we can copy just the last byte in
bstreamReaderinstead.This change saves 56 bytes per series, which is very worthwhile when you have millions of series.
(#11280 and #11288 each remove another 8 bytes)
I kept the head snapshot binary compatible by writing zeros where the samples used to be and ignoring any values found, but perhaps we should bump the version and save the work.