Load the record on dereference, not on every step - #10
Open
mclueppers wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
TypedStore::iteratordecodes the record in its constructor and again inoperator++, andload()dereferences the underlying iterator to do it.BasicFileStoreis lazy on purpose — it reads only onoperator*, and documentsoperator->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 aloaded_flag: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 fromoperator*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:The three failures are pre-existing on master and unrelated —
test_file_store_compact_basic,test_dir_prefix_reload_indexandtest_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 theTypedStore.hchange and re-running it fails as it should: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
FlashFileSystembackends. 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