Speedup time_series agg by caching current tsid ordinal, parent bucket ordinal and buck ordinal

[martijnvg]

This avoids needlessly adding the same parent bucket ordinal or TSIDs to BytesKeyedBucketOrds.

Relates to #74660

[elasticsearchmachine]

Pinging @elastic/es-analytics-geo (Team:Analytics)

[jpountz]

It looks like this aggregation is always called with bucket == 0 so maybe remove this currentBucket == bucket condition here and replace it with an assertion that bucket is zero?

[martijnvg]

I think that is only the case if time_series aggregation is a top level aggregation.
I don't think this is always the case and for example a date_histogram aggregation is a likely parent aggregation in the case of time_series aggregation.

[jpountz]

Ah, that makes sense. Then maybe the currentBucket == bucket condition is a bit fragile as it would prevent the optimization from kicking in if this collector is called with different bucket ordinals even though it the TSID doesn't change. What about changing currentTsidOrd and currentBucketOrdinal to arrays that are keyed by bucket, ie. tracking separately the current TSID and bucket ordinal per bucket?

[martijnvg]

I think the case we know there isn't a parent aggregation then we can return a leaf bucket collector that only does the currentTsidOrd == aggCtx.getTsidOrd() check.

[nik9000]

What about changing currentTsidOrd and currentBucketOrdinal to arrays that are keyed by bucket, ie. tracking separately the current TSID and bucket ordinal per bucket?

We'd need a BigArray thing because we can have many buckets above us and we'd want to track the memory. It could be useful though. If you ever "come back" to the same bucket. But I don't think, say, date_histogram will. I think once it's finished collecting some date for the time series it'll never come back.

I think the case we know there isn't a parent aggregation then we can return a leaf bucket collector that only does the currentTsidOrd == aggCtx.getTsidOrd() check.

That's what the CardinalityUpperBound bucketCardinality argument to the ctor is for. If it's exactly one you know you don't have a parent agg or it always makes a single bucket.

[jpountz]

Actually things should be fine with a terms aggregation as a parent too, because almost all the time this terms aggregation would run on a field that is a dimension or a tag. So bucket would only change when tsidOrd changes too?

[martijnvg]

Yes, I think that is true if the field of the terms agg is a dimension field. But maybe not in the case if the field is a non dimension field (label field)?

[jpountz]

Right. My intuition is that we don't have to care much about this case of labels that could take different values for the same TSID (as long as the aggregation output is correct, just less optimized), do we?

[martijnvg]

My intuition is that we don't have to care much about this case of labels that could take different values for the same TSID (as long as the aggregation output is correct, just less optimized), do we?

This is what I think as well. This change favours parent aggs based on dimension fields, @timestamp or no parent aggregation. For the other cases, this change doesn't help, but I think doesn't hurt either. We are always correct, since we then fall back to checking whether we have seen tsid & bucket combination in BytesKeyedBucketOrds.

[martijnvg]

I added a comment: f23de49

[nik9000]

What about changing currentTsidOrd and currentBucketOrdinal to arrays that are keyed by bucket, ie. tracking separately the current TSID and bucket ordinal per bucket?

We'd need a BigArray thing because we can have many buckets above us and we'd want to track the memory. It could be useful though. If you ever "come back" to the same bucket. But I don't think, say, date_histogram will. I think once it's finished collecting some date for the time series it'll never come back.

I think the case we know there isn't a parent aggregation then we can return a leaf bucket collector that only does the currentTsidOrd == aggCtx.getTsidOrd() check.

That's what the CardinalityUpperBound bucketCardinality argument to the ctor is for. If it's exactly one you know you don't have a parent agg or it always makes a single bucket.

[nik9000]

If we have a tsidOrd can we use that as a key instead of the bytes?

[martijnvg]

So we need the tsid as bytes in the buildAggregations() method and I don't think we can use this tsidord that TimeSeriesIndexSearcher generates as a lookup to get the tsid?

[jpountz]

Indeed this tsidord is neither a segment ordinal nor a global ordinal but an ordinal for TSIDs that intersect with the query on this shard. So it can't be used as a key to retrieve TSIDs from the terms dictionary. (Separately, maybe we should rename this method to reduce chances that someone mistakenly uses it as an ordinal for the _tsid terms dict? I worry a bit that it would look like it works for common cases like the query only consists of a range query on the @timestamp and this time range happens to match every unique TSID, so tests might not even catch the problem if someone started doing this?)

[martijnvg]

Let's rename the method in a follow up change?

[jpountz]

Looks great.

[martijnvg]

Benchmarking this change to time series aggregation by using the query added in elastic/rally-tracks#348 results in a good improvement:

|                                                Min Throughput | date-histo-with-time-series-1h |     0.036549    |     0.0854548   |      0.04891 |  ops/s | +133.81% |
|                                               Mean Throughput | date-histo-with-time-series-1h |     0.0366182   |     0.0855126   |      0.04889 |  ops/s | +133.53% |
|                                             Median Throughput | date-histo-with-time-series-1h |     0.0366221   |     0.0854992   |      0.04888 |  ops/s | +133.46% |
|                                                Max Throughput | date-histo-with-time-series-1h |     0.0366767   |     0.0856078   |      0.04893 |  ops/s | +133.41% |
|                                       50th percentile latency | date-histo-with-time-series-1h | 27112.2         | 11575           | -15537.2     |     ms |  -57.31% |
|                                       90th percentile latency | date-histo-with-time-series-1h | 27310.6         | 11748.9         | -15561.7     |     ms |  -56.98% |
|                                       99th percentile latency | date-histo-with-time-series-1h | 27600           | 11901.7         | -15698.4     |     ms |  -56.88% |
|                                      100th percentile latency | date-histo-with-time-series-1h | 27797.9         | 12036.8         | -15761.1     |     ms |  -56.70% |
|                                  50th percentile service time | date-histo-with-time-series-1h | 27112.2         | 11575           | -15537.2     |     ms |  -57.31% |
|                                  90th percentile service time | date-histo-with-time-series-1h | 27310.6         | 11748.9         | -15561.7     |     ms |  -56.98% |
|                                  99th percentile service time | date-histo-with-time-series-1h | 27600           | 11901.7         | -15698.4     |     ms |  -56.88% |
|                                 100th percentile service time | date-histo-with-time-series-1h | 27797.9         | 12036.8         | -15761.1     |     ms |  -56.70% |
|                                                    error rate | date-histo-with-time-series-1h |     0           |     0           |      0       |      % |    0.00% |