Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 0-materialized-column-field-iter #559 +/- ##
===================================================================
Coverage ? 78.18%
===================================================================
Files ? 251
Lines ? 19284
Branches ? 0
===================================================================
Hits ? 15077
Misses ? 4207
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
cheb0
changed the title
| for i, mid := range mids { | ||
| if sources[i] < 0 { |
There was a problem hiding this comment.
Are you sure that sizes of these two arrays (mids and sources) are always identical?
You extract mids based on non-filtered lids:
func (n *SingleSourceCountAggregator) Next(lids []node.LID) error {
/* ... */
mids := n.extractMID(lids, n.midsBuf)
n.midsBuf = mids[:0]
/* ... */
}| s.countBySource[s.lastSource]++ | ||
| if s.uniqSourcesLimit.limit > 0 && len(s.countBySource) > s.uniqSourcesLimit.limit { | ||
| return lid.Unpack(), true, fmt.Errorf("%w: iterator limit is exceeded", s.uniqSourcesLimit.err) | ||
| s.countBySource[s.lastSource]++ |
There was a problem hiding this comment.
Here hashmap is still used and it is on the hot-path.
I guess you could reuse the same chunked-array strategy here. Or you could remove interfaces at all -- you have to count occurrences of tokens anyway.
Take a look at the patch I propose: no-interface.patch. In my opinion, code is much simpler and I believe it will show the same performance (or even slightly better, I guess).
There was a problem hiding this comment.
I've run quick benchmark and got slightly better results. Could you verify this against your dataset?
| Query | Type | mean (ms) |
stddev (ms) |
p(50) (ms) |
p(95) (ms) |
p(99) (ms) |
iterations |
total |
||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| base | comp | diff | base | comp | diff | base | comp | diff | base | comp | diff | base | comp | diff | base | comp | diff | base | comp | diff | ||
|
hot | 110.84 |
104.62 |
-5.61% |
12.97 |
13.52 |
+4.24% |
109.88 |
106.42 |
-3.14% |
130.10 |
125.40 |
-3.61% |
142.65 |
134.39 |
-5.79% |
90 |
90 |
0.00% |
20061562 |
20061562 |
0.00% |
There was a problem hiding this comment.
Here hashmap is still used and it is on the hot-path.
Yes, countBySource is on the hot path. However, the ultimate plan is to wipe it our altogether. I'll explain.
Why do we need it? The author of the package is not with us anymore, so I tried to mentally reconstruct the thinking process behind this map.
countBySource serves two purposes:
- it counts a number of unique sources which are part of the current agg for limiting
- it counts a number of lids per each source
Why does it count a number of occurrences for each source? Apparently the answer is another map - tokensCache. Why do we need tokensCache? Because same source can be value-resolved multiple times. For time series agg a number of buckets for same sources can be literally anything, so same source can be resolved multiple times. This is why tokensCache is used for every source which has count of occurrences >= 2.
So, the whole point of of chunked array is that it's source-ordered (perf boost is extra, I'd be happy if chunked array had same perf as map). If it's source ordered, then I don't need tokensCache - I can just iterate all buckets in source-order. Hense we don't need a sort+prefetch in Aggregate.
The tricks it to do this for all single source aggregators (if possible). If tokensCache is not needed, then countBySource can be literally a bool array. Calculate the legnth of bool array can be slow, but I'm sure limits will be ok if I calc the length once-per-batch.
The tricky part is to do all of this for TwoSourceAggregator. It will be way harder to do source-ordered collections (though possible, I haven't thought about this). So, my plan is to create another iterator - something like TokenCachingSourcedNodeIterator which can wrap a SourcedNodeIterator. It can be used in TwoSourceAggregator. It will have tokensCache and countBySource inside. prefetch+sort can be pushed up to two source aggregator for now, untill we start working on two source agg.
So, this is the plan for now. I'm not sure if everything works as I expect, but looks sane to me. This is why I don't touch countBySource now - the simple reason is I'm trying to delete it altogether.
There was a problem hiding this comment.
countBySource serves two purposes
Yep, this is correct.
then countBySource can be literally a bool array
Wait, but how in this case you would know how many documents contain token countBySource is necessary. Am I missing something?
In my patch countBySource is the only array that is used for counting.
There was a problem hiding this comment.
Yes, in my plan counter (chunked array) stays here, same as now, in plainSourceCounter. countBySource will eventually be downgraded to bool array/bitset, so there is no counting in aggregations which do not need it.
There was a problem hiding this comment.
Why you need to store two almost identical arrays instead of one? Only one countBySource in SourcedNodeIterator is necessary.
| aggMap := make(map[seq.AggBin]*seq.SamplesContainer, n.group.UniqueSources()) | ||
| // tsSourceCounter is a counter used in time series count aggregations. | ||
| type tsSourceCounter struct { | ||
| counts map[AggBin[int]]int64 |
There was a problem hiding this comment.
I guess you could apply the same chunked-array strategy here as well. As a follow-up, of course.
There was a problem hiding this comment.
Yes, you are right. I'm working on it right now. Techincally, it's easy to have an array of arrays (chunked array of arrays to be exact, i.e. 3D array). The entry can be a small struct of MID+count.
However, it's gonna suck when interval is low as 1ms and it's a low cardinality agg. So, I'm trying to come up with some sort of hybrid structure where we use 3D array when number of buckets is low (it's gonna be true in 99% scenarios) and spill to hash-map when there are too many buckets.
There was a problem hiding this comment.
hybrid structure where we use 3D array
Interesting. 3D-array implies three pointer dereferences -- I wonder how cache locality in this case will affect performance.
1c8fc56 to
830b602
Compare
830b602 to
7d8acb7
Compare
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
This branch has not been deployed
4 participants
Description
Aggregatornow accepts batches of lids,SourceNodeIteratorrerturns a batch of sourcesSingleSourceCountAggregatornow delegates aggregation logic tosourceCounterinterface, which has two implementations:plainSourceCounterhandles ordinary aggs, uses a chunked array for counting.tsSourceCounterhandles timeseries aggs, uses mapArray is better than map when we have many many lids processed in agg, but can be slower when the number of lids is low and agg is high cardinality (we need to allocate an array). This is partially handled by "chunking" the array.
If you have used LLM/AI assistance please provide model name and full prompt:
Stack created with GitHub Stacks CLI • Give Feedback 💬