Revert D22211304: Eliminate need for reading upper bound at construction.

Differential Revision:
D22211304 (https://github.com/facebook/folly/commit/c9c5564232aaffbc0f1d3807b21bb2cd60ec4d2d)

Original commit changeset: 0dbe904c9fd8

fbshipit-source-id: 49cf24dab4f91fcebf9dc5a2db66d23683fcaa37

folly/experimental/EliasFanoCoding.h

@@ -24,11 +24,9 @@
 #pragma once
 
 #include <algorithm>
-#include <cstddef>
 #include <cstdlib>
 #include <limits>
 #include <type_traits>
-#include <utility>
 
 #include <folly/Likely.h>
 #include <folly/Portability.h>
@@ -356,24 +354,18 @@ struct EliasFanoEncoderV2<
 
 namespace detail {
 
-template <
-    class Encoder,
-    class Instructions,
-    class SizeType,
-    bool kUnchecked = false>
+template <class Encoder, class Instructions, class SizeType>
 class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
                         SkipPointers<Encoder::skipQuantum> {
-  using SkipValueType = typename Encoder::SkipValueType;
+  typedef typename Encoder::SkipValueType SkipValueType;
 
  public:
-  using ValueType = typename Encoder::ValueType;
+  typedef typename Encoder::ValueType ValueType;
 
   explicit UpperBitsReader(const typename Encoder::CompressedList& list)
       : ForwardPointers<Encoder::forwardQuantum>(list.forwardPointers),
         SkipPointers<Encoder::skipQuantum>(list.skipPointers),
-        start_(list.upper),
-        size_(list.size),
-        upperBound_(8 * list.upperSizeBytes - size_) {
+        start_(list.upper) {
     reset();
   }
 
@@ -388,23 +380,11 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
   FOLLY_ALWAYS_INLINE SizeType position() const {
     return position_;
   }
-
   FOLLY_ALWAYS_INLINE ValueType value() const {
     return value_;
   }
 
-  FOLLY_ALWAYS_INLINE bool valid() const {
-    return position() < size();
-  }
-
-  FOLLY_ALWAYS_INLINE SizeType size() const {
-    return size_;
-  }
-
-  FOLLY_ALWAYS_INLINE bool previous() {
-    if (!kUnchecked && UNLIKELY(position() == 0)) {
-      return false;
-    }
+  FOLLY_ALWAYS_INLINE ValueType previous() {
     size_t inner;
     block_t block;
     getPreviousInfo(block, inner, outer_);
@@ -414,12 +394,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
     return setValue(inner);
   }
 
-  // Returns `true` if the next position is within bounds.
-  FOLLY_ALWAYS_INLINE bool next() {
-    if (!kUnchecked && UNLIKELY(position() + 1 >= size())) {
-      return setDone();
-    }
-
+  FOLLY_ALWAYS_INLINE ValueType next() {
     // Skip to the first non-zero block.
     while (block_ == 0) {
       outer_ += sizeof(block_t);
@@ -429,14 +404,12 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
     ++position_;
     size_t inner = Instructions::ctz(block_);
     block_ = Instructions::blsr(block_);
+
     return setValue(inner);
   }
 
-  FOLLY_ALWAYS_INLINE bool skip(SizeType n) {
+  FOLLY_ALWAYS_INLINE ValueType skip(SizeType n) {
     DCHECK_GT(n, 0);
-    if (!kUnchecked && UNLIKELY(position_ + n >= size())) {
-      return setDone();
-    }
 
     position_ += n; // n 1-bits will be read.
 
@@ -462,18 +435,15 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
     DCHECK_GT(n, 0);
     size_t inner = select64<Instructions>(block_, n - 1);
     block_ &= (block_t(-1) << inner) << 1;
+
     return setValue(inner);
   }
 
   // Skip to the first element that is >= v and located *after* the current
   // one (so even if current value equals v, position will be increased by 1).
-  FOLLY_ALWAYS_INLINE bool skipToNext(ValueType v) {
+  FOLLY_ALWAYS_INLINE ValueType skipToNext(ValueType v) {
     DCHECK_GE(v, value_);
 
-    if (!kUnchecked && UNLIKELY(v > upperBound_)) {
-      return setDone();
-    }
-
     // Use skip pointer.
     if (Encoder::skipQuantum > 0 &&
         UNLIKELY(v >= value_ + Encoder::skipQuantum)) {
@@ -481,12 +451,6 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
       const size_t dest = folly::loadUnaligned<SkipValueType>(
           this->skipPointers_ + (steps - 1) * sizeof(SkipValueType));
 
-      DCHECK_LE(dest, size());
-      // Check if we've reached the end.
-      if (!kUnchecked && UNLIKELY(dest == size())) {
-        return setDone();
-      }
-
       reposition(dest + Encoder::skipQuantum * steps);
       position_ = dest - 1;
 
@@ -506,7 +470,6 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
       skip -= cnt;
       position_ += kBitsPerBlock - cnt;
       outer_ += sizeof(block_t);
-      DCHECK_LT(outer_, (static_cast<size_t>(upperBound_) + size()) / 8);
       block_ = folly::loadUnaligned<block_t>(start_ + outer_);
     }
 
@@ -516,24 +479,17 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
       block_ &= block_t(-1) << inner;
     }
 
-    DCHECK_LT(position() + 1, size() + 1);
-    return next();
+    next();
+    return value_;
   }
 
   /**
-   * Try to prepare to skip to value. This is a constant-time operation
-   * that will attempt to prefetch memory required for a subsequent
-   * skipTo(value) call if the value to skip to is within this list.
+   * Prepare to skip to `value`. This is a constant-time operation that will
+   * prefetch memory required for a `skipTo(value)` call.
    *
-   * Returns:
-   *   {true, position of the reader} if the skip is valid,
-   *   {false, size()} otherwise.
+   * @return position of reader
    */
-  FOLLY_ALWAYS_INLINE std::pair<bool, SizeType> prepareSkipTo(
-      ValueType v) const {
-    if (!kUnchecked && UNLIKELY(v > upperBound_)) {
-      return std::make_pair(false, size());
-    }
+  FOLLY_ALWAYS_INLINE SizeType prepareSkipTo(ValueType v) const {
     auto position = position_;
 
     if (Encoder::skipQuantum > 0 && v >= value_ + Encoder::skipQuantum) {
@@ -542,12 +498,6 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
       const size_t dest = folly::loadUnaligned<SkipValueType>(
           this->skipPointers_ + (steps - 1) * sizeof(SkipValueType));
 
-      DCHECK_LE(dest, size());
-      // Check if we're going out of bounds.
-      if (!kUnchecked && UNLIKELY(dest == size())) {
-        return std::make_pair(false, size());
-      }
-
       position = dest - 1;
       outer = (dest + Encoder::skipQuantum * steps) / 8;
 
@@ -562,7 +512,7 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
       __builtin_prefetch(addr + kCacheLineSize);
     }
 
-    return std::make_pair(true, position);
+    return position;
   }
 
   FOLLY_ALWAYS_INLINE ValueType previousValue() const {
@@ -584,15 +534,14 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
     return (start_[bitPos / 8] & (1 << (bitPos % 8))) == 0;
   }
 
-  FOLLY_ALWAYS_INLINE bool setDone() {
-    position_ = size_;
-    return false;
+  FOLLY_ALWAYS_INLINE void setDone(SizeType endPos) {
+    position_ = endPos;
   }
 
  private:
-  FOLLY_ALWAYS_INLINE bool setValue(size_t inner) {
+  FOLLY_ALWAYS_INLINE ValueType setValue(size_t inner) {
     value_ = static_cast<ValueType>(8 * outer_ + inner - position_);
-    return true;
+    return value_;
   }
 
   FOLLY_ALWAYS_INLINE void reposition(SizeType dest) {
@@ -624,8 +573,6 @@ class UpperBitsReader : ForwardPointers<Encoder::forwardQuantum>,
   }
 
   const unsigned char* const start_;
-  const SizeType size_; // Size of the list.
-  const ValueType upperBound_; // Upper bound of values in this list.
   block_t block_;
   SizeType position_; // Index of current value (= #reads - 1).
   OuterType outer_; // Outer offset: number of consumed bytes in upper.
@@ -651,8 +598,24 @@ class EliasFanoReader {
   typedef typename Encoder::ValueType ValueType;
 
   explicit EliasFanoReader(const typename Encoder::CompressedList& list)
-      : upper_(list), lower_(list.lower), numLowerBits_(list.numLowerBits) {
+      : upper_(list),
+        lower_(list.lower),
+        size_(list.size),
+        numLowerBits_(list.numLowerBits) {
     DCHECK(Instructions::supported());
+    // To avoid extra branching during skipTo() while reading
+    // upper sequence we need to know the last element.
+    // 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;
+    }
+    ValueType lastUpperValue = ValueType(8 * list.upperSizeBytes - size_);
+    auto it = list.upper + list.upperSizeBytes - 1;
+    DCHECK_NE(*it, 0);
+    lastUpperValue -= 8 - folly::findLastSet(*it);
+    lastValue_ = readLowerPart(size_ - 1) | (lastUpperValue << numLowerBits_);
   }
 
   void reset() {
@@ -661,21 +624,25 @@ class EliasFanoReader {
   }
 
   bool previous() {
-    if (LIKELY(upper_.previous())) {
-      setCurrentValue();
-      return true;
-    }
+    if (!kUnchecked && UNLIKELY(position() == 0)) {
       reset();
       return false;
     }
-
-  bool next() {
-    if (LIKELY(upper_.next())) {
-      setCurrentValue();
+    upper_.previous();
+    value_ =
+        readLowerPart(upper_.position()) | (upper_.value() << numLowerBits_);
     return true;
   }
+
+  bool next() {
+    if (!kUnchecked && UNLIKELY(position() + 1 >= size_)) {
       return setDone();
     }
+    upper_.next();
+    value_ =
+        readLowerPart(upper_.position()) | (upper_.value() << numLowerBits_);
+    return true;
+  }
 
   /**
    * Advances by n elements. n = 0 is allowed and has no effect. Returns false
@@ -686,13 +653,16 @@ class EliasFanoReader {
       return valid();
     }
 
-    if (!upper_.skip(n)) {
-      return setDone();
-    }
-    setCurrentValue();
+    if (kUnchecked || LIKELY(position() + n < size_)) {
+      upper_.skip(n);
+      value_ =
+          readLowerPart(upper_.position()) | (upper_.value() << numLowerBits_);
       return true;
     }
 
+    return setDone();
+  }
+
   /**
    * Skips to the first element >= value whose position is greater or equal to
    * the current position. Requires that value >= value() (or that the reader is
@@ -701,26 +671,19 @@ class EliasFanoReader {
   bool skipTo(ValueType value) {
     if (value != kInvalidValue) {
       DCHECK_GE(value + 1, value_ + 1);
-      if (UNLIKELY(value == value_)) {
-        return true;
     }
-    }
-
-    ValueType upperValue = value >> numLowerBits_;
 
-    if (UNLIKELY(!upper_.skipToNext(upperValue))) {
-      // The value is beyond the last value in the list.
+    if (!kUnchecked && UNLIKELY(value > lastValue_)) {
       return setDone();
+    } else if (UNLIKELY(value == value_)) {
+      return true;
     }
 
-    do {
-      setCurrentValue();
-      if (LIKELY(value_ >= value)) {
+    ValueType upperValue = value >> numLowerBits_;
+    upper_.skipToNext(upperValue);
+    iterateTo(value);
     return true;
   }
-    } while (LIKELY(upper_.next()));
-    return setDone();
-  }
 
   /**
    * Prepare to skip to `value` by prefetching appropriate memory in both the
@@ -730,33 +693,31 @@ class EliasFanoReader {
     // Also works when value_ == kInvalidValue.
     if (value != kInvalidValue) {
       DCHECK_GE(value + 1, value_ + 1);
-      if (UNLIKELY(value == value_)) {
-        return;
     }
+
+    if ((!kUnchecked && value > lastValue_) || (value == value_)) {
+      return;
     }
 
     // Do minimal computation required to prefetch address used in
     // `readLowerPart()`.
     ValueType upperValue = (value >> numLowerBits_);
-    const auto skipDetails = upper_.prepareSkipTo(upperValue);
-    // If we have a valid skip position, prefetch the lower bits for it.
-    if (LIKELY(skipDetails.first)) {
-      const auto addr = lower_ + (skipDetails.second * numLowerBits_ / 8);
+    const auto upperPosition = upper_.prepareSkipTo(upperValue);
+    const auto addr = lower_ + (upperPosition * numLowerBits_ / 8);
     __builtin_prefetch(addr);
     __builtin_prefetch(addr + kCacheLineSize);
   }
-  }
 
   /**
    * Jumps to the element at position n. The reader can be in any state. Returns
    * false if n >= size().
    */
   bool jump(SizeType n) {
-    if (n + 1 < position() + 1) { // Also works if position() == -1.
+    if (n + 1 < upper_.position() + 1) { // Also works if position() == -1.
       reset();
       n += 1; // Initial position is -1.
     } else {
-      n -= position();
+      n -= upper_.position();
     }
     return skip(n);
   }
@@ -777,7 +738,7 @@ class EliasFanoReader {
       // We might be in the middle of a run, iterate backwards to the beginning.
       auto valueLower = Instructions::bzhi(value_, numLowerBits_);
       while (!upper_.isAtBeginningOfRun() &&
-             readLowerPart(position() - 1) == valueLower) {
+             readLowerPart(upper_.position() - 1) == valueLower) {
         upper_.previous();
       }
       return true;
@@ -792,15 +753,20 @@ class EliasFanoReader {
     return skipTo(value);
   }
 
+  ValueType lastValue() const {
+    CHECK(!kUnchecked);
+    return lastValue_;
+  }
+
   ValueType previousValue() const {
     DCHECK_GT(position(), 0);
     DCHECK_LT(position(), size());
-    return readLowerPart(position() - 1) |
+    return readLowerPart(upper_.position() - 1) |
         (upper_.previousValue() << numLowerBits_);
   }
 
   SizeType size() const {
-    return upper_.size();
+    return size_;
   }
 
   bool valid() const {
@@ -821,27 +787,37 @@ class EliasFanoReader {
 
   FOLLY_ALWAYS_INLINE bool setDone() {
     value_ = kInvalidValue;
-    return upper_.setDone();
-  }
-
-  FOLLY_ALWAYS_INLINE void setCurrentValue() {
-    value_ = readLowerPart(position()) | (upper_.value() << numLowerBits_);
+    upper_.setDone(size_);
+    return false;
   }
 
   FOLLY_ALWAYS_INLINE ValueType readLowerPart(SizeType i) const {
-    DCHECK_LT(i, size());
+    DCHECK_LT(i, size_);
     const size_t pos = i * numLowerBits_;
     const unsigned char* ptr = lower_ + (pos / 8);
     const uint64_t ptrv = folly::loadUnaligned<uint64_t>(ptr);
     // This removes the branch in the fallback implementation of
     // bzhi. The condition is verified at encoding time.
-    folly::assume(numLowerBits_ < sizeof(ValueType) * 8);
+    assume(numLowerBits_ < sizeof(ValueType) * 8);
     return Instructions::bzhi(ptrv >> (pos % 8), numLowerBits_);
   }
 
-  detail::UpperBitsReader<Encoder, Instructions, SizeType, kUnchecked> upper_;
+  FOLLY_ALWAYS_INLINE void iterateTo(ValueType value) {
+    while (true) {
+      value_ =
+          readLowerPart(upper_.position()) | (upper_.value() << numLowerBits_);
+      if (LIKELY(value_ >= value)) {
+        break;
+      }
+      upper_.next();
+    }
+  }
+
+  detail::UpperBitsReader<Encoder, Instructions, SizeType> upper_;
   const uint8_t* lower_;
+  SizeType size_;
   ValueType value_ = kInvalidValue;
+  ValueType lastValue_;
   uint8_t numLowerBits_;
 };
 

folly/experimental/test/CodingTestUtils.h

@@ -35,11 +35,6 @@
 namespace folly {
 namespace compression {
 
-template <typename ValueType, class List>
-folly::Optional<std::size_t> getUniverseUpperBound(const List& /* list */) {
-  return folly::none;
-}
-
 template <class URNG>
 std::vector<uint64_t> generateRandomList(
     size_t n,
@@ -273,16 +268,6 @@ void testSkipTo(const std::vector<uint64_t>& data, const List& list) {
     EXPECT_EQ(reader.position(), reader.size());
     EXPECT_FALSE(reader.next());
   }
-  // Skip past the last element and before the upperBound.
-  using ValueType = typename Reader::ValueType;
-  if (const auto upperBound = getUniverseUpperBound<ValueType>(list);
-      upperBound && *upperBound != data.back()) {
-    Reader reader(list);
-    EXPECT_FALSE(reader.skipTo(*upperBound));
-    EXPECT_FALSE(reader.valid());
-    EXPECT_EQ(reader.position(), reader.size());
-    EXPECT_FALSE(reader.next());
-  }
 }
 
 template <class Reader, class List>
@@ -377,19 +362,9 @@ void testEmpty() {
   }
 }
 
-// `upperBoundExtension` is required to inject additional 0-blocks
-// at the end of the list. This allows us to test lists with a large gap between
-// last element and universe upper bound, to exercise bounds-checking when
-// skipping past the last element
 template <class Reader, class Encoder>
-void testAll(
-    const std::vector<uint64_t>& data,
-    uint64_t upperBoundExtension = 0) {
-  Encoder encoder(data.size(), data.back() + upperBoundExtension);
-  for (const auto value : data) {
-    encoder.add(value);
-  }
-  auto list = encoder.finish();
+void testAll(const std::vector<uint64_t>& data) {
+  auto list = Encoder::encode(data.begin(), data.end());
   testNext<Reader>(data, list);
   testSkip<Reader>(data, list);
   testSkipTo<Reader>(data, list);

folly/experimental/test/EliasFanoCodingTest.cpp

@@ -21,32 +21,11 @@
 #include <vector>
 
 #include <folly/Benchmark.h>
-#include <folly/Optional.h>
-#include <folly/Random.h>
 #include <folly/experimental/EliasFanoCoding.h>
 #include <folly/experimental/Select64.h>
 #include <folly/experimental/test/CodingTestUtils.h>
 #include <folly/init/Init.h>
 
-namespace folly {
-namespace compression {
-
-// Overload to help CodingTestUtils retrieve the universe upperbound
-// of the list for certain test cases.
-template <typename ValueType, typename T>
-folly::Optional<std::size_t> getUniverseUpperBound(
-    const EliasFanoCompressedListBase<T>& list) {
-  constexpr ValueType maxUpperValue = std::numeric_limits<ValueType>::max();
-  const ValueType maxUpperBits = maxUpperValue >> list.numLowerBits;
-  const ValueType upperBitsUniverse = std::min(
-      static_cast<ValueType>(8 * list.upperSizeBytes - list.size),
-      maxUpperBits);
-  return (upperBitsUniverse << list.numLowerBits) |
-      ((1 << list.numLowerBits) - 1);
-}
-} // namespace compression
-} // namespace folly
-
 using namespace folly::compression;
 
 namespace {
@@ -135,13 +114,6 @@ class EliasFanoCodingTest : public ::testing::Test {
     // max() cannot be read, as it is assumed an invalid value.
     // TODO(ott): It should be possible to lift this constraint.
     testAll<Reader, Encoder>({0, 1, std::numeric_limits<uint32_t>::max() - 1});
-    // Test data with additional trailing 0s in the upperBits by extending
-    // the upper bound.
-    constexpr uint64_t minUpperBoundExtension = 2;
-    constexpr uint64_t maxUpperBoundExtension = 1024;
-    testAll<Reader, Encoder>(
-        generateRandomList(100 * 1000, 10 * 1000 * 1000),
-        folly::Random::rand32(minUpperBoundExtension, maxUpperBoundExtension));
   }
 };