Significantly improve performance of FetchResultToPromResult and helper functions #1003
Conversation
richardartoul
requested review from
nikunjgit,
robskillington,
arnikola,
prateek and
benraskin92
and removed request for
nikunjgit and
robskillington
| benchResult *prompb.QueryResult | ||
| ) | ||
|
|
||
| func BenchmarkFetchResultToPromResult(b *testing.B) { |
There was a problem hiding this comment.
Might be useful to paste the benchmark results in a comment here just for easier comparisons in the future?
| // Perform bulk allocation upfront then convert to pointers afterwards | ||
| // to reduce total number of allocations. See BenchmarkFetchResultToPromResult | ||
| // if modifying. | ||
| timeseries := make([]prompb.TimeSeries, 0, len(result.SeriesList)) |
There was a problem hiding this comment.
Nice pattern, we can probably make use fo this approach elsewhere too
| for i := 0; i < numDatapointsPerSeries; i++ { | ||
| values = append(values, ts.Datapoint{ | ||
| Timestamp: time.Time{}, | ||
| Value: float64(i), |
There was a problem hiding this comment.
nit: maybe use a random value and time.Now() instead?
Codecov Report
@@ Coverage Diff @@
## master #1003 +/- ##
=========================================
- Coverage 77.6% 75.21% -2.4%
=========================================
Files 412 407 -5
Lines 34653 34474 -179
=========================================
- Hits 26892 25929 -963
- Misses 5887 6641 +754
- Partials 1874 1904 +30
Continue to review full report at Codecov.
|
src/query/storage/converter.go
Outdated
| func SeriesToPromTS(series *ts.Series) prompb.TimeSeries { | ||
| labels := TagsToPromLabels(series.Tags) | ||
| samples := SeriesToPromSamples(series) | ||
| return prompb.TimeSeries{Labels: labels, Samples: samples} | ||
| } | ||
|
|
||
| // TagsToPromLabels converts tags to prometheus labels | ||
| // TagsToPromLabels converts tags to prometheus labels.TagsToPromLabels. |
There was a problem hiding this comment.
Looks like a find+replace issue?
| } | ||
| var ( | ||
| seriesLen = series.Len() | ||
| values = series.Values() |
There was a problem hiding this comment.
nit: we only use these in one place so probably prefer to drop the var for it and just use the accessor?
There was a problem hiding this comment.
nah because then it gets called on every iteration of the loop. Moving these out into vars actually made a significant impact (20%ish percent improvement or so)
| }) | ||
| } | ||
|
|
||
| samplesPointers := make([]*prompb.Sample, 0, len(samples)) |
There was a problem hiding this comment.
nit; alternatively can use seriesLen here too
There was a problem hiding this comment.
just seemed safer to use the length of the thing I'm actually iterating through
Before
After