refactor: extract QList move helper and ZSTD dict predicates by romange · Pull Request #7755 · dragonflydb/dragonfly

romange · 2026-06-30T16:54:25Z

Summary

extract a shared MoveFrom() path for QList move construction and move assignment
rename the ZSTD bulk-compression failure state and helper entry point for clearer intent
replace repeated inline ZSTD guard logic with small helper predicates without changing behavior

Changes

update src/core/qlist.cc to reuse MoveFrom() from the move constructor and assignment operator
rename dict_compress_failed_ to dict_bulk_failed_ and CompressWithZstdDict() to BackfillCompressWithZstdDict()
add IsZstdDictMode(), IsInterior(), and CanCompressWithZstdDict() in src/core/qlist.h and use them in the ZSTD compression paths

Test plan

Not run; refactor-only split of an existing branch commit.

Route QList move construction and assignment through MoveFrom() so the shared state transfer lives in one place. Rename the ZSTD bulk-compression failure flag and helper entry point to better reflect their roles, and add IsZstdDictMode(), IsInterior(), and CanCompressWithZstdDict() to collapse repeated guard logic. No functional changes intended.

qodo-free-for-open-source-projects · 2026-06-30T16:55:37Z

PR Summary by Qodo

Refactor QList move logic and ZSTD dict-mode guard helpers

✨ Enhancement 🕐 20-40 Minutes

AI Description

• Route QList move ctor/assignment through MoveFrom() to centralize state transfer.
• Rename ZSTD bulk-compression failure flag and bulk-compress entry point for clearer intent.
• Replace repeated ZSTD dict-mode guard checks with small helper predicates (no behavior change).

Diagram

graph TD
  A["QList operations"] --> B["MoveFrom()"]
  A --> C["ZSTD guard predicates"] --> D["BackfillCompressWithZstdDict()"] --> E["CompressNodeWithDict()"] --> F[("tl_zstd_dict")]
  D --> G["dict_bulk_* flags"]
  C --> G

High-Level Assessment

Current approach is appropriate: it removes duplication in move operations and ZSTD dict-mode guards without changing semantics. Alternatives like swap-based moves or leaving guards inline would either add overhead/complexity or keep duplication, with no clear benefit.

Files changed (2) +47 / -43

Refactor (2) +47 / -43

qlist.ccCentralize move state transfer and refactor ZSTD dict-mode call sites +31/-40

Centralize move state transfer and refactor ZSTD dict-mode call sites

• Routes the move constructor and move assignment operator through a new MoveFrom() helper. Renames the ZSTD bulk-compression entry point to BackfillCompressWithZstdDict() and updates dict-mode branches to use the new predicate helpers, including the renamed bulk-failure flag.

src/core/qlist.cc

qlist.hAdd ZSTD dict-mode predicates and rename bulk-failure flag/API +16/-3

Add ZSTD dict-mode predicates and rename bulk-failure flag/API

• Introduces IsZstdDictMode(), IsInterior(), and CanCompressWithZstdDict() to consolidate repeated guard logic in compression paths. Renames dict_compress_failed_ to dict_bulk_failed_ and updates the bulk-compression method declaration to BackfillCompressWithZstdDict().

src/core/qlist.h

augmentcode · 2026-06-30T16:59:33Z

🤖 Augment PR Summary

Summary: This PR refactors QList move semantics and simplifies the ZSTD dictionary compression guard logic without intending to change behavior.

Changes:

Extracts shared move logic into `QList::MoveFrom()` and reuses it from the move constructor and move assignment operator.
Renames the ZSTD dictionary bulk-compression failure flag to clarify intent (`dict_compress_failed_` → `dict_bulk_failed_`).
Renames the bulk-compression entry point for readability (`CompressWithZstdDict()` → `BackfillCompressWithZstdDict()`).
Adds small helper predicates (`IsZstdDictMode()`, `IsInterior()`, `CanCompressWithZstdDict()`) to remove repeated inline checks in ZSTD paths.

Technical Notes: The ZSTD dict mode remains mutually exclusive with LZF depth-compression; the refactor aims to keep the same control flow while centralizing the conditions.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-06-30T16:59:34Z

  return *this;
 }

