Make InternalKeyComparator not configurable #10342

siying · 2022-07-11T23:38:38Z

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

facebook-github-bot · 2022-07-11T23:38:53Z

@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

pdillinger · 2022-07-12T05:49:42Z

HISTORY.md

@@ -23,6 +23,7 @@
 * Fix a bug in which backup/checkpoint can include a WAL deleted by RocksDB.
 * Fix a bug where concurrent compactions might cause unnecessary further write stalling. In some cases, this might cause write rate to drop to minimum.
 * Fix a bug in Logger where if dbname and db_log_dir are on different filesystems, dbname creation would fail wrt to db_log_dir path returning an error and fails to open the DB.
+* Fix the regression introduce by https://github.com/facebook/rocksdb/pull/8336 which made InternalKeyComparator configurable as an unintended side effect.


CPU efficiency (kind of regression)

pdillinger · 2022-07-12T05:55:26Z

include/rocksdb/comparator.h

@@ -17,6 +17,20 @@ namespace ROCKSDB_NAMESPACE {

 class Slice;

+class CompareInterface {


There should be a comment for why this exists, such as "used internally"

pdillinger · 2022-07-12T05:59:22Z

table/merging_iterator.cc

@@ -386,7 +386,7 @@ void MergingIterator::SwitchToForward() {
      if (child.status() == Status::TryAgain()) {
        continue;
      }
-      if (child.Valid() && comparator_->Equal(target, child.key())) {
+      if (child.Valid() && comparator_->Compare(target, child.key()) == 0) {


I could see this being a performance regression on large keys where there's a good chance a.size() != b.size().

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.

mrambacher · 2022-07-12T12:40:32Z

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)?

siying · 2022-07-12T17:37:52Z

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.

facebook-github-bot · 2022-07-13T04:53:18Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-13T04:54:14Z

@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mrambacher · 2022-07-13T09:49:13Z

db/dbformat.h

+  void FindShortestSeparator(std::string* start, const Slice& limit) const;
+  void FindShortSuccessor(std::string* key) const;


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?

These functions are still used index_builder.h. Let me see whether I can move it to clear InternalKeyComparator.

Moved the functions to IndexBuilder.

mrambacher · 2022-07-13T09:50:13Z

db/dbformat.h

@@ -273,7 +277,7 @@ class InternalKeyComparator
  // value `kDisableGlobalSequenceNumber`.
  int Compare(const Slice& a, SequenceNumber a_global_seqno, const Slice& b,
              SequenceNumber b_global_seqno) const;
-  virtual const Comparator* GetRootComparator() const override {
+  virtual const Comparator* GetRootComparator() const {


Is this method still needed? If so, can it be renamed to prevent confusion with the method in Comparator?

Let me see whether I can delete the function.

Removed it.

facebook-github-bot · 2022-07-13T23:13:44Z

@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

facebook-github-bot · 2022-07-13T23:13:55Z

@siying has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot · 2022-07-13T23:14:11Z

@siying has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

mrambacher

Thanks for the changes

mrambacher · 2022-07-14T00:46:47Z

db/dbformat_test.cc

@@ -9,6 +9,7 @@

 #include "db/dbformat.h"

+#include "table/block_based/index_builder.h"


Nit: This should probably include compaator.h as well.

siying requested a review from pdillinger July 11, 2022 23:38

facebook-github-bot added the CLA Signed label Jul 11, 2022

pdillinger reviewed Jul 12, 2022

View reviewed changes

XinyuZeng mentioned this pull request Jul 13, 2022

Reduce comparator objects init cost in BlockIter #9611

Closed

siying force-pushed the internal_key_name branch from 18017b9 to 189c3bb Compare July 13, 2022 04:53

mrambacher reviewed Jul 13, 2022

View reviewed changes

siying added 3 commits July 13, 2022 16:13

Address comments

85d033d

Remove some functions in InternalKeyComparator

7342554

siying force-pushed the internal_key_name branch from 1c30c38 to 7342554 Compare July 13, 2022 23:13

mrambacher approved these changes Jul 14, 2022

View reviewed changes

facebook-github-bot closed this in c8b20d4 Jul 14, 2022

pdillinger mentioned this pull request Oct 20, 2022

Make UserComparatorWrapper not Customizable #10837

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make InternalKeyComparator not configurable #10342

Make InternalKeyComparator not configurable #10342

siying commented Jul 11, 2022

facebook-github-bot commented Jul 11, 2022

pdillinger Jul 12, 2022

pdillinger Jul 12, 2022

pdillinger Jul 12, 2022

siying Jul 12, 2022

mrambacher commented Jul 12, 2022

siying commented Jul 12, 2022

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

mrambacher Jul 13, 2022

siying Jul 13, 2022

siying Jul 13, 2022

mrambacher Jul 13, 2022

siying Jul 13, 2022

siying Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

mrambacher left a comment

mrambacher Jul 14, 2022

		@@ -17,6 +17,20 @@ namespace ROCKSDB_NAMESPACE {

		class Slice;

		class CompareInterface {

		void FindShortestSeparator(std::string* start, const Slice& limit) const;
		void FindShortSuccessor(std::string* key) const;

		@@ -9,6 +9,7 @@

		#include "db/dbformat.h"

		#include "table/block_based/index_builder.h"

Make InternalKeyComparator not configurable #10342

Make InternalKeyComparator not configurable #10342

Conversation

siying commented Jul 11, 2022

facebook-github-bot commented Jul 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrambacher commented Jul 12, 2022

siying commented Jul 12, 2022

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

facebook-github-bot commented Jul 13, 2022

mrambacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment