avoid unneeded copy on touching but non-overlaping byte ranges #4219
Conversation
trinity-1686a
force-pushed
the
trinity--avoid-unneeded-copy
branch
from
660ee3b
to
790dd44
Compare
trinity-1686a
force-pushed
the
trinity--avoid-unneeded-copy
branch
from
790dd44
to
904dfd9
Compare
| .map(|(k, _v)| k); | ||
|
|
||
| let end_key = CacheKey::from_borrowed(tag.borrow(), byte_range.end - 1); |
There was a problem hiding this comment.
my understanding is that with this, we remove (as opposed to merge) any data that is overlapping (not just is included in) with the slice we are trying to put.
Is my understanding correct?
If so can we comment this more clearly?
possibly isolate in a different method:
fn remove_overlapping_slices(&mut self, ....)
There was a problem hiding this comment.
I'm not sure I understand your query.
In this function, the only change is to what we consider for merge.
Before, if someone would put [10..15), we would search for something where either starting or ending bound (or both) is in [10..=15]. This includes [5..10) and [15..20).
After, we only consider things in [11..=14], that is, things with which we share at least one byte. If we share something, the merge happens as before, if the new rule consider we don't overlap, the range is added to the cache without merging anything
There was a problem hiding this comment.
yes: correction.
we merge any data that is overlapping (not just touches) with the slice we are trying to put.
could we comment what we are doing, or even better isolate it into a method?
Description
when a
ByteRangeCachehas to store bytes for the range 0..100_000 and the range 100_000..100_100, it currently merged both range, issuing a memcpy which takes up to a few milliseconds. In practice this is almost never useful, as future get are unlikely to make a request that cross boundaries. This PR removes that merging on range that touch but do not overlap. When fetching from cache, if the cache realize it can only server a query by merging multiple ranges. This impact mostly the warmup of fastfields. On a samplefind_trace_idsrequest, done on a single searcher over ~1b spans, it lowered query time from ~5.8s to ~4.1sHow was this PR tested?
added unit test, manually checked there is a performance improvement