attempt to optimize sorted_ords_to_term_cb #2664
Conversation
|
we could get away with always returning the next block 1st ordinal, that looked like a regression in some bench, but it's also possible that was only noise |
|
|
||
| /// Get the [`BlockAddr`] of the block containing the `ord`-th term. | ||
| pub(crate) fn get_block_with_ord(&self, ord: TermOrdinal) -> BlockAddr { | ||
| pub(crate) fn get_block_with_ord<const FETCH_NEXT_ORD: bool>( |
There was a problem hiding this comment.
I think FETCH_NEXT_ORD could be just a regular parameter. With inlining it will be the same code, and without I think it won't make a performance difference.
There was a problem hiding this comment.
There are quite a bunch of fatty calls in between. I wouldn't bet on inlining happening here.
There was a problem hiding this comment.
also the goal of having this as a const generics was to not impact perf when we don't care about the upper bound ord, which isn't very expensive, but isn't free to fetch. But it looks like most benchs having regressed might not be using sstables at all (noisy bench?), so it's possible this added complexity brings nothing
| } | ||
|
|
||
| fn binary_search_ord(&self, ord: TermOrdinal) -> (u64, BlockAddr) { | ||
| fn binary_search_ord<const FETCH_NEXT_ORD: bool>( |
There was a problem hiding this comment.
A comment to explain this signature please. This is not obvious at all.
make it so that operation doesn't need to query the index nearly as much (that operation isn't free on index v3)