Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
=======================================
Coverage 71.42% 71.43%
=======================================
Files 205 205
Lines 14910 14918 +8
=======================================
+ Hits 10650 10656 +6
- Misses 3484 3485 +1
- Partials 776 777 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔴 Performance DegradationSome benchmarks have degraded compared to the previous run. Show table
|
| if i > 0 { | ||
| entry := data.Entries[i-1] | ||
| if tid <= entry.getLastTID() { | ||
| return entry |
There was a problem hiding this comment.
Seems like this check is redundant. We checked earlier in line 79 that there MUST exist an entry which has StartTID <= tid && tid <= LastTID (at least at 0-th index). So we can rewrite it simpler:
for _, data := range t {
from := data.Entries[0].StartTID
to := data.Entries[len(data.Entries)-1].getLastTID()
if tid < from || tid > to {
continue
}
i := sort.Search(len(data.Entries), func(j int) bool {
return data.Entries[j].StartTID > tid
})
return data.Entries[i-1]
}And what's important that for sort.Search we have following documentation:
Search uses binary search to find and return the smallest index i in [0, n) at which f(i) is true, assuming that on the range [0, n), f(i) == true implies f(i+1) == true.
And also there is:
If there is no such index, Search returns n.
So sort.Search cannot return index that is less or equal to 0 in this case.
There was a problem hiding this comment.
Personally, I would add invariant check here as well:
for _, data := range t {
from := data.Entries[0].StartTID
to := data.Entries[len(data.Entries)-1].getLastTID()
if tid < from || tid > to {
continue
}
i := sort.Search(len(data.Entries), func(j int) bool {
return data.Entries[j].StartTID > tid
})
if i <= 0 {
panic("invariant violation")
}
return data.Entries[i-1]
}but that's unnecessary of course.
Description
For some aggs, half of time is spent just finding TID entry. Example:
_exists_:remote_addr | group by remote_addrTotal number of buckets found:
492756Timings:
5481 ms=>2151 ms