Make InternalKeyComparator not configurable#10342
Make InternalKeyComparator not configurable#10342siying wants to merge 3 commits intofacebook:mainfrom
Conversation
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
HISTORY.md
Outdated
There was a problem hiding this comment.
CPU efficiency (kind of regression)
include/rocksdb/comparator.h
Outdated
There was a problem hiding this comment.
There should be a comment for why this exists, such as "used internally"
table/merging_iterator.cc
Outdated
There was a problem hiding this comment.
I could see this being a performance regression on large keys where there's a good chance a.size() != b.size().
There was a problem hiding this comment.
Potentially yes but now InternalKeyComparator just uses a default implementation of Equal() which does exactly what is done here. Improving efficiency of InternalKeyComparator::Equal() sounds outside the scope of the PR and I don't see a need to mix the two.
|
While I agree that it is less-than-ideal to have an unused vector here taking up space, I am wondering if this is the best solution or what problem is being solved here. What is the performance difference with this change? Many of the methods that are part of the Comparator have not been moved to the ComparatorInterface. Is there a reason these were skipped? What would happen if instead the std::vector was changed into a std::unique_ptrstd::vector, and constructors (and guards) were added that allowed for this vector to be null? Would the equivalent performance/size gain be accomplished for all classes/implementations that chose to use that new constructor (and not have any options)? |
We know that BlockIter is quite sensitive to memory size. The reason is that it would usually use memory allocated by ArenaWrappedDBIter. We experienced performance regression at least twice for object size increase in BlockIter. A whole InternalKeyComparator will likely to be put in BlockIter after we fix #9611 for #10340 . Even 8 byte should ideally be removed. Fundamentally, I don't see a point of making a non-configurable class configurable. It is simply confusing and is hard to maintain. Regarding not putting more functions in CompareInterface. Ideally, Comparator and InternalKeyComparator don't need to share any function. They operate on different things and don't need to share. I am trying to be conscious on what they share and minimize it. It would also make it easy to get ride of CompareInterface in the future. |
18017b9 to
189c3bb
Compare
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/dbformat.h
Outdated
There was a problem hiding this comment.
Are these methods still needed/used? If so, can they be renamed so that it is clear they are not the same/related to the ones in Comparator?
There was a problem hiding this comment.
These functions are still used index_builder.h. Let me see whether I can move it to clear InternalKeyComparator.
There was a problem hiding this comment.
Moved the functions to IndexBuilder.
db/dbformat.h
Outdated
There was a problem hiding this comment.
Is this method still needed? If so, can it be renamed to prevent confusion with the method in Comparator?
There was a problem hiding this comment.
Let me see whether I can delete the function.
|
@siying has updated the pull request. You must reimport the pull request before landing. |
Summary: InternalKeyComparator is an internal class which is a simple wrapper of Comparator. facebook#8336 made Comparator customizeable. As a side effect, internal key comparator was made configurable too. This introduces overhead to this simple wrapper. For example, every InternalKeyComparator will have an std::vector attached to it, which consumes memory and possible allocation overhead too. We remove InternalKeyComparator from being customizable by making InternalKeyComparator not a subclass of Comparator. Test Plan: Run existing CI tests and make sure it doesn't fail
1c30c38 to
7342554
Compare
|
@siying has updated the pull request. You must reimport the pull request before landing. |
|
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
mrambacher
left a comment
There was a problem hiding this comment.
Thanks for the changes
|
|
||
| #include "db/dbformat.h" | ||
|
|
||
| #include "table/block_based/index_builder.h" |
There was a problem hiding this comment.
Nit: This should probably include compaator.h as well.
Summary:
InternalKeyComparator is an internal class which is a simple wrapper of Comparator. #8336 made Comparator customizeable. As a side effect, internal key comparator was made configurable too. This introduces overhead to this simple wrapper. For example, every InternalKeyComparator will have an std::vector attached to it, which consumes memory and possible allocation overhead too.
We remove InternalKeyComparator from being customizable by making InternalKeyComparator not a subclass of Comparator.
Test Plan: Run existing CI tests and make sure it doesn't fail