chunkIterator: optimize caching and AtT() calculation

[duricanikolic]

What this PR does

This PR improves the current implementation of iterators.chunkIterator.AtT(). Namely, if a cached value is not valid, the current implementation returns the timestamp obtained as a result of loading an appropriate value from the underlying iterator (of type chunk.Iterator). In order to retrieve the current timestamp it is, though, not necessary to load a new value, since the timestamp can be taken directly by calling the underlying iterator's Timestamp() function.

For example:

var it chunkIterator = ... // we create an instance of type chunkIterator

it.Next() // we initialize the iterator, but we don't load any value
ts := it.AtT() // we get the current timestamp from the underlying iterator, although we have never loaded a value

t, h := it.AtHistogram(nil) // we load and cahce a histogram
ts = it.AtT() // we get the current timestamp, which has been cached, i.e., t == ts

it.Next() // this call invalidates the cache, but moves to the next position in the underlying iterator
ts = it.AtT() // we get the new timestamp from the underlying iterator, without loading its value. t != ts

Moreover, this PR simplifies the checking whether the cached value is valid: auxiliary fields cachedValueValid, cachedHistogramValid and cachedFloatHistogramValid have been replaced with just one field cacheValid.

A comparison between the old and the new way of fetching the current timestamp showed that the latter requires less CPU:

                                        │   old.txt   │              new.txt               │
                                        │   sec/op    │   sec/op     vs base               │
ChunkIterator_AtT/float_chunk             44.90µ ± 4%   44.68µ ± 1%       ~ (p=0.190 n=10)
ChunkIterator_AtT/histogram_chunk         52.01µ ± 2%   46.99µ ± 2%  -9.65% (p=0.000 n=10)
ChunkIterator_AtT/float_histogram_chunk   54.04µ ± 1%   49.61µ ± 1%  -8.20% (p=0.000 n=10)
geomean                                                50.16µ        47.05µ       -6.20%

Which issue(s) this PR fixes or relates to

Part of #7235

Checklist

• Tests updated.
• [na] Documentation added.
• [na] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
• [na] about-versioning.md updated with experimental features.

[56quarters]

Change LGTM but I'll let @krajorama 👍 it

[krajorama]

I think we're on the right track , however this might lead to a degradation in context of the chunk merge iterator. Currently in the heap sort there we'd not do any call through an interface due to the caching. Please check if this iterator is used in that context and let's do some benchmark with the chunk merge iterator. https://github.com/grafana/mimir/blob/main/pkg/querier/iterators/chunk_merge_iterator.go

It might turn out that what we really want is to just cache the timestamp and literally nothing else - in case values are read once and never multiple times.

[duricanikolic]

I have gone through the implementation of chunkMergeIterator, and these are my observations:

• calls to AtT(), AtFloat(), AtHistogram(), AtFloatHistogram() never call a corresponding method on chunkIterator, and always return only cached values, if any.
• building a new chunkMergeIterator:
  • the constructor of chunkMergeIterator creates a list of non-overlapping iterators, calls a Next() on each of the non-overlapping iterators, and finally initializes a heap of non-overlapping chunks.
  • the Next() on a non-overlapping iterator calls the Next() on the current chunk of type chunkIterator belonging to the former, which updates the timeseries and type of the underlying iterator of the latter.
  • the constructor doesn't cache any value, so calls to AtT(), AtFloat(), AtHistogram(), AtFloatHistogram() would return erroneous values, which is correct, since it is supposed to call Next() or Seek() before any of the At*() functions
• calling chunkMergeIterator.Next():
  • this method loads the value from the topmost element in the heap, by calling At(), AtHistogram() or AtFloatHistogram(), depending on the type, caches the returned timestamp and value. Then it calls the Next() on the topmpost element in the heap, so that its timestamp and value type are updated for a successive calls to Next()
  • this means that, after a call to chunkMergeIterator.Next(), it is safe to return cached values when calling AtT(), At(), AtHistogram() and AtFloatHistogram(), because these values have actually been loaded from the corresponding chunkIterator during the chunkMergeIterator.Next().
• calling chunkMergeIterator.Seek():
  • this method calls the Seek() on all the non-overlapping iterators belonging to chunkMergeIterator, and then updates the heap.
  • after that, it loads the value from the topmost element in the heap, by calling At(), AtHistogram() or AtFloatHistogram(), depending on the type, caches the returned timestamp and value.
  • this means that, after a call to chunkMergeIterator.Next(), it is safe to return cached values when calling AtT(), At(), AtHistogram() and AtFloatHistogram(), because these values have actually been loaded from the corresponding chunkIterator during the chunkMergeIterator.Seek().

[krajorama]

Discussed IRL: we'll cache the timestamp independently of the value via updates in Next() and Seek(). This will allow the seriesIteratorHeap to be extremely efficient. While at the same time get rid of calling AtHistogram() and AtFloatHistogram() with nil, which is for sure wasteful.