-
Notifications
You must be signed in to change notification settings - Fork 6.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make UserComparatorWrapper not Customizable #10837
Conversation
@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
5952cee
to
ed1fecb
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. |
include/rocksdb/comparator.h
Outdated
// The general interface for comparing two Slices that are related to | ||
// timestamps. | ||
// They are defined for both Comparator and some internal data structures. | ||
class CompareWithTimestampInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would we ever have a Compare that was a CompareWithTimestamp that is not also a CompareInterface? If not, is there a reason why to not have CompareWithTimestamp extend CompareInterface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CompareInterface is used for Internal comparators, and internal operators don't seem to do those timestamp comparisons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing the two adds to confusion and bugs. AFAIK the only reason they are mixed in Comparator is API compatibility. CompareInterface has internal applications where only one kind of compare is available.
include/rocksdb/comparator.h
Outdated
class CompareWithTimestampInterface { | ||
public: | ||
virtual ~CompareWithTimestampInterface() {} | ||
int CompareWithoutTimestamp(const Slice& a, const Slice& b) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this function be virtual?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not virtual in the first place. I don't think we should create virtual functions when nobody use them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's apparently a convenience function (and should be documented accordingly)
include/rocksdb/comparator.h
Outdated
virtual int CompareTimestamp(const Slice& /*ts1*/, | ||
const Slice& /*ts2*/) const = 0; | ||
|
||
virtual int CompareWithoutTimestamp(const Slice& a, bool /*a_has_ts*/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a function comment
include/rocksdb/comparator.h
Outdated
const Slice& b, | ||
bool /*b_has_ts*/) const = 0; | ||
|
||
virtual bool EqualWithoutTimestamp(const Slice& a, const Slice& b) const = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a function comment
include/rocksdb/comparator.h
Outdated
virtual int CompareTimestamp(const Slice& /*ts1*/, | ||
const Slice& /*ts2*/) const { | ||
const Slice& /*ts2*/) const override { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just override, not virtual+override.
This same comment applies to many of the functions below.
include/rocksdb/comparator.h
Outdated
// The general interface for comparing two Slices that are related to | ||
// timestamps. | ||
// They are defined for both Comparator and some internal data structures. | ||
class CompareWithTimestampInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timestamp_size should be moved into this class (and the Comparator constructor) should call the appropriate related constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface may be used a wrapper. If we move timestamp_size there, there will be potential consistency issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the argument for keeping this an "interface" with no data. Multiple inheritance with data fields should be minimized.
explicit UserComparatorWrapper(const Comparator* const user_cmp) | ||
: Comparator(user_cmp->timestamp_size()), user_comparator_(user_cmp) {} | ||
: user_comparator_(user_cmp) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason that user_comparator_ should not be the interface rather than the Comparator? Comparably why this constructor does not take the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users sometimes call other functions of the inner comparator, and we will need to support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me. Wrapper shares a common interface for essential functions while also providing lower level access to those who know they're using the wrapper.
Though I don't see anywhere where the wrapper is strictly used as a CompareWithTimestampInterface. Does it really need virtual functions and a virtual destructor if we are counting on its uses being de-virtualized anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good point. Let me see whether I can just make them non-virtual and avoid this new interface.
I figured out why this seems so familiar. This is basically part 2 of #10342 . |
Yes, it is very similar. |
db/db_iter.cc
Outdated
@@ -522,7 +523,8 @@ bool DBIter::MergeValuesNewToOld() { | |||
return false; | |||
} | |||
|
|||
if (!user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { | |||
if (!user_comparator_.user_comparator()->Equal(ikey.user_key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bypassing the perf counter here?
db/db_iter.cc
Outdated
@@ -1158,7 +1160,8 @@ bool DBIter::FindValueForCurrentKeyUsingSeek() { | |||
if (!ParseKey(&ikey)) { | |||
return false; | |||
} | |||
if (!user_comparator_.Equal(ikey.user_key, saved_key_.GetUserKey())) { | |||
if (!user_comparator_.user_comparator()->Equal(ikey.user_key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why bypassing the perf counter here?
include/rocksdb/comparator.h
Outdated
// The general interface for comparing two Slices that are related to | ||
// timestamps. | ||
// They are defined for both Comparator and some internal data structures. | ||
class CompareWithTimestampInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mixing the two adds to confusion and bugs. AFAIK the only reason they are mixed in Comparator is API compatibility. CompareInterface has internal applications where only one kind of compare is available.
include/rocksdb/comparator.h
Outdated
// The general interface for comparing two Slices that are related to | ||
// timestamps. | ||
// They are defined for both Comparator and some internal data structures. | ||
class CompareWithTimestampInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the argument for keeping this an "interface" with no data. Multiple inheritance with data fields should be minimized.
include/rocksdb/comparator.h
Outdated
class CompareWithTimestampInterface { | ||
public: | ||
virtual ~CompareWithTimestampInterface() {} | ||
int CompareWithoutTimestamp(const Slice& a, const Slice& b) const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's apparently a convenience function (and should be documented accordingly)
explicit UserComparatorWrapper(const Comparator* const user_cmp) | ||
: Comparator(user_cmp->timestamp_size()), user_comparator_(user_cmp) {} | ||
: user_comparator_(user_cmp) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me. Wrapper shares a common interface for essential functions while also providing lower level access to those who know they're using the wrapper.
Though I don't see anywhere where the wrapper is strictly used as a CompareWithTimestampInterface. Does it really need virtual functions and a virtual destructor if we are counting on its uses being de-virtualized anyway?
Summary: Right now UserComparatorWrapper is a Customizable object, although it is not, which introduces some intialization overhead for the object. In some benchmarks, it shows up in CPU profiling. Make it not configurable by defining most functions needed by UserComparatorWrapper to an interface and implement the interface. Test Plan: Make sure existing tests pass
ed1fecb
to
1166c98
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdillinger @mrambacher I remove the new interface and hopefully it looks better to you now.
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Summary:
Right now UserComparatorWrapper is a Customizable object, although it is not, which introduces some intialization overhead for the object. In some benchmarks, it shows up in CPU profiling. Make it not configurable by defining most functions needed by UserComparatorWrapper to an interface and implement the interface.
Test Plan: Make sure existing tests pass