+void QList::MoveFrom(QList&& other) {


MoveFrom() copies most state, but it doesn’t transfer fields like malloc_size_ (used by MallocUsed(false) and ZSTD training thresholds) or per-list configuration like db_id_/zstd_threshold_, so a moved-to populated QList may end up with inconsistent accounting/config. Is it guaranteed that populated lists are never moved in a way that relies on these fields afterwards?

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

qodo-free-for-open-source-projects · 2026-06-30T17:00:07Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

1. Move drops QList state 🐞 Bug ≡ Correctness

Description

QList::MoveFrom transfers the node chain but does not move malloc_size_, zstd_threshold_, or db_id_,
so moved-to lists lose memory accounting and ZSTD-dict enablement/configuration even though they
still own nodes. This can under-report MallocUsed(false) and prevent ZSTD dict training/compression
decisions that depend on malloc_size_ and zstd_threshold_.

Code

src/core/qlist.cc[R496-511]

+void QList::MoveFrom(QList&& other) {
+  head_ = other.head_;
+  len_ = other.len_;
+  count_ = other.count_;
+  fill_ = other.fill_;
+  dict_learning_failed_ = other.dict_learning_failed_;
+  dict_bulk_failed_ = other.dict_bulk_failed_;
+  dict_bulk_finished_ = other.dict_bulk_finished_;
+  tiering_enabled_ = other.tiering_enabled_;
+  compress_ = other.compress_;
+  bookmark_count_ = other.bookmark_count_;
+  tiering_params_ = std::move(other.tiering_params_);
+
+  other.head_ = nullptr;
+  other.len_ = other.count_ = 0;
+}

Evidence

The refactor introduces MoveFrom as the single path for move construction/assignment, but it only
copies pointer/len/count and some flags, omitting malloc_size_, zstd_threshold_, and db_id_.
Those omitted fields directly affect MallocUsed(false) (which adds malloc_size_) and the ZSTD
dict compression gate (which is configured via set_compr_threshold() and checks `malloc_size_ >=
zstd_threshold_`).

src/core/qlist.cc[470-511]
src/core/qlist.cc[661-675]
src/core/qlist.cc[960-974]
src/core/qlist.cc[1628-1641]
src/core/qlist.h[272-276]
src/core/qlist.h[386-402]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`QList::MoveFrom(QList&&)` copies only a subset of member fields. After a move, the destination list owns the nodes but retains default values for `malloc_size_`, `zstd_threshold_`, and `db_id_`, which are used for memory accounting and ZSTD dict compression gating.

## Issue Context
- `MallocUsed(false)` returns `node_size + malloc_size_`, so `malloc_size_` must remain accurate after a move.
- ZSTD dict mode is enabled/gated via `set_compr_threshold()` (`zstd_threshold_`) and compares against `malloc_size_` in `CoolOff()` / `CompressAfterLoad()`.
- `db_id_` is set via `SetDbIndex()` and is part of QList's operational state (tiering path checks/materialization decisions depend on it).

## Fix Focus Areas
- src/core/qlist.cc[496-511]
- src/core/qlist.cc[661-675]
- src/core/qlist.h[272-276]
- src/core/qlist.h[386-402]

## Suggested fix
1. In `MoveFrom`, also transfer:
  - `malloc_size_ = other.malloc_size_;`
  - `zstd_threshold_ = other.zstd_threshold_;`
  - `db_id_ = other.db_id_;`
2. Reset the moved-from fields to safe defaults (`other.malloc_size_ = 0; other.zstd_threshold_ = 0; other.db_id_ = kInvalidDbId;`) alongside the existing `head_/len_/count_` resets.
3. (Optional hardening) Mark `MoveFrom` as `noexcept` since it is used by `noexcept` move ctor/assignment.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-free-for-open-source-projects · 2026-06-30T17:00:08Z

+void QList::MoveFrom(QList&& other) {
+  head_ = other.head_;
+  len_ = other.len_;
+  count_ = other.count_;
+  fill_ = other.fill_;
+  dict_learning_failed_ = other.dict_learning_failed_;
+  dict_bulk_failed_ = other.dict_bulk_failed_;
+  dict_bulk_finished_ = other.dict_bulk_finished_;
+  tiering_enabled_ = other.tiering_enabled_;
+  compress_ = other.compress_;
+  bookmark_count_ = other.bookmark_count_;
+  tiering_params_ = std::move(other.tiering_params_);
+
+  other.head_ = nullptr;
+  other.len_ = other.count_ = 0;
+}


1. Move drops qlist state 🐞 Bug ≡ Correctness

QList::MoveFrom transfers the node chain but does not move malloc_size_, zstd_threshold_, or db_id_, so moved-to lists lose memory accounting and ZSTD-dict enablement/configuration even though they still own nodes. This can under-report MallocUsed(false) and prevent ZSTD dict training/compression decisions that depend on malloc_size_ and zstd_threshold_.

Agent Prompt

## Issue description `QList::MoveFrom(QList&&)` copies only a subset of member fields. After a move, the destination list owns the nodes but retains default values for `malloc_size_`, `zstd_threshold_`, and `db_id_`, which are used for memory accounting and ZSTD dict compression gating. ## Issue Context - `MallocUsed(false)` returns `node_size + malloc_size_`, so `malloc_size_` must remain accurate after a move. - ZSTD dict mode is enabled/gated via `set_compr_threshold()` (`zstd_threshold_`) and compares against `malloc_size_` in `CoolOff()` / `CompressAfterLoad()`. - `db_id_` is set via `SetDbIndex()` and is part of QList's operational state (tiering path checks/materialization decisions depend on it). ## Fix Focus Areas - src/core/qlist.cc[496-511] - src/core/qlist.cc[661-675] - src/core/qlist.h[272-276] - src/core/qlist.h[386-402] ## Suggested fix 1. In `MoveFrom`, also transfer: - `malloc_size_ = other.malloc_size_;` - `zstd_threshold_ = other.zstd_threshold_;` - `db_id_ = other.db_id_;` 2. Reset the moved-from fields to safe defaults (`other.malloc_size_ = 0; other.zstd_threshold_ = 0; other.db_id_ = kInvalidDbId;`) alongside the existing `head_/len_/count_` resets. 3. (Optional hardening) Mark `MoveFrom` as `noexcept` since it is used by `noexcept` move ctor/assignment.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

mkaruza

lgtm

kostasrim · 2026-06-30T17:15:00Z

@romange before you merge this plz double check bot's comment. I understand you just moved the code as it was but it's write we don't copy/omit some fields

romange · 2026-06-30T17:24:44Z

@romange before you merge this plz double check bot's comment. I understand you just moved the code as it was but it's write we don't copy/omit some fields

you are correct, the next PR fixes this, it was too confusing to do all the changes in the same commit.

romange requested review from abhijat and mkaruza June 30, 2026 16:55

augmentcode Bot reviewed Jun 30, 2026

View reviewed changes

qodo-free-for-open-source-projects Bot reviewed Jun 30, 2026

View reviewed changes

mkaruza approved these changes Jun 30, 2026

View reviewed changes

romange enabled auto-merge (squash) June 30, 2026 17:25

romange merged commit 33b08e4 into main Jun 30, 2026
13 checks passed

romange deleted the refactor-qlist-zstd-dict-helpers branch June 30, 2026 18:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: extract QList move helper and ZSTD dict predicates#7755

refactor: extract QList move helper and ZSTD dict predicates#7755
romange merged 1 commit into
mainfrom
refactor-qlist-zstd-dict-helpers

romange commented Jun 30, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 30, 2026

Uh oh!

augmentcode Bot commented Jun 30, 2026

Uh oh!

augmentcode Bot left a comment

Uh oh!

augmentcode Bot Jun 30, 2026 •

edited

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 30, 2026

Uh oh!

qodo-free-for-open-source-projects Bot Jun 30, 2026

Uh oh!

mkaruza left a comment

Uh oh!

kostasrim commented Jun 30, 2026

Uh oh!

romange commented Jun 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

romange commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Test plan

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 30, 2026

PR Summary by Qodo

Uh oh!

augmentcode Bot commented Jun 30, 2026

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qodo-free-for-open-source-projects Bot commented Jun 30, 2026

Code Review by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot Jun 30, 2026

Choose a reason for hiding this comment

Uh oh!

mkaruza left a comment

Choose a reason for hiding this comment

Uh oh!

kostasrim commented Jun 30, 2026

Uh oh!

romange commented Jun 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

romange commented Jun 30, 2026 •

edited

Loading

augmentcode Bot Jun 30, 2026 •

edited

Loading