Skip to content

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

Open
mclueppers wants to merge 1 commit into
attermann:masterfrom
dobrevit:fix/typed-store-iterator-loads-eagerly
Open

Load the record on dereference, not on every step#10
mclueppers wants to merge 1 commit into
attermann:masterfrom
dobrevit:fix/typed-store-iterator-loads-eagerly

Conversation

@mclueppers

@mclueppers mclueppers commented Sep 6, 2026

Copy link
Copy Markdown

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.
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

1 participant