fix comparison count for format_version=3 indexes #6650
Conversation
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
ajkr
force-pushed
the
fix-comparison-accounting
branch
from
9d47cf6
to
065fd7c
Compare
|
@ajkr has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/dbformat.h
Outdated
| @@ -214,7 +214,7 @@ class InternalKeyComparator | |||
| virtual void FindShortSuccessor(std::string* key) const override; | |||
|
|
|||
| const Comparator* user_comparator() const { | |||
| return user_comparator_.user_comparator(); | |||
| return &user_comparator_; | |||
There was a problem hiding this comment.
There seems to be a lot of caller to this functions. Are we fixing accounts for all of them, or is there a chance we overcount somewhere?
There was a problem hiding this comment.
I think about it again. Can we create a new function that returns the wrapper instead? In that case, the user of the function will be explicit about whether the comparisons are counted underlying or not. And it can also potentially avoid one virtual function call.
There was a problem hiding this comment.
Do you mind helping me understand what kind of comparison should not be counted when PerfContext is enabled? I am not that familiar with this wrapper so am still naively hoping we can land in one of the following simple situations:
- the wrapper is cheap and we allow it to count all comparisons
- the wrapper is expensive so we get rid of it entirely
There was a problem hiding this comment.
Never mind, I saw there's an existing pattern to follow (call UserComparatorWrapper(rep_->internal_comparator.user_comparator()).Compare()). Will follow that pattern.
Summary: In index blocks since `format_version=3`, user keys are written rather than internal keys. When reading such blocks, the comparator is obtained via `InternalKeyComparator::user_comparator()`. That function must not return an unwrapped result as the wrapper class provides accounting logic to populate `PerfContext::user_key_comparison_count`. Test Plan: ran db_bench and verified `PerfContext::user_key_comparison_count` became larger.
ajkr
force-pushed
the
fix-comparison-accounting
branch
from
065fd7c
to
6e6fa71
Compare
|
@ajkr has updated the pull request. Re-import the pull request |
|
@siying I've addressed your comment; this is ready for another look. By the way, I am thinking we should get rid of the An alternative is just use macros. There would be a |
There was a problem hiding this comment.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: In index blocks since
format_version=3, user keys are writtenrather than internal keys. When reading such blocks, the comparator is
obtained via
InternalKeyComparator::user_comparator(). We need towrap that function's result in a
UserComparatorWrapperin order forits comparisons to be counted in
PerfContext::user_key_comparison_count.Test Plan: ran db_bench and verified
PerfContext::user_key_comparison_countbecame larger.