From 7c14abf2c7614a57619780a74876a506a3c3c227 Mon Sep 17 00:00:00 2001
From: Islam AbdelRahman <tec@fb.com>
Date: Mon, 25 Apr 2016 23:02:14 -0700
Subject: [PATCH] Improve BytewiseComparatorImpl::FindShortestSeparator

Summary:
The current implementation find the first different byte and try to increment it, if it cannot it return the original key
we can improve this by keep going after the first different byte to find the first non 0xFF byte and increment it

After trying this patch on some logdevice sst files I see decrease in there index block size by 8.5%

Test Plan: existing tests and updated test

Reviewers: yhchiang, andrewkr, sdong

Reviewed By: sdong

Subscribers: andrewkr, dhruba

Differential Revision: https://reviews.facebook.net/D56241
---
 db/db_properties_test.cc |  2 +-
 db/dbformat_test.cc      | 24 ++++++++++++++++++++++++
 util/comparator.cc       | 36 ++++++++++++++++++++++++++++++++----
 3 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/db/db_properties_test.cc b/db/db_properties_test.cc
index ad1afc661..bae1963d4 100644
--- a/db/db_properties_test.cc
+++ b/db/db_properties_test.cc
@@ -224,7 +224,7 @@ void GetExpectedTableProperties(TableProperties* expected_tp,
                                 const int kBloomBitsPerKey,
                                 const size_t kBlockSize) {
   const int kKeyCount = kTableCount * kKeysPerTable;
-  const int kAvgSuccessorSize = kKeySize / 2;
+  const int kAvgSuccessorSize = kKeySize / 5;
   const int kEncodingSavePerKey = kKeySize / 4;
   expected_tp->raw_key_size = kKeyCount * (kKeySize + 8);
   expected_tp->raw_value_size = kKeyCount * kValueSize;
diff --git a/db/dbformat_test.cc b/db/dbformat_test.cc
index e79dbc683..b431690cc 100644
--- a/db/dbformat_test.cc
+++ b/db/dbformat_test.cc
@@ -92,6 +92,30 @@ TEST_F(FormatTest, InternalKeyShortSeparator) {
             Shorten(IKey("foo", 100, kTypeValue),
                     IKey("hello", 200, kTypeValue)));
 
+  ASSERT_EQ(IKey("ABC2", kMaxSequenceNumber, kValueTypeForSeek),
+            Shorten(IKey("ABC1AAAAA", 100, kTypeValue),
+                    IKey("ABC2ABB", 200, kTypeValue)));
+
+  ASSERT_EQ(IKey("AAA2", kMaxSequenceNumber, kValueTypeForSeek),
+            Shorten(IKey("AAA1AAA", 100, kTypeValue),
+                    IKey("AAA2AA", 200, kTypeValue)));
+
+  ASSERT_EQ(
+      IKey("AAA2", kMaxSequenceNumber, kValueTypeForSeek),
+      Shorten(IKey("AAA1AAA", 100, kTypeValue), IKey("AAA4", 200, kTypeValue)));
+
+  ASSERT_EQ(
+      IKey("AAA1B", kMaxSequenceNumber, kValueTypeForSeek),
+      Shorten(IKey("AAA1AAA", 100, kTypeValue), IKey("AAA2", 200, kTypeValue)));
+
+  ASSERT_EQ(IKey("AAA2", kMaxSequenceNumber, kValueTypeForSeek),
+            Shorten(IKey("AAA1AAA", 100, kTypeValue),
+                    IKey("AAA2A", 200, kTypeValue)));
+
+  ASSERT_EQ(
+      IKey("AAA1", 100, kTypeValue),
+      Shorten(IKey("AAA1", 100, kTypeValue), IKey("AAA2", 200, kTypeValue)));
+
   // When start user key is prefix of limit user key
   ASSERT_EQ(IKey("foo", 100, kTypeValue),
             Shorten(IKey("foo", 100, kTypeValue),
diff --git a/util/comparator.cc b/util/comparator.cc
index cb802d55b..4a5d3641a 100644
--- a/util/comparator.cc
+++ b/util/comparator.cc
@@ -49,13 +49,41 @@ class BytewiseComparatorImpl : public Comparator {
     if (diff_index >= min_length) {
       // Do not shorten if one string is a prefix of the other
     } else {
-      uint8_t diff_byte = static_cast<uint8_t>((*start)[diff_index]);
-      if (diff_byte < static_cast<uint8_t>(0xff) &&
-          diff_byte + 1 < static_cast<uint8_t>(limit[diff_index])) {
+      uint8_t start_byte = static_cast<uint8_t>((*start)[diff_index]);
+      uint8_t limit_byte = static_cast<uint8_t>(limit[diff_index]);
+      if (start_byte >= limit_byte || (diff_index == start->size() - 1)) {
+        // Cannot shorten since limit is smaller than start or start is
+        // already the shortest possible.
+        return;
+      }
+      assert(start_byte < limit_byte);
+
+      if (diff_index < limit.size() - 1 || start_byte + 1 < limit_byte) {
         (*start)[diff_index]++;
         start->resize(diff_index + 1);
-        assert(Compare(*start, limit) < 0);
+      } else {
+        //     v
+        // A A 1 A A A
+        // A A 2
+        //
+        // Incrementing the current byte will make start bigger than limit, we
+        // will skip this byte, and find the first non 0xFF byte in start and
+        // increment it.
+        diff_index++;
+
+        while (diff_index < start->size()) {
+          // Keep moving until we find the first non 0xFF byte to
+          // increment it
+          if (static_cast<uint8_t>((*start)[diff_index]) <
+              static_cast<uint8_t>(0xff)) {
+            (*start)[diff_index]++;
+            start->resize(diff_index + 1);
+            break;
+          }
+          diff_index++;
+        }
       }
+      assert(Compare(*start, limit) < 0);
     }
   }