Make Comparator into a Customizable Object #8336
Conversation
|
|
||
| Status Comparator::CreateFromString(const ConfigOptions& config_options, | ||
| const std::string& value, | ||
| const Comparator** result) { |
There was a problem hiding this comment.
It's counter intuitive to have the return result as const. Is because of the option is const?
rocksdb/include/rocksdb/options.h
Line 113 in 2f1984d
There was a problem hiding this comment.
Yes, it is defined here as const because the option is const. This actually makes everything else slightly harder to do. If I make them non-const, it also will break (at least UBSAN) compatibility with any code that currently uses the ObjectRegistry to register their comparators.
| @@ -20,7 +21,7 @@ class Slice; | |||
| // used as keys in an sstable or a database. A Comparator implementation | |||
| // must be thread-safe since rocksdb may invoke its methods concurrently | |||
| // from multiple threads. | |||
| class Comparator { | |||
| class Comparator : public Customizable { | |||
There was a problem hiding this comment.
As this is public interface, is there potential ABI breakage?
There was a problem hiding this comment.
I am not sure what the ABI breakage would be or how to test for it. Ideas?
There was a problem hiding this comment.
I think it's fine to break binary compatibility, especially for non patch release. I found a website which tracks our API binary compatibility: https://abi-laboratory.pro/?view=timeline&l=rocksdb
| @@ -20,7 +21,7 @@ class Slice; | |||
| // used as keys in an sstable or a database. A Comparator implementation | |||
| // must be thread-safe since rocksdb may invoke its methods concurrently | |||
| // from multiple threads. | |||
| class Comparator { | |||
| class Comparator : public Customizable { | |||
There was a problem hiding this comment.
Customizable increases the comparator size, what would be the performance impact?
There was a problem hiding this comment.
Here is a comparison of master and this branch for some operations from benchmark.sh (same performance tests as the performance Wiki):
Master:
readrandom : 461.911 micros/op 138552 ops/sec; 35.1 MB/s (7364222 of 11651999 found)
multireadrandom : 461.621 micros/op 138639 ops/sec; (7369984 of 11654990 found)
overwrite : 694.754 micros/op 92117 ops/sec; 36.9 MB/s
readwhilewriting : 659.770 micros/op 97001 ops/sec; 30.8 MB/s (6496957 of 8195999 found)
Change:
readrandom : 461.858 micros/op 138568 ops/sec; 35.1 MB/s (7365481 of 11649999 found)
multireadrandom : 461.689 micros/op 138619 ops/sec; (7366962 of 11652990 found)
overwrite : 698.994 micros/op 91558 ops/sec; 36.7 MB/s
readwhilewriting : 661.834 micros/op 96698 ops/sec; 30.7 MB/s (6455689 of 8151999 found)
This change does not appear to have a noticeable performance impact.
include/rocksdb/comparator.h
Outdated
| static const char* kBytewiseClassName() { | ||
| return "leveldb.BytewiseComparator"; | ||
| } | ||
| static const char* kReverseBytewiseClassName() { | ||
| return "rocksdb.ReverseBytewiseComparator"; | ||
| } |
There was a problem hiding this comment.
Should not these be defined in derived class?
There was a problem hiding this comment.
They are defined here so they can be used publicly. If they are in the derived class, then these constants cannot be used as strings/inputs to CreateFromString. Generally speaking, I have thought the names of the builtin Customizable objects should be public, but I can go either way.
There was a problem hiding this comment.
It looks odd to me that the derived class name is defined in base class. How about other compactors?
There was a problem hiding this comment.
So, the sampling is small (so far) as there are not that many Customizable classes.
For TableFactory, the derived names (Block, Plain, Cuckoo) are in the base class. This is necessary because there is often casting that goes on based on the type and the derived classes are not in the public API.
For Env, I could see the potential of needing the "default" or "posix" names in the base class. I am not certain that other names will have to be there. Default might be necessary so that one could cast an Env to the default or base implementation.
If the name is not in the base class, then constructing a derived class can be more problematic. The options are:
- Create a unique constructor for every type (e.g ByteWiseComparator(), ReverseByteWiseComparator(), etc). This is obviously not sustainable for all permutations
- Document what the names are to create each type (e.g. "Comparator has two built-in types. a "leveldb.BytewiseComparator" and a "rocksdb.ReverseBytewiseComparator". These names can be passed to CreateFromString to create ..."). This is changing the problem from having a constant in the public API to having the behavior in documentation.
- Make the implementation classes public. Yuck.
I am not opposed to making it a "documentation" problem. I have also thought about ways of creating a tool that would allow a user to see what "instances" are available (via the ObjectRegistry) to discover what these names are and possibly make the features self-documenting.
Since the names (for Comparators) are not really necessary in the public API at the moment, I can move them to the implementation classes if that is better.
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
|
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@mrambacher merged this pull request in 6ad0810. |
This reverts commit 6ad0810.
This reverts commit 6ad0810.
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
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. Pull Request resolved: #10342 Test Plan: Run existing CI tests and make sure it doesn't fail Reviewed By: riversand963 Differential Revision: D37771351 fbshipit-source-id: 917256ee04b2796ed82974549c734fb6c4d8ccee
Makes the Comparator class into a Customizable object. Added/Updated the CreateFromString method to create Comparators. Added test for using the ObjectRegistry to create one.