Skip to content

Fix quadratic zstd list load compression#7753

Merged
romange merged 2 commits into
mainfrom
fix-zstd-list-load
Jul 1, 2026
Merged

Fix quadratic zstd list load compression#7753
romange merged 2 commits into
mainfrom
fix-zstd-list-load

Conversation

@romange

@romange romange commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary:
Fix chunked RDB list loading with experimental zstd dictionaries so post-load compression scans only newly appended nodes after the initial pass.

Changes:

  • Add incremental post-load compression for appended list chunks
  • Preserve QList move state needed by chunked load handoff
  • Add a regression test for chunked append after move

Validation:

  • pre-commit run --files src/core/qlist.cc src/core/qlist.h src/core/qlist_test.cc
  • ninja -C build-opt qlist_test && ./build-opt/qlist_test --gtest_filter=QListZstdTest.*
  • ninja -C build-opt dragonfly
  • Timed DFLY LOAD on the provided DFS snapshot with and without the zstd flag

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Avoid quadratic ZSTD dict compression during chunked QList loads

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Make post-load ZSTD compression incremental so only newly appended nodes are scanned.
• Preserve QList move state required for chunked-load handoff (threshold/db/malloc size).
• Add a regression test covering chunked append after move + post-load compression.
Diagram

flowchart TD
  loader["Chunked RDB load"] --> qlist["QList"] --> afterload["CompressAfterLoad()"] --> first{"Bulk pass done?"}
  first -->|"Yes"| incr["Compress appended"] --> nodecomp["Compress node"]
  first -->|"No"| dict["Train or reuse dict"] --> bulk["Compress with dict"] --> nodecomp
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Track a tail boundary pointer/index for incremental compression
  • ➕ Avoids per-node state like attempted_compress
  • ➕ Makes the incremental region explicit (e.g., last-compressed node marker)
  • ➖ Harder to keep correct across node merges/splits/deletes
  • ➖ More state to maintain during mutations and moves
2. Move compression triggering into AppendListpack once dict_bulk_finished_ is set
  • ➕ Eliminates the need for repeated CompressAfterLoad calls to do incremental work
  • ➕ Keeps compression closer to the append that created new interior nodes
  • ➖ Adds overhead/branching to hot append paths
  • ➖ More complicated behavior during bulk-load vs normal operation

Recommendation: Current approach is the best trade-off: it reuses an existing 1-bit Node field (attempted_compress) to establish a clear stop condition, making incremental scans robust and simple. Preserving move state (malloc_size_/db_id_/zstd_threshold_) keeps chunked-load handoff behavior correct without introducing additional boundary-tracking complexity.

Files changed (3) +83 / -2

Bug fix (2) +45 / -2
qlist.ccIncremental ZSTD post-load compression + preserve state on move +42/-2

Incremental ZSTD post-load compression + preserve state on move

• Initializes Node::attempted_compress for raw nodes and marks nodes on each compression attempt. Adds CompressAppendedWithZstdDict() to walk backward from the tail and compress only newly appended (previously unattempted) interior nodes, avoiding repeated full-prefix rescans. Extends move ctor/assignment to carry malloc_size_, db_id_, and zstd_threshold_ so chunked load + move retains compression behavior and thresholds.

src/core/qlist.cc

qlist.hDeclare incremental appended-node ZSTD compression helper +3/-0

Declare incremental appended-node ZSTD compression helper

• Introduces a private helper declaration and comment for CompressAppendedWithZstdDict(), documenting that only nodes appended since the prior post-load pass are processed.

src/core/qlist.h

Tests (1) +38 / -0
qlist_test.ccAdd regression test for chunked append after move with ZSTD dict +38/-0

Add regression test for chunked append after move with ZSTD dict

• Adds CompressAfterLoadChunkedAppend test that bulk-loads in chunks, runs CompressAfterLoad, moves the list, appends more chunks, and verifies only the newly-interior former tail gets ZSTD-compressed while head/tail remain raw and all entries remain readable.

src/core/qlist_test.cc

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

@augmentcode

augmentcode Bot commented Jun 30, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR fixes quadratic behavior in QList post-load ZSTD dictionary compression when RDB list loading appends additional listpack chunks.

Changes:

  • Tracks per-node compression attempts to support incremental compression passes after the initial bulk pass
  • Adds an incremental compressor that scans/compresses only newly-appended interior nodes instead of re-walking the full list
  • Preserves additional QList state across moves (e.g. malloc accounting and ZSTD-related fields) needed by the chunked-load handoff
  • Marks dict-compression attempts in CompressNodeWithDict to bound future scans
  • Adds a regression test covering move + chunked append + incremental post-load compression behavior

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. No suggestions at this time.

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

@romange
romange force-pushed the fix-zstd-list-load branch 6 times, most recently from 26d7a3d to 185ec3b Compare June 30, 2026 16:56
Post-load zstd compression now scans backward from the tail and stops at the first already-compressed node, avoiding repeated scans of previously processed chunks.
@romange
romange force-pushed the fix-zstd-list-load branch from 185ec3b to e0de255 Compare June 30, 2026 18:30
@romange
romange requested review from abhijat and mkaruza June 30, 2026 18:32
Comment thread src/core/qlist.cc Outdated
Comment on lines +1631 to +1632
if (any_compressed)
dict_bulk_finished_ = 1;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets say that all nodes we see are too small, so none are compressed, then this flag is not set. then we need to walk the full list every time, when called from CoolOff? because dict_bulk_finished_ is not 1.

Wouldn't it be better to mark the list finished once we are able to walk its length, even if we didnt compress some nodes? What would walking over it a second time (and each time) accomplish, as we can't compress in the second/third/Nth pass anyway?

The only false returns I see in CompressNodeWithDict is when the node is not compressible anyway (too small/not enough gain on compression) which are going to be permanent.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, you have a point.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will make it as conservative as possible.

A completed BackfillCompressWithZstdDict walk is terminal: RAW nodes left
behind can never compress on a rescan since CompressNodeWithDict failures
are permanent. Previously the all-nodes-too-small case set neither flag,
forcing CoolOff to re-walk the whole list on every push. Now only a real
dict mismatch (nodes tried, none compressed) sets dict_bulk_failed_; every
other outcome sets dict_bulk_finished_.
@romange
romange requested a review from abhijat July 1, 2026 06:39
@romange
romange merged commit 6ace3cb into main Jul 1, 2026
13 checks passed
@romange
romange deleted the fix-zstd-list-load branch July 1, 2026 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants