Skip to content

Load the record on dereference, not on every step - #10

Merged
attermann merged 1 commit into
attermann:masterfrom
dobrevit:fix/typed-store-iterator-loads-eagerly
Nov 10, 2026
Merged

attermann merged 1 commit into
attermann:masterfrom
dobrevit:fix/typed-store-iterator-loads-eagerly

Conversation

@mclueppers

@mclueppers mclueppers commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

The problem

TypedStore::iterator decodes the record in its constructor and again in operator++, and load() dereferences the underlying iterator to do it. BasicFileStore is lazy on purpose — it reads only on operator*, and documents operator-> as metadata-only for exactly that reason — so the typed wrapper takes that back one layer up and every step reads the record off the filesystem whether the caller wanted it or not.

Anyone who does not dereference every entry is affected: counting, searching for a key, or stopping early all read the whole prefix. The sharper consequence is that reaching position N always costs N reads, which rules out bounded or resumable traversal — a cursor cannot skip to where it left off more cheaply than re-reading everything before it.

That is not hypothetical. On an ESP32 with the store on LittleFS a record read costs roughly 32 ms, and a table walk long enough to outlast a task watchdog has to be split across passes. We tried exactly that: a time budget per pass and a saved position to resume from. It never advanced — on a node holding 87 paths the walk sat at position 15 for ever, spending its whole budget re-reading the prefix, because stepping over an entry costs the same as reading it. Full report in #9.

To reproduce, count opens while stepping without dereferencing: 8 records, 8 opens.

The solution

Decode in operator* instead, behind a loaded_ flag:

iterator& operator++()      { ++it_; loaded_ = false; return *this; }
Entry&    operator*()       { if (!loaded_) load(); return current_; }
void      load()            { loaded_ = true; if (it_ == end_) return; /* decode */ }

Why the flag. Dereferencing twice at one position must read once, which is what the constructor's eager load gave callers implicitly. The flag keeps that guarantee while making the read happen only if it is asked for.

Why the guard moved into load(). The end iterator is not dereferenceable, and the constructor said so by not loading. With the load deferred, that check belongs where the load now happens.

Trade-offs and limitations. A full walk that dereferences every entry costs exactly what it did before — the same one read per record, moved from ++ to *. Nothing gets faster for that caller; what changes is that callers who skip entries stop paying for them, and bounded traversal becomes expressible. current_ is still overwritten in place, so a reference taken from operator* is still only valid until the iterator moves, exactly as before.

Compatibility. No public API change, no on-disk format change. The only observable difference is when the read happens and how many happen.

Testing

pio test -e native, on Linux/x86-64:

before (master 0f28567):  74 test cases: 3 failed, 70 succeeded
after  (this branch):     75 test cases: 3 failed, 71 succeeded

The three failures are pre-existing on master and unrelated — test_file_store_compact_basic, test_dir_prefix_reload_index and test_dir_prefix_compact, all failing on future-timestamp sweeping.

Adds test_typed_store_iterator_advance_reads_nothing, which counts opens through the RAM filesystem already used by the typed-store tests. It asserts three things: stepping over 8 records without dereferencing does 0 opens, the values are still correct when a caller does dereference, and dereferencing twice at one position reads once. Reverting only the TypedStore.h change and re-running it fails as it should:

test_typed_store_iterator_advance_reads_nothing: Expected 0 Was 8   [FAILED]

Also built and run on hardware: LilyGO T-Beam (ESP32) with the store on LittleFS, through microReticulum's path table, as part of a firmware that walks the table continuously. No behavioural change and no regression — the node's path list and count agree and the walk completes normally.

Not tested: nRF52 / RAK4631 and the ttgo-lora32-v21 target (no hardware to hand), and the FAT/SD and FlashFileSystem backends. The change is backend-agnostic — it moves when a read happens and removes reads nobody asked for; it does not alter what is read or written.

Closes #9

TypedStore's iterator decoded the record in its constructor and again on
every ++, so advancing read it off the filesystem whether the caller
wanted it or not. BasicFileStore is lazy on purpose — it reads only on
operator*, and documents operator-> as metadata-only for that reason —
and the typed wrapper took that back one layer up.

The cost is paid by anything that does not dereference every entry:
counting, searching for a key, or stopping early all read the whole
prefix, and reaching position N always costs N reads. That last one
rules out bounded or resumable traversal, because a cursor cannot skip
to where it left off more cheaply than re-reading everything before it.

Decoding now happens in operator*, guarded so the end iterator is still
never dereferenced, with a flag so asking twice at one position reads
once. A full walk that dereferences every entry costs exactly what it
did before.

Adds test_typed_store_iterator_advance_reads_nothing, which counts opens
through the RAM filesystem the typed-store tests already use: stepping
over 8 records without dereferencing did 8 opens before this change and
does 0 after.
@attermann

Copy link
Copy Markdown
Owner

Thanks for this, and for the detailed write-up in #9. The watchdog case makes the cost very clear, and the open-counting test is a good way to pin the fix down.

Two remarks:

  1. The new test ended up between test_typed_store_vector_value and its comment ("round-trips raw binary values"), so that comment now sits above the wrong function.
  2. The doc docs/microstore_user_guide.md (around line 281) says the TypedStore iterator decodes "on each step". Updating to say it decodes on dereference would keep the docs in line with this change.

I don't have permission to edit your PR so I'll make these changes as a follow-up commit.
Thanks again.

@attermann
attermann merged commit f024cc4 into attermann:master Nov 10, 2026
attermann added a commit that referenced this pull request Nov 10, 2026
…docs

Move the "round-trips raw binary values" comment back above
test_typed_store_vector_value, where it was before the new
test_typed_store_iterator_advance_reads_nothing was inserted.

Update the user guide to say the TypedStore iterator decodes on
dereference rather than on each step, matching the lazy-load
behavior introduced in #10.
@mclueppers
mclueppers deleted the fix/typed-store-iterator-loads-eagerly branch November 10, 2026 20:00
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.

Bug: TypedStore's iterator reads the record on every step, so advancing costs as much as dereferencing

2 participants