Implement previousValue on EliasFanoReader

Summary: It is often useful to retrieve the value preceding the current value
in an Elias-Fano iterator, for example when the list represents
adjacent ranges. This diff implements a new method `previousValue` in
`EliasFanoReader`.

It also adds a new `kUnchecked` boolean template argument to
`EliasFanoReader` which skips bounds checking.

Reviewed By: @philippv

Differential Revision: D2155049

folly/experimental/EliasFanoCoding.h

@@ -24,26 +24,26 @@
 #ifndef FOLLY_EXPERIMENTAL_ELIAS_FANO_CODING_H
 #define FOLLY_EXPERIMENTAL_ELIAS_FANO_CODING_H
 
-#ifndef __GNUC__
-#error EliasFanoCoding.h requires GCC
-#endif
-
-#if !FOLLY_X64
-#error EliasFanoCoding.h requires x86_64
-#endif
-
 #include <cstdlib>
 #include <limits>
 #include <type_traits>
-#include <boost/noncopyable.hpp>
 #include <glog/logging.h>
 
 #include <folly/Bits.h>
 #include <folly/CpuId.h>
 #include <folly/Likely.h>
+#include <folly/Portability.h>
 #include <folly/Range.h>
 #include <folly/experimental/Select64.h>
 
+#ifndef __GNUC__
+#error EliasFanoCoding.h requires GCC
+#endif
+
+#if !FOLLY_X64
+#error EliasFanoCoding.h requires x86_64
+#endif
+
 #if __BYTE_ORDER__ != __ORDER_LITTLE_ENDIAN__
 #error EliasFanoCoding.h requires little endianness
 #endif
