Reduce comparator objects init cost in BlockIter#9611
Reduce comparator objects init cost in BlockIter#9611XinyuZeng wants to merge 3 commits intofacebook:mainfrom
Conversation
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@XinyuZeng has updated the pull request. You must reimport the pull request before landing. |
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ajkr
left a comment
There was a problem hiding this comment.
LGTM. Updated the "Test Plan" to show results of microbenchmark
|
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
|
||
| UserComparatorWrapper ucmp() { return UserComparatorWrapper(raw_ucmp_); } | ||
| icmp_ = | ||
| std::make_unique<InternalKeyComparator>(raw_ucmp, false /* named */); |
There was a problem hiding this comment.
Can InternalKeyComparator wrap a UserComparatorWrapper?
There was a problem hiding this comment.
It seems like InternalKeyComparator already has a UserComparatorWrapper as a private member. Calling icmp_->user_comparator() refers to it.
Summary: I tried evaluating #9611 using DBGet microbenchmarks but mostly found the change is well within the noise even for hundreds of repetitions; meanwhile, the InternalKeyComparator CPU it saves is 1-2% according to perf so it should be measurable. In this PR I tried adding a mmap mode that will bypass compression/checksum/block cache/file read to focus more on the block lookup paths, and also increased the Get() count. Pull Request resolved: #9903 Reviewed By: jay-zhuang, riversand963 Differential Revision: D35907375 Pulled By: ajkr fbshipit-source-id: 69490d5040ef0863e1ce296724104d0aa7667215
|
hey @ajkr, do you mind share the compare.py script you used in the Test Plan? Thanks! |
Sure, it's a script provided in google benchmark -- https://github.com/google/benchmark/blob/main/tools/compare.py |
|
@XinyuZeng can you take a look at #10340 ? |
|
@riversand963 Through a quick look, I am not sure why #9611 introduces more allocations using glibc because it should reduce the number of allocations compared to the commit before it. Before the commit, each call to CompareCurrentKey will have an allocation. But I do agree with #10342:
This is also what I profiled and found when I did #9611 |
This PR solves the problem discussed in #7149. By storing the pointer of InternalKeyComparator as icmp_ in BlockIter, the object size remains the same. And for each call to CompareCurrentKey, there is no need to create Comparator objects. One can use icmp_ directly or use the "user_comparator" from the icmp_.
Test Plan: with #9903,