Skip to content

fix: scroll sequential walks were racing and re-reading page one#153

Open
tellet-q wants to merge 2 commits into
devfrom
feat/scroll-walk-seed
Open

fix: scroll sequential walks were racing and re-reading page one#153
tellet-q wants to merge 2 commits into
devfrom
feat/scroll-walk-seed

Conversation

@tellet-q

Copy link
Copy Markdown
Contributor
  • Every walk started at the top. A fresh walk was opened with offset: None, which means "from the beginning of the collection." So all concurrent walks began at point 0 and marched through the same pages in lockstep — the first fetched each page from disk, the rest got cache hits. Walks now open at a random point, so concurrent walks cover different stretches and each page is genuinely read once.
  • Drops the --rps guard. It was added to block mode: sequential with --rps, purely because unbounded concurrency guaranteed the slot collision. The pool removes the race, so the restriction — and its now-false comment — is gone; --rps runs sequential correctly, with the walk count simply floating with the in-flight request count.

before:
walk 0 ─┐
walk 1 ─┼─ all from point 0 ──▶ 1 disk read, N−1 cache hits
walk N ─┘

after:
walk 0 ──── from random point A ──▶
walk 1 ──── from random point B ──▶ each page read once, from disk
walk N ──── from random point C ──▶

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes sequential scroll concurrency so concurrent walks don’t all start from offset None (page one), which previously caused lockstep re-reads and cache hits instead of genuine disk reads. It also removes the previous --rps + mode: sequential prohibition by introducing a walk pool that prevents sharing a cursor between concurrent requests.

Changes:

  • Replaces per-worker fixed walk slots with a mutex-protected pool of in-progress walks that are checked out per request and returned advanced.
  • Seeds new sequential walks from a random point (via a sample: random query) to avoid concurrent walks re-reading the same initial pages.
  • Removes the --rps guard for sequential mode and updates docs/examples to reflect the new sequential behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/scroll.rs Implements the walk pool and random seeding for sequential mode; removes slot-based walk sharing.
src/main.rs Drops the --rps + sequential-mode bail-out now that walks are no longer shared by slot.
README.md Updates sequential mode documentation to describe random seeding / reduced overlap for concurrent walks.
examples/scroll-config.yaml Updates config comments to match the new sequential walk behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/scroll.rs
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