Merge branch 'dragonflydb:main' into tls_info

Author: BenjaminGoehry

src/core/collection_entry.h

@@ -21,6 +21,8 @@ struct CollectionEntry {
   explicit CollectionEntry(long long longval) : value_{nullptr}, longval_{longval} {
   }
 
+  CollectionEntry& operator=(const CollectionEntry&) = default;
+
   std::string ToString() const {
     if (value_)
       return {value_, length_};

src/core/detail/listpack.cc

@@ -88,15 +88,11 @@ bool ListPack::Insert(string_view pivot, string_view elem, QList::InsertOpt inse
   return false;
 }
 
-unsigned ListPack::Remove(string_view elem, unsigned count, QList::Where where) {
+unsigned ListPack::Remove(const CollectionEntry& elem, unsigned count, QList::Where where) {
   unsigned removed = 0;
-  int64_t ival;
-
-  // try parsing the element into an integer.
-  int is_int = lpStringToInt64(elem.data(), elem.size(), &ival);
 
   auto is_match = [&](const QList::Entry& entry) {
-    return is_int ? entry.is_int() && entry.ival() == ival : entry == elem;
+    return elem.is_int() ? entry.is_int() && entry.ival() == elem.ival() : entry == elem.view();
   };
 
   uint8_t* p = GetFirst(where);

src/core/detail/listpack.h

@@ -50,7 +50,7 @@ class ListPack {
   bool Insert(std::string_view pivot, std::string_view elem, QList::InsertOpt insert_opt);
 
   // Removes up to count occurrences of elem from the specified direction.
-  unsigned Remove(std::string_view elem, unsigned count, QList::Where where);
+  unsigned Remove(const CollectionEntry& elem, unsigned count, QList::Where where);
 
   // Replaces the element at the specified index with a new value.
   bool Replace(long index, std::string_view elem);
@@ -66,7 +66,7 @@ class ListPack {
   }
 
  private:
-  static QList::Entry GetEntry(uint8_t* pos);
+  static CollectionEntry GetEntry(uint8_t* pos);
 
   uint8_t* GetFirst(QList::Where where) const {
     return (where == QList::HEAD) ? lpFirst(lp_) : lpLast(lp_);

src/core/listpack_test.cc

@@ -39,6 +39,10 @@ class ListPackTest : public ::testing::Test {
     EXPECT_EQ(zmalloc_used_memory_tl, 0);
   }
 
+  unsigned Remove(string_view elem, unsigned count, QList::Where where) {
+    return lp_.Remove(CollectionEntry{elem.data(), elem.size()}, count, where);
+  }
+
   ListPack lp_;
   uint8_t* ptr_ = nullptr;
 };
@@ -59,7 +63,7 @@ TEST_F(ListPackTest, RemoveIntegerFromHead) {
   lp_.Push("3", QList::TAIL);
 
   // Remove integer value "1" from head
-  unsigned removed = lp_.Remove("1", 0, QList::HEAD);
+  unsigned removed = Remove("1", 0, QList::HEAD);
   EXPECT_EQ(2, removed);
   EXPECT_EQ(2, lp_.Size());
 
@@ -76,7 +80,7 @@ TEST_F(ListPackTest, RemoveFromTailAll) {
   lp_.Push("a", QList::TAIL);
 
   // Remove all "a" from tail direction
-  unsigned removed = lp_.Remove("a", 0, QList::TAIL);
+  unsigned removed = Remove("a", 0, QList::TAIL);
   EXPECT_EQ(3, removed);
   EXPECT_EQ(2, lp_.Size());
 
@@ -94,7 +98,7 @@ TEST_F(ListPackTest, RemoveFromTailWithCount) {
   lp_.Push("a", QList::TAIL);
 
   // Remove only 2 occurrences of "a" from tail (removes indices 4 and 2)
-  unsigned removed = lp_.Remove("a", 2, QList::TAIL);
+  unsigned removed = Remove("a", 2, QList::TAIL);
   EXPECT_EQ(2, removed);
   EXPECT_EQ(3, lp_.Size());
 
@@ -113,7 +117,7 @@ TEST_F(ListPackTest, RemoveFromTailConsecutive) {
   lp_.Push("target", QList::TAIL);
   lp_.Push("target", QList::TAIL);
 
-  unsigned removed = lp_.Remove("target", 0, QList::TAIL);
+  unsigned removed = Remove("target", 0, QList::TAIL);
   EXPECT_EQ(3, removed);
   EXPECT_EQ(1, lp_.Size());
   EXPECT_EQ("x", lp_.At(0));
@@ -129,7 +133,7 @@ TEST_F(ListPackTest, RemoveFromTailDeletesHead) {
   lp_.Push("b", QList::TAIL);
   lp_.Push("c", QList::TAIL);
 
-  unsigned removed = lp_.Remove("a", 0, QList::TAIL);
+  unsigned removed = Remove("a", 0, QList::TAIL);
   EXPECT_EQ(1, removed);
   EXPECT_EQ(2, lp_.Size());
 

src/core/qlist.cc

@@ -611,19 +611,21 @@ void QList::AppendPlain(unsigned char* data, size_t sz) {
 bool QList::Insert(std::string_view pivot, std::string_view elem, InsertOpt opt) {
   Iterator it = GetIterator(HEAD);
 
-  while (it.Next()) {
-    if (it.Get() == pivot) {
-      Insert(it, elem, opt);
-      return true;
-    }
+  if (it.Valid()) {
+    do {
+      if (it.Get() == pivot) {
+        Insert(it, elem, opt);
+        return true;
+      }
+    } while (it.Next());
   }
 
   return false;
 }
 
 bool QList::Replace(long index, std::string_view elem) {
   Iterator it = GetIterator(index);
-  if (it.Next()) {
+  if (it.Valid()) {
     Replace(it, elem);
     return true;
   }
@@ -650,10 +652,12 @@ void QList::Iterate(IterateFunc cb, long start, long end) const {
   if (end < 0 || end >= long(Size()))
     end = Size() - 1;
   Iterator it = GetIterator(start);
-  while (start <= end && it.Next()) {
-    if (!cb(it.Get()))
-      break;
-    start++;
+  if (it.Valid()) {
+    do {
+      if (start > end || !cb(it.Get()))
+        break;
+      start++;
+    } while (it.Next());
   }
 }
 
@@ -1053,6 +1057,16 @@ bool QList::DelPackedIndex(Node* node, uint8_t* p) {
   return false;
 }
 
+void QList::InitIteratorEntry(Iterator* it) const {
+  DCHECK(it->current_);
+  const_cast<QList*>(this)->malloc_size_ += DecompressNodeIfNeeded(true, it->current_);
+  if (QL_NODE_IS_PLAIN(it->current_)) {
+    it->zi_ = it->current_->entry;
+  } else {
+    it->zi_ = lpSeek(it->current_->entry, it->offset_);
+  }
+}
+
 auto QList::GetIterator(Where where) const -> Iterator {
   Iterator it;
   it.owner_ = this;
@@ -1067,6 +1081,10 @@ auto QList::GetIterator(Where where) const -> Iterator {
     it.direction_ = REV;
   }
 
+  if (it.current_) {
+    InitIteratorEntry(&it);
+  }
+
   return it;
 }
 
@@ -1119,6 +1137,8 @@ auto QList::GetIterator(long idx) const -> Iterator {
     iter.offset_ = (-index) - 1 + accum;
   }
 
+  InitIteratorEntry(&iter);
+
   return iter;
 }
 
@@ -1137,10 +1157,9 @@ auto QList::Erase(Iterator it) -> Iterator {
     deleted_node = DelPackedIndex(node, it.zi_);
   }
 
-  /* after delete, the zi is now invalid for any future usage. */
-  it.zi_ = NULL;
+  it.zi_ = NULL;  // Reset current entry pointer
 
-  /* If current node is deleted, we must update iterator node and offset. */
+  // If current node is deleted, we must update iterator node and offset.
   if (deleted_node) {
     if (it.direction_ == FWD) {
       it.current_ = next;
@@ -1151,6 +1170,10 @@ auto QList::Erase(Iterator it) -> Iterator {
     }
   }
 
+  if (it.current_) {
+    InitIteratorEntry(&it);
+  }
+
   // Sanity, should be noop in release mode.
   if (len_ == 1) {
     DCHECK_EQ(count_, head_->count);
@@ -1241,29 +1264,35 @@ bool QList::Erase(const long start, unsigned count) {
   return true;
 }
 
+uint8_t* QList::TryExtractListpack() {
+  // TODO: enable this once we support listpack encoding for lists.
+  return nullptr;
+
+  if (len_ != 1 || QL_NODE_IS_PLAIN(head_) || !ShouldStoreAsListPack(head_->sz) ||
+      head_->IsCompressed()) {
+    return nullptr;
+  }
+
+  uint8_t* res = std::exchange(head_->entry, nullptr);
+  DelNode(head_);
+
+  return res;
+}
+
 bool QList::Iterator::Next() {
   if (!current_)
     return false;
 
   int plain = QL_NODE_IS_PLAIN(current_);
-  if (!zi_) {
-    /* If !zi, use current index. */
-    const_cast<QList*>(owner_)->malloc_size_ += DecompressNodeIfNeeded(true, current_);
-    if (ABSL_PREDICT_FALSE(plain))
-      zi_ = current_->entry;
-    else
-      zi_ = lpSeek(current_->entry, offset_);
-  } else if (ABSL_PREDICT_FALSE(plain)) {
+
+  // Advance to the next element in the current node.
+  if (ABSL_PREDICT_FALSE(plain)) {
     zi_ = NULL;
   } else {
-    unsigned char* (*nextFn)(unsigned char*, unsigned char*) = NULL;
-    int offset_update = 0;
+    unsigned char* (*nextFn)(unsigned char*, unsigned char*) = lpNext;
+    int offset_update = 1;
 
-    /* else, use existing iterator offset and get prev/next as necessary. */
-    if (direction_ == FWD) {
-      nextFn = lpNext;
-      offset_update = 1;
-    } else {
+    if (direction_ == REV) {
       DCHECK_EQ(REV, direction_);
       nextFn = lpPrev;
       offset_update = -1;
@@ -1275,7 +1304,7 @@ bool QList::Iterator::Next() {
   if (zi_)
     return true;
 
-  // Retry again with the next node.
+  // Move to the next node.
   const_cast<QList*>(owner_)->Compress(current_);
 
   if (direction_ == FWD) {
@@ -1289,7 +1318,11 @@ bool QList::Iterator::Next() {
     current_ = (current_ == owner_->head_) ? nullptr : current_->prev;
   }
 
-  return current_ ? Next() : false;
+  if (!current_)
+    return false;
+
+  owner_->InitIteratorEntry(this);
+  return zi_ != nullptr;
 }
 
 auto QList::Iterator::Get() const -> Entry {

src/core/qlist.h

@@ -6,6 +6,7 @@
 
 #include <absl/functional/function_ref.h>
 
+#include <cstdint>
 #include <string>
 
 #include "core/collection_entry.h"
@@ -27,6 +28,14 @@ namespace dfly {
 
 class PageUsage;
 
+// Heuristic: for values smaller than 2 KiB we prefer the compact listpack
+// representation. 2048 was chosen as a conservative threshold that matches
+// common quicklist usage patterns and avoids creating very large listpacks
+// that are costly to reallocate or compress.
+inline bool ShouldStoreAsListPack(size_t size) {
+  return size < 2048;
+}
+
 class QList {
  public:
   enum Where : uint8_t { TAIL, HEAD };
@@ -69,9 +78,14 @@ class QList {
   using Entry = CollectionEntry;
   class Iterator {
    public:
+    // Returns true if the iterator is valid (points to an element).
+    bool Valid() const {
+      return zi_ != nullptr;
+    }
+
     Entry Get() const;
 
-    // Returns false if no more entries.
+    // Advances to the next/prev element. Returns false if no more entries.
     bool Next();
 
    private:
@@ -138,8 +152,8 @@ class QList {
   // Returns the popped value. Precondition: list is not empty.
   std::string Pop(Where where);
 
-  void AppendListpack(unsigned char* zl);
-  void AppendPlain(unsigned char* zl, size_t sz);
+  void AppendListpack(uint8_t* zl);
+  void AppendPlain(uint8_t* zl, size_t sz);
 
   // Returns true if pivot found and elem inserted, false otherwise.
   bool Insert(std::string_view pivot, std::string_view elem, InsertOpt opt);
@@ -155,14 +169,13 @@ class QList {
   void Iterate(IterateFunc cb, long start, long end) const;
 
   // Returns an iterator to tail or the head of the list.
-  // To mirror the quicklist interface, the iterator is not valid until Next() is called.
-  // TODO: to fix this.
+  // result.Valid() is true if the list is not empty.
   Iterator GetIterator(Where where) const;
 
   // Returns an iterator at a specific index 'idx',
   // or Invalid iterator if index is out of range.
   // negative index - means counting from the tail.
-  // Requires calling subsequent Next() to initialize the iterator.
+  // result.Valid() is true if the index is within range.
   Iterator GetIterator(long idx) const;
 
   uint32_t node_count() const {
@@ -188,6 +201,11 @@ class QList {
     return _Tail();
   }
 
+  // Returns nullptr if quicklist does not fit the necessary requirements
+  // to be converted to listpack, and listpack otherwise. The ownership over the listpack
+  // blob is moved to the caller.
+  uint8_t* TryExtractListpack();
+
   void set_fill(int fill) {
     fill_ = fill;
   }
@@ -244,6 +262,10 @@ class QList {
   void DelNode(Node* node);
   bool DelPackedIndex(Node* node, uint8_t* p);
 
+  // Initializes iterator's zi_ to point to the element at offset_.
+  // Decompresses the node if needed. Assumes current_ is not null.
+  void InitIteratorEntry(Iterator* it) const;
+
   Node* head_ = nullptr;
   size_t malloc_size_ = 0;  // size of the quicklist struct
   uint32_t count_ = 0;      /* total count of all entries in all listpacks */