@@ -118,9 +118,9 @@ struct EliasFanoEncoderV2 {
 
   // Requires: input range (begin, end) is sorted (encoding
   // crashes if it's not).
-  // WARNING: encode() mallocates lower, upper, skipPointers
-  // and forwardPointers. As EliasFanoCompressedList has
-  // no ownership of them, you need to call free() explicitly.
+  // WARNING: encode() mallocates EliasFanoCompressedList::data. As
+  // EliasFanoCompressedList has no ownership of it, you need to call
+  // free() explicitly.
   template <class RandomAccessIterator>
   static EliasFanoCompressedList encode(RandomAccessIterator begin,
                                         RandomAccessIterator end) {
@@ -142,6 +142,7 @@ struct EliasFanoEncoderV2 {
         forwardPointers_(reinterpret_cast<SkipValueType*>(
                            result.forwardPointers)),
         result_(result) {
+    memset(result.data.data(), 0, result.data.size());
   }
 
   EliasFanoEncoderV2(size_t size, ValueType upperBound)
@@ -308,11 +309,11 @@ struct EliasFanoEncoderV2<Value,
     uint8_t* buf = nullptr;
     // WARNING: Current read/write logic assumes that the 7 bytes
     // following the last byte of lower and upper sequences are
-    // readable (stored value doesn't matter and won't be changed),
-    // so we allocate additional 7B, but do not include them in size
+    // readable (stored value doesn't matter and won't be changed), so
+    // we allocate additional 7 bytes, but do not include them in size
     // of returned value.
     if (size > 0) {
-      buf = static_cast<uint8_t*>(calloc(bytes() + 7, 1));
+      buf = static_cast<uint8_t*>(malloc(bytes() + 7));
     }
     folly::MutableByteRange bufRange(buf, bytes());
     return openList(bufRange);
@@ -348,6 +349,10 @@ struct Default {
     DCHECK_GT(value, 0);
     return __builtin_ctzll(value);
   }
+  static inline int clz(uint64_t value) {
+    DCHECK_GT(value, 0);
+    return __builtin_clzll(value);
+  }
   static inline uint64_t blsr(uint64_t value) {
     return value & (value - 1);
   }
@@ -522,6 +527,24 @@ class UpperBitsReader {
     return skipToNext(v);
   }
 
+  ValueType previousValue() const {
+    DCHECK_NE(position(), -1);
+    DCHECK_GT(position(), 0);
+
+    size_t outer = outer_;
+    block_t block = folly::loadUnaligned<block_t>(start_ + outer);
+    block &= (block_t(1) << inner_) - 1;
+
+    while (UNLIKELY(block == 0)) {
+      DCHECK_GE(outer, sizeof(block_t));
+      outer -= sizeof(block_t);
+      block = folly::loadUnaligned<block_t>(start_ + outer);
+    }
+
+    auto inner = 8 * sizeof(block_t) - 1 - Instructions::clz(block);
+    return static_cast<ValueType>(8 * outer + inner - (position_ - 1));
+  }
+
   void setDone(size_t endPos) {
     position_ = endPos;
   }
@@ -538,7 +561,7 @@ class UpperBitsReader {
     block_ &= ~((block_t(1) << (dest % 8)) - 1);
   }
 
-  typedef unsigned long long block_t;
+  typedef uint64_t block_t;
   const unsigned char* const forwardPointers_;
   const unsigned char* const skipPointers_;
   const unsigned char* const start_;
@@ -551,9 +574,13 @@ class UpperBitsReader {
 
 }  // namespace detail
 
+// If kUnchecked = true the caller must guarantee that all the
+// operations return valid elements, i.e., they would never return
+// false if checked.
 template <class Encoder,
-          class Instructions = instructions::Default>
-class EliasFanoReader : private boost::noncopyable {
+          class Instructions = instructions::Default,
+          bool kUnchecked = false>
+class EliasFanoReader {
  public:
   typedef Encoder EncoderType;
   typedef typename Encoder::ValueType ValueType;
@@ -567,7 +594,9 @@ class EliasFanoReader : private boost::noncopyable {
     DCHECK(Instructions::supported());
     // To avoid extra branching during skipTo() while reading
     // upper sequence we need to know the last element.
-    if (UNLIKELY(list.size == 0)) {
+    // If kUnchecked == true, we do not check that skipTo() is called
+    // within the bounds, so we can avoid initializing lastValue_.
+    if (kUnchecked || UNLIKELY(list.size == 0)) {
       lastValue_ = 0;
       return;
     }
@@ -584,7 +613,7 @@ class EliasFanoReader : private boost::noncopyable {
   }
 
   bool next() {
-    if (UNLIKELY(position() + 1 >= size_)) {
+    if (!kUnchecked && UNLIKELY(position() + 1 >= size_)) {
       return setDone();
     }
     upper_.next();
@@ -596,7 +625,7 @@ class EliasFanoReader : private boost::noncopyable {
   bool skip(size_t n) {
     CHECK_GT(n, 0);
 
-    if (LIKELY(position() + n < size_)) {
+    if (kUnchecked || LIKELY(position() + n < size_)) {
       if (LIKELY(n < kLinearScanThreshold)) {
         for (size_t i = 0; i < n; ++i) upper_.next();
       } else {
@@ -614,7 +643,7 @@ class EliasFanoReader : private boost::noncopyable {
     DCHECK_GE(value, value_);
     if (value <= value_) {
       return true;
-    } else if (value > lastValue_) {
+    } else if (!kUnchecked && value > lastValue_) {
       return setDone();
     }
 
@@ -649,7 +678,7 @@ class EliasFanoReader : private boost::noncopyable {
     if (value <= 0) {
       reset();
       return true;
-    } else if (value > lastValue_) {
+    } else if (!kUnchecked && value > lastValue_) {
       return setDone();
     }
 
@@ -658,6 +687,13 @@ class EliasFanoReader : private boost::noncopyable {
     return true;
   }
 
+  ValueType previousValue() const {
+    DCHECK_GT(position(), 0);
+    DCHECK_LT(position(), size());
+    return readLowerPart(upper_.position() - 1) |
+      (upper_.previousValue() << numLowerBits_);
+  }
+
   size_t size() const { return size_; }
 
   size_t position() const { return upper_.position(); }

folly/experimental/test/CodingTestUtils.h

@@ -73,6 +73,21 @@ inline std::vector<uint32_t> loadList(const std::string& filename) {
   return result;
 }
 
+// Test previousValue only if Reader has it.
+template <class... Args>
+void maybeTestPreviousValue(Args&&...) { }
+
+// Make all the arguments template because if the types are not exact,
+// the above overload will be picked (for example i could be size_t or
+// ssize_t).
+template <class Vector, class Reader, class Index>
+auto maybeTestPreviousValue(const Vector& data, Reader& reader, Index i)
+  -> decltype(reader.previousValue(), void()) {
+  if (i != 0) {
+    EXPECT_EQ(reader.previousValue(), data[i - 1]);
+  }
+}
+
 template <class Reader, class List>
 void testNext(const std::vector<uint32_t>& data, const List& list) {
   Reader reader(list);
@@ -81,6 +96,7 @@ void testNext(const std::vector<uint32_t>& data, const List& list) {
     EXPECT_TRUE(reader.next());
     EXPECT_EQ(reader.value(), data[i]);
     EXPECT_EQ(reader.position(), i);
+    maybeTestPreviousValue(data, reader, i);
   }
   EXPECT_FALSE(reader.next());
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
@@ -97,6 +113,7 @@ void testSkip(const std::vector<uint32_t>& data, const List& list,
     EXPECT_TRUE(reader.skip(skipStep));
     EXPECT_EQ(reader.value(), data[i]);
     EXPECT_EQ(reader.position(), i);
+    maybeTestPreviousValue(data, reader, i);
   }
   EXPECT_FALSE(reader.skip(skipStep));
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
@@ -134,6 +151,7 @@ void testSkipTo(const std::vector<uint32_t>& data, const List& list,
     EXPECT_TRUE(reader.skipTo(value));
     EXPECT_EQ(reader.value(), *it);
     value = reader.value() + delta;
+    maybeTestPreviousValue(data, reader, std::distance(data.begin(), it));
   }
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());
   EXPECT_EQ(reader.position(), reader.size());
@@ -177,6 +195,7 @@ void testJump(const std::vector<uint32_t>& data, const List& list) {
     EXPECT_TRUE(reader.jump(i + 1));
     EXPECT_EQ(reader.value(), data[i]);
     EXPECT_EQ(reader.position(), i);
+    maybeTestPreviousValue(data, reader, i);
   }
   EXPECT_FALSE(reader.jump(data.size() + 1));
   EXPECT_EQ(reader.value(), std::numeric_limits<uint32_t>::max());