- LFS3_T_COMPACT -> LFS3_T_COMPACTMETA
- gc_compact_thresh -> gc_compactmeta_thresh
And friends:
LFS3_M_COMPACTMETA 0x00000800 Compact metadata logs
LFS3_GC_COMPACTMETA 0x00000800 Compact metadata logs
LFS3_I_COMPACTMETA 0x00000800 Filesystem may have uncompacted metadata
LFS3_T_COMPACTMETA 0x00000800 Compact metadata logs
---
This does two things:
1. Highlights that LFS3_T_COMPACTMETA only interacts with metadata logs,
and has no effect on data blocks.
2. Better matches the verb+noun names used for other gc/traversal flags
(REPOPGBMAP, CKMETA, etc).
It is a bit more of a mouthful, but I'm not sure that's entirely a bad
thing. These are pretty low-level flags.
And friends:
LFS3_M_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_GC_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_I_REPOPLOOKAHEAD 0x00000200 Lookahead buffer is not full
LFS3_T_REPOPLOOKAHEAD 0x00000200 Repopulate lookahead buffer
To match LFS3_T_REPOPGBMAP, which is more-or-less the same operation.
Though this does turn into quite the mouthful...
There's a strong argument for naming this inline_size as that's more
likely what users expect, but shrub_size is just the more correct name
and avoids confusion around having multiple names for the same thing.
It also highlights that shrubs in littlefs3 are a bit different than
inline files in littlefs2, and that this config also affects large files
with a shrubbed root.
May rerevert this in the future, but probably only if there is
significant user confusion.
And tweaked a few related comments.
I'm still on the fence with this name, I don't think it's great, but it
at least betters describes the "repopulation" operation than
"rebuilding". The important distinction is that we don't throw away
information. Bad/erased block info (future) is still carried over into
the new gbmap snapshot, and persists unless you explicitly call
rmgbmap + mkgbmap.
So, adopting gbmap_repop_thresh for now to see if it's just a habit
thing, but may adopt a different name in the future.
As a plus, gbmap_repop_thresh is two characters shorter.
A big downside of LFS3_T_REBUILDGBMAP is the addition of an lfs3_btree_t
struct to _every_ traversal object.
Unfortunately, I don't see a way around this. We need to track the new
gbmap snapshot _somewhere_, and other options (such as a global gbmap.b_
snapshot) just move the RAM around without actually saving anything.
To at least mitigate this internally, this splits lfs3_trv_t into
distinct lfs3_trv_t, lfs3_mgc_t, and lfs3_mtrv_t structs that capture
only the relevant state for internal traversal layers:
- lfs3_mtree_traverse <- lfs3_mtrv_t
- lfs3_mtree_gc <- lfs3_mgc_t (contains lfs3_mtrv_t)
- lfs3_trv_read <- lfs3_trv_t (contains lfs3_mgc_t)
This minimizes the impact of the gbmap rebuild snapshots, and saves a
big chunk of RAM. As a plus it also saves RAM in the default build by
limiting the 2-block block queue to the high-level lfs3_trv_read API:
code stack ctx
before: 37176 2360 684
after: 37176 (+0.0%) 2352 (-0.3%) 684 (+0.0%)
code stack ctx
gbmap before: 40060 2432 848
gbmap after: 40024 (-0.1%) 2368 (-2.6%) 848 (+0.0%)
The main downside? Our field names are continuing in their
ridiculousness:
lfs3.gc.gc.t.b.h.flags // where else would the global gc flags be?
- lfs3_gbmap_set* -> lfs3_gbmap_mark*
- lfs3_alloc_markfree -> lfs3_alloc_adopt
- lfs3_alloc_mark* -> lfs3_alloc_markinuse*
Mainly for consistency, since the gbmap and lookahead buffer are more or
less the same algorithm, ignoring nuances (lookahead only ors inuse
bits, gbmap rebuilding can result in multiple snapshots, etc).
The rename lfs3_gbmap_set* -> lfs3_gbmap_mark* also makes space for
lfs3_gbmap_set* to be used for range assignments with a payload, which
may be useful for erased ranges (gbmap tracked ecksums?)
This adds LFS3_T_REBUILDGBMAP and friends, and enables incremental gbmap
rebuilds as a part of gc/traversal work:
LFS3_M_REBUILDGBMAP 0x00000400 Rebuild the gbmap
LFS3_GC_REBUILDGBMAP 0x00000400 Rebuild the gbmap
LFS3_I_REBUILDGBMAP 0x00000400 The gbmap is not full
LFS3_T_REBUILDGBMAP 0x00000400 Rebuild the gbmap
On paper, this is more or less identical to repopulating the lookahead
buffer -- traverse the filesystem, mark blocks as in-use, adopt the new
gbmap/lookahead buffer on success -- but a couple nuances make
rebuilding the gbmap a bit trickier:
- Unlike the lookahead buffer, which eagerly zeros in allocation, we
need an explicit zeroing pass before we start marking blocks as
in-use. This means multiple traversals can potentially conflict with
each other, risking the adoption of a clobbered gbmap.
- The gbmap, which stores information on disk, relies on block
allocation and the temporary "in-flight window" defined by allocator
ckpoints to avoid circular block states during gbmap rebuilds. This
makes gbmap rebuilds sensitive to allocator ckpoints, which we
consider more-or-less a noop in other parts of the system.
Though now that I'm writing this, it might have been possible to
instead include gbmap rebuild snapshots in fs traversals... but that
would probably have been much more complicated.
- Rebuilding the gbmap requires writing to disk and is generally much
more expensive/destructive. We want to avoid trying to rebuild the
gbmap when it's not possible to actually make progress.
On top of this, the current trv-clobber system is a delicate,
error-prone mess.
---
To simplify everything related to gbmap rebuilds, I added a new
internal traversal flag: LFS3_t_CKPOINTED:
LFS3_t_CKPOINTED 0x04000000 Filesystem ckpointed during traversal
LFS3_t_CKPOINTED is set, unconditionally, on all open traversals in
lfs3_alloc_ckpoint, and provides a simple, robust mechanism for checking
if _any_ allocator checkpoints have occured since a traversal was
started. Since lfs3_alloc_ckpoint is required before any block
allocation, this provides a strong guarantee that nothing funny happened
to any allocator state during a traversal.
This makes lfs3_alloc_ckpoint a bit less cheap, but the strong
guarantees that allocator state is unmodified during traversal are well
worth it.
This makes both lookahead and gbmap passes simpler, safer, and easier to
reason about.
I'd like to adopt something similar+stronger for LFs3_t_MUTATED, and
reduce this back to two flags, but that can be a future commit.
---
Unfortunately due to the potential for recursion, this ended up reusing
less logic between lfs3_alloc_rebuildgbmap and lfs3_mtree_gc than I had
hoped, but at like the main chunks (lfs3_alloc_remap,
lfs3_gbmap_setbptr, lfs3_alloc_adoptgbmap) could be split out into
common functions.
The result is a decent chunk of code and stack, but the value is high as
incremental gbmap rebuilds are the only option to reduce the latency
spikes introduced by the gbmap allocator (it's not significantly worse
than the lookahead buffer, but both do require traversing the entire
filesystem):
code stack ctx
before: 37164 2352 684
after: 37208 (+0.1%) 2360 (+0.3%) 684 (+0.0%)
code stack ctx
gbmap before: 39708 2376 848
gbmap after: 40100 (+1.0%) 2432 (+2.4%) 848 (+0.0%)
Note the gbmap build is now measured with LFS3_GBMAP=1, instead of
LFS3_YES_GBMAP=1 (maybe-gbmap) as before. This includes the cost of
mkgbmap, lfs3_f_isgbmap, etc.
And:
- Tweaked the behavior of gbmap.window/known to _not_ match disk.
gbmap.known matching disk is what required a separate
lookahead.bmapped in the first place, but we never use both fields.
- _Don't_ revert gbmap on failed mdir commits!
This was broken! If we reverted we risked inheriting outdated
in-flight block information.
This could be fixed by also zeroing lookahead.bmapped, but would force
a gbmap rebuild. And why? The only interaction between mdir commit and
the gbmap is block allocation, which is intentionally allowed to go
out-of-sync to relax issues like this.
Note we still revert in lfs3_fs_grow, the new gbmap we create there is
incompatible with the previous disk size.
As a part of these changes, gbmap.window now behaves roughly the same as
gbmap.known and updates eagerly on block allocation.
This makes lookahead.window and gbmap.window somewhat redundant, but
simplifies the relevant logic (especially due to how lookahead.window
lags behind lookahead.off).
---
A bunch of bugs fell out-of-this, the interactions with lfs3_fs_mkgbmap
and lfs3_fs_grow being especially tricky, but fortunately our testing is
doing a good job.
At least the code changes were minimal, saves a bit of RAM:
code stack ctx
no-gbmap before: 37168 2352 684
no-gbmap after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
maybe-gbmap before: 39688 2392 852
maybe-gbmap after: 39720 (+0.1%) 2376 (-0.7%) 848 (-0.5%)
code stack ctx
yes-gbmap before: 39156 2392 852
yes-gbmap after: 39208 (+0.1%) 2376 (-0.7%) 848 (-0.5%)
These two functions allow changing whether or not the gbmap is in use
after format:
// Enable the global on-disk block-map
//
// Returns a negative error code on failure. Does nothing if a gbmap
// already exists.
int lfs3_fs_mkgbmap(lfs3_t *lfs3);
// Disable the global on-disk block-map
//
// Returns a negative error code on failure. Does nothing if no gbmap
// is found.
int lfs3_fs_rmgbmap(lfs3_t *lfs3);
rmgbmap was easy enough, but implementing mkgbmap turned out to be
surprisingly tricky due to how gstate permeates the system:
- Even if we zero gstate when we removing the gbmap, mounting the
image on a driver that doesn't understand the gbmap results in garbage
gstate over time as mdir compacts drop unknown gdeltas.
I think this sort of implicit gdelta cleanup is a good thing, but the
possibility of garbage gstate is a bit annoying.
Example A: the dbg scripts are currently printing a bunch of warnings
for corrupt gstate that can be safely ignored.
To support recovering from garbage gstate in mkgbmap, I changed
lfs3_fs_commitgdelta to _always_ track p state even when disabled. We
already needed to do this in lfs3_fs_flush/consumegdelta anyways,
since we don't know if the gbmap is used until parsing wcompat flags.
- The commit that enables the gbmap is tricky. We need the gbmap enabled
to calculate the new gdelta, but we also need it disabled so we don't
traverse the existing gbmap_p (which may be garbage).
As a workaround I added gbmap.b_p, which is in theory redundant with
gbmap_p, but (1) avoids needing to decode gbmap_p during traversals,
and (2) allows the two to temporarily fall out-of-sync in mkgbmap.
This means we potentially have 5 (!) snaphots flying around when
rebuilding the gbmap, which is starting to get a bit silly. But this
was also motivated by gbmap_p decoding adding roughly the same amount
of RAM to lfs3_mtree_traverse_, so the total RAM usage should in
theory be roughly the same.
There might be a better solution, but this at least gets mkgbmap
working. The gbmap builds are not our most RAM senstive configurations
anyways.
---
Also added a couple more tests in test_gbmap to test these:
- test_gbmap_files
- test_gbmap_rmgbmap
- test_gbmap_mkgbmap
- test_gbmap_rmmkgbmap
- test_gbmap_mkrmgbmap
And an explicit wraparound test to test_alloc. This was loosely implied
by the nospc tests, but it's probably better to have an explicit test.
The only downside is this implementation is limited to files:
- test_alloc_wraparound_files
---
Note we are currently dealing with three different configurations:
no-gbmap (the default), yes-gbmap (LFS3_YES_GBMAP), and maybe-gbmap
(LFS3_GBMAP + LFS3_F_GBMAP at runtime).
It only makes sense to include these in maybe-gbmap mode, so this is the
only mode with a notable code increase. However these functions are
relatively cheap. The stack/ctx changes also affect yes-gbmap, but
should mostly cancel out, see above:
code stack ctx
no-gbmap before: 37168 2352 684
no-gbmap after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
maybe-gbmap before: 39292 2456 800
maybe-gbmap after: 39688 (+1.0%) 2392 (-2.6%) 852 (+6.5%)
code stack ctx
yes-gbmap before: 39116 2456 800
yes-gbmap after: 39156 (+0.1%) 2392 (-2.6%) 852 (+6.5%)
In lfs3_fs_grow, we need to update any gbmaps to match the new disk
size. The actual patch to the gbmap is easy, but it does get a bit
delicate since we need to feed the gbmap with an allocator in the new
disk size.
Fortunately, the opportunistism of the gbmap allocator avoids any
catch-22 issues, as long as we make sure to not trigger any gbmap
rebuilds.
Adds a bit of code, but not much:
code stack ctx
before: 37168 2352 684
after: 37168 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
gbmap before: 39000 2456 800
gbmap after: 39116 (+0.3%) 2456 (+0.0%) 800 (+0.0%)
Having gbmap/bmap used in different places for the same thing was
confusing. Preferring gbmap as it is consistent with other gstate (grm
queue, gcksums), even if it is a bit noisy.
It's interesting to note what didn't change:
- The BM* range tags: LFS3_TAG_BMFREE, etc. These already differs from
the GBMAP* prefix enough, and adopting GBM* would risk confusion for
actual gstate.
- The gbmap revdbg string: "bb~r". We don't have enough characters for
anything else!
- dbgbmap.py/dbgbmapsvg.py. These aren't actually related to the gbmap,
so the name difference is a good thing.
TLDR: This drops the idea of different bmap strategies/modes, and sorts
out most of the compile-time/runtime conditional bmap interactions.
---
Motivation: Benchmarking (at least up to the 32-bit word limit) has
shown the bmap will unlikely be a significant bottleneck, even on large
disks. The largest disks tend to be NAND, and NAND's ridiculous block
size limits pressure on block allocation.
There are still concerns for areas I haven't measured yet:
- SD/eMMC/FTL - Small blocks, so more pressure on block allocation. In
theory the logical block size can be artificially increased, but this
comes with a granularity tradeoff.
- I've only measured throughput, latency is a whole other story.
However, users have reported lfs3_fs_gc is useful for mitigating this,
so maybe latency is less of a concern now?
But while there may still be room for improvement via alternative bmap
strategies, the risk a concerning amount of complexity. Yes,
configuration gets more complicated, but the real issue is any bmap
strategies that try to track _deallocations_ (the original idea being
treediffing) risk falling leaking blocks if all cases aren't covered.
The current "bmap cache" strategy strikes a really nice balance where it
reduces _amortized_ block allocation -> ~O(log n) without RAM, while
retaining the safe, bug-resistant, single-source-of-truth properties
that come with lookahead-based allocation.
---
So, long story short, dropping other strategies, and now the presence of
the bmap is a boolean flag.
This is also the first format-specific flag:
- Define LFS3_BMAP to enable the bmap logic, but note by default the
bmap will still not be used.
- Define LFS3_YES_BMAP to force the bmap to be used.
- With LFS3_BMAP, passing LFS3_F_GBMAP to lfs3_format will include the
on-disk block-map.
- No flag is needed during mount, the presence of the bmap is determined
by the on-disk wcompat flags (LFS3_WCOMPAT_GBMAP). This also prevents
rw mounting if the bmap is not supported, but rdonly mounting is
allowed.
- Users can check if the bmap is in use via lfs3_fs_stat, which reports
LFS3_I_GBMAP in the flags field.
There's still some missing pieces, but these will be a bit more
involved:
- lfs3_fs_grow needs to be made bmap aware!
- We probably want something like lfs3_fs_mkgbmap and lfs3_fs_rmgbmap to
allow converting between bmap backed/not-backed filesystem images.
Code changes minimal:
code stack ctx
before: 37172 2352 684
after: 37172 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38844 2456 800
bmap after: 38852 (+0.0%) 2456 (+0.0%) 800 (+0.0%)
New bmap range tags:
LFS3_TAG_BMRANGE 0x033u v--- --11 --11 uuuu
LFS3_TAG_BMFREE 0x0330 v--- --11 --11 ----
LFS3_TAG_BMINUSE 0x0331 v--- --11 --11 ---1
LFS3_TAG_BMERASED 0x0332 v--- --11 --11 --1-
LFS3_TAG_BMBAD 0x0333 v--- --11 --11 --11
Note 0x334-0x33f are still reserved for future bmap tags, but the new
encoding fits in the surprisingly common 2-bit subfield that may
deduplicate some decoding code.
Fitting in 2-bits is the main reason for this, now that in-flight ranges
look like they won't be worth exploring further. Worst case we can
always add more bm tags in the future. And it may even make sense to use
an entire bit for in-flight tags, since in theory the concept can apply
to more than just in-use blocks.
---
Another benefit of this encoding: In-use vs free is a bit check, and I
like the implication that an in-use + erased block can only be a bad
block.
No code changes:
code stack ctx
before: 37172 2352 684
after: 37172 (+0.0%) 2352 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38844 2456 800
bmap after: 38844 (+0.0%) 2456 (+0.0%) 800 (+0.0%)
Now that lfs3_alloc_ckpoint is more complicated, and can error, it makes
sense for lfs3_alloc_ckpoint to be implied by lfs3_mdir_commit.
Most lfs3_mdir_commit calls represent an atomic transaction from one
state -> another, so this saves a bit of code:
code stack ctx
before: 36912 2368 684
after: 36836 (-0.2%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38512 2400 812
bmap after: 38436 (-0.2%) 2400 (+0.0%) 812 (+0.0%)
The notable exception being bshrub-related commits in
lfs3_bshrub_commitroot_. Bshrub commits are trying to resolve an
in-flight btree, so the relevant blocks are very much _not_ at rest.
---
I've been hesitant to adopt this mostly just because it makes the
lfs3_mdir_commit* names even more of a mess:
- lfs3_mdir_commit__ -> lfs3_mdir_commit___
- lfs3_mdir_commit_ -> lfs3_mdir_commit__
- lfs3_mdir_commit -> lfs3_mdir_commit_
- added lfs3_mdir_commit
- lfs3_mdir_compact -> lfs3_mdir_compact_
- add lfs3_mdir_compact
- lfs3_mdir_alloc__ -> lfs3_mdir_alloc___
- lfs3_mdir_estimate__ -> lfs3_mdir_estimate___
- lfs3_mdir_swap__ -> lfs3_mdir_swap___
The main change is error propagation in lfs3_alloc_ckpoint. Since
lfs3_alloc_ckpoint writes to disk during bmap rebuilds, it can now fail
in all sorts of ways. Fortunately lfs3_alloc_ckpoint should only ever be
called by write operations, where these errors are be expected.
With bmap rebuild errors now reported correctly, this unblocks most of
the remaining test TODOs:
- Passing test_badblocks
- Passing test_ck
- Passing test_trvs
With this, LFS3_YES_BMAP is now passing all but two tests, which are
still ifndef-disabled as a temporary measure:
- test_btree - We make some low-level assumptions about the lookahead
allocator when testing btrees. It's probably not worth trying to get
this passing with the bmap allocator.
- test_grow - This one does need fixing! We currently don't update
on-disk bmaps correctly when growing the filesystem.
Code changes minimal:
code stack ctx
before: 36912 2368 684
after: 36912 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38456 2400 812
bmap after: 38512 (+0.1%) 2400 (+0.0%) 812 (+0.0%)
We need to adjust mids to ignore orphans during dir traversal, but we
shouldn't also adjust the dir position. In theory it shouldn't matter if
we use adjusted/non-adjusted dir positions, but it becomes a problem if
intermediate writes cause those orphans to be cleaned up. Now all your
dir positions are wrong.
Not entirely sure why this only started to fail with the bmap. I'm
guessing it's just due to the additional gstate causing the mdirs to
split differently.
Code changes minimal:
code stack ctx
before: 36920 2368 684
after: 36912 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
Tangential, but toss this on the pile of problems with dir positions.
I'm increasingly convinced we should just remove the concept if we can
get away with it.
There's really no reason to immediately commit the bmap to disk, at
least no until the first mdir commit, when we need to at least discard
the previous bmap state.
We already do all the gstate handling in lfs3_mdir_commit anyways, and
piggybacking on mdir commit lets us get rid of the annoying extra mdir
param in lfs3_alloc_ckpoint.
This does mean a slightly higher risk of needing to re-rebuild the bmap
after a powerloss, but in theory only if the user does something weird
like writing to a file and never calling sync. Most on-disk operations
terminate in an mdir commit as that's how any state change becomes
atomically visibile in littlefs.
Saves a nice bit of stack:
code stack ctx
before: 36920 2368 684
after: 36920 (+0.0%) 2368 (+0.0%) 684 (+0.0%)
code stack ctx
bmap before: 38552 2472 812
bmap after: 38464 (-0.2%) 2400 (-2.9%) 812 (+0.0%)
At least at a proof-of-concept level, there's still a lot of cleanup
needed.
To make things work, lfs3_alloc_ckpoint now takes an mdir, which
provides the target for gbmap gstate updates.
When the bmap is close to empty (configurable via bmap_scan_thresh), we
opportunistically rebuild it during lfs3_alloc_ckpoints. The nice thing
about lfs3_alloc_ckpoint is we know the state of all in-flight blocks,
so rebuilding the bmap just requires traversing the filesystem + in-RAM
state.
We might still fall back to the lookahead buffer, but in theory a well
tuned bmap_scan_thresh can prevent this from becoming a bottleneck (at
the cost of more frequent bmap rebuilds).
---
This is also probably a good time to resume measuring code/ram costs,
though it's worth repeating the above note about the bmap work still
needing cleanup:
code stack ctx
before: 36840 2368 684
after: 36920 (+0.2%) 2368 (+0.0%) 684 (+0.0%)
Haha, no, the bmap isn't basically free, it's just an opt-in features.
With -DLFS3_YES_BMAP=1:
code stack ctx
no bmap: 36920 2368 684
yes bmap: 38552 (+4.4%) 2472 (+4.4%) 812 (+18.7%)
This doesn't fully replace the lookahead buffer, but at least augments
it with known bmap state when available.
To be honest, this is a minimal effort hack to try to get something
benchmarkable without dealing with all the catch-22 issues that a
self-support bmap allocator would encounter (allocating blocks for the
bmap requires a bmap, oh no).
Though now that I'm writing this, maybe this is a reasonable long-term
solution? Having the lookahead buffer to fall back on solves a lot of
problems, and, realistically, it's unlikely to be a performance
bottleneck unless the user has extreme write requests (>available
storage?).
---
Also tweaked field naming to be consistent between the bmap and
lookahead buffer.
This avoids issues with the different traversal paths with an mtree vs
inline-mtree. Previously this was broken when the mtree was inlined.
This order also makes more sense if we want to check mdirs before we
consider the gstate to be trustworthy enough for gbmap traversal.
The neat thing about the on-disk bmap is that it's a range tree. We can
leverage order-statistic properties to compactly represent ranges of
similar blocks.
However, this does make updating the bmap slightly more complicated...
This comes from an observation that we never actually use the leaf cache
during traversals, and there is surprisingly little risk of a lookup
creating a conflict in the future.
Btree traversal fall into two categories:
1. Full traversals, where we traverse a full btree all at once. These
are unlikely to have lookup conflicts because everything is
usually self-contained in one chunk of logic.
2. Incremental traversals. These _are_ at risk, but in our current
design limited to lfs3_trv_t, which already creates a fully
bshrub/btree copy for tracking purposes.
This copy unintentionally, but conveniently, protects against lookup
conflicts.
So, why not reuse the btree leaf cache to hold the rbyd state during
traversals? In theory this makes lfs3_btree_traverse the same cost and
lfs3_btree_lookupnext, drops the need for lfs3_btrv_t, and simplifies
the internal API.
The only extra bit of state we need is the current target bid, which is
now expected as a caller-incremented argument similar to
lfs3_btree_lookupnext iteration.
There was a bit of futzing around with bid=-1 being necessary to
initialize traversal (to avoid conflicts with bid=-1 => 0 caused by
empty btrees). But the end result is a btree traversal that only needs
one extra word of state.
---
Unfortunately, in practice, the savings were not as great as expected:
code stack ctx
before: 36792 2400 684
after: 36876 (+0.2%) 2384 (-0.7%) 684 (+0.0%)
This does claw back some stack, but less than a full rbyd due to the
union with the mtortoise in lfs3_trv_t. The mtortoise now dominates. It
might be possible to union the mtortoise and the bshrub/btree state
better (both are not needed at the same time), but strict aliasing rules
in C make this tricky.
The new lfs3_btree_traverse is also a bit more complicated in terms of
code cost. In theory this would be offset by the simpler traversal setup
logic, but we only actually call lfs3_btree_traverse twice:
1. In lfs3_mtree_traverse
2. In lfs3_file_ck
Still, some stack savings + a simpler internal API makes this worthwhile
for now. lfs3_trv_t is also due for a revisit, and hopefully it's
possible to better union things with btree leaf caches somehow.
This is an indulgence to simplify the upcoming auxiliary btree work.
Brings back the previously-reverted per-btree leaf caches, where each
lfs3_btree_t keeps track of two rbyds: The root and the most recently
accessed leaf.
At the surface level, this optimizes repeated access to the same btree
leaf. A common pattern for a number of littlefs's operations that has
proven tricky to manually optimize:
- Btree iteration
- Pokes for our crystalization heuristic
- Checksum collision resolution for dids and (FUTURE) ddkeys
- Related rattrs attached to a single bid
But the real motivation is to drop lfs3_btree_*lookupleaf and simplify
the internal APIs. If repeated lfs3_btree_lookup*s are already
efficient, there's no reason for extra leaf-level APIs, and in theory
any logic that interacts with btrees will be simpler.
---
This comes at a cost (humorously about the same amount as the
tag-returning refactor, if you ignore the extra 28 bytes of ctx).
Unsurprisingly, increasing the size of lfs3_btree_t has the biggest
impact on stack and ctx:
code stack ctx
before: 36084 2336 656
after: 36784 (+1.9%) 2400 (+2.7%) 684 (+4.3%)
Also note from the previous commit messages: Btree leaf caching has
resulted in surprisingly little performance improvement for our current
benchmarks + implementation. It turns out if you're dominated by write
cost, optimizing btree lookups -- which already skip rbyd fetches, has
barely noticeable impact.
---
A note on reverting!
Eventually (after the auxiliary btree work) it will probably make sense
to revert this -- or at least provide a non-leaf-caching build for
code/RAM sensitive users.
I don't think this should be reverted as-is. Instead, I think we should
allow the option to just disable the leaf cache, while keeping the
simpler internal API. This would give us the best of all three worlds:
- A small code/RAM option
- Optimal btree iteration/nearby-lookup performance
- Simpler internal APIs
The only reason this isn't already implemented is because I want to
avoid fragmenting the codebase further while we're still in development
mode.
Note this includes both the lfs3_config -> lfs3_cfg structs as well as
the LFS3_CONFIG -> LFS3_CFG include define:
- LFS3_CONFIG -> LFS3_CFG
- struct lfs3_config -> struct lfs3_cfg
- struct lfs3_file_config -> struct lfs3_file_cfg
- struct lfs3_*bd_config -> struct lfs3_*bd_cfg
- cfg -> cfg
We were already using cfg as the variable name everywhere. The fact that
these names were different was an inconsistency that should be fixed
since we're committing to an API break.
LFS3_CFG is already out-of-date from upstream, and there's plans for a
config rework, but I figured I'd go ahead and change it as well to lower
the chances it gets overlooked.
---
Note this does _not_ affect LFS3_TAG_CONFIG. Having the on-disk vs
driver-level config take slightly different names is not a bad thing.
- test_traversal -> test_trvs
- lfs3_traversal_t -> lfs3_trv_t
- lfs3_btraversal_t -> lfs3_btrv_t
- t -> trv
- bt -> btrv
- lfs3_traversal_* -> lfs3_trv_*
- lfs3_btraversal_* -> lfs3_btrv_*
The traversal type is becoming one of the more fundamental types in
littlefs, and if DIR and REG both get shortened names, it makes sense
for TRV to have one as well.
This also removes the temptation to use t for traversals, which is
probably an even worse name.
---
Note that lfs3_btree_traverse, lfs3_mtree_traverse, etc, remain
unaffected. This may change in the future, but it's interesting to note
that verbs seem to need much less typing than nouns.
- lfs3_omdir_t -> lfs3_handle_t
- lfs3.omdirs -> lfs3.handles
- o -> h
- lfs3_omdir_* -> lfs3_handle_*
- lfs3_omdir_ismidopen -> lfs3_mid_isopen
From conversations with users, the term "handle" or "file handle" seems
to be the most common/easily understood term for the lfs3_file_t struct
itself. It makes sense to adopt this in our codebase.
I usually dislike inventing new names for things when prefixes can imply
a relationship (size -> ssize, cache -> rcache, shrub -> bshrub, etc),
but lfs3_omdirs_t was probably a bit much.
Last but not least, this adopts tag-returns in lfs3_mtree_pathlookup,
and indirectly in all of lfs3_mtree_pathlookup's callers (which is
almost every top-level filesystem function -- anything that needs to
look up a path).
At this level, the muxed tag/err type really shows its versatility. Take
the LFS3_ERR_NOENT and LFS3_TAG_ORPHAN tags/errs for example.
Conceptually, these take very different code paths, but after calling
lfs3_mtree_pathlookup, it's easy to switch on both as though they
represent the same file-not-found condition.
We have to be a bit more careful now to not confuse err and tag
variables in these functions, and `goto failed` is now a bit of a
landmine, but the end result is another nice chunk of code savings:
code stack ctx
before: 36216 2336 656
after: 36084 (-0.4%) 2336 (+0.0%) 656 (+0.0%)
---
I believe this finishes the tag-returning refactor, which means we can
take a step back and look at how effective tag/err muxing is as a code
size optimization:
code stack ctx
before tag-returns: 36828 2368 656
after tag-returns: 36084 (-2.0%) 2336 (-1.4%) 656 (+0.0%)
A free 744 bytes is not bad! Especially considering there's no real
downside to this.
The 32 bytes of stack savings is nice too, and suggests we had ~8
unnecessary tag out-pointers sitting on the stack hot-path.
- lfs3_mdir_namelookup
- lfs3_mtree_namelookup
These are interesting, because, unlike lfs3_rbyd_namelookup, we don't
care about how query mids compare with the found mid.
Adopting tag-returns does mean we no longer return the relevant tag
when the query mid is missing, but the fact that the tests are passing
means this is a non-issue.
Shaves off a bit more code:
code stack ctx
before: 36260 2336 656
after: 36216 (-0.1%) 2336 (+0.0%) 656 (+0.0%)
Maybe these should have been updated in lock-step with
lfs3_mtree_pathlookup, but lfs3_mtree_pathlookup is going to impact a
lot more code...
- lfs3_mtree_traverse_
- lfs3_mtree_traverse
- lfs3_mtree_gc
I like this one if only for the reduced API noise. All of these layers
need to inspect the tag to know what to do, moving the tag to the return
position means less mucking around with points in our core traversal
logic.
Shaves off a bit more code:
code stack ctx
before: 36348 2336 656
after: 36260 (-0.2%) 2336 (+0.0%) 656 (+0.0%)
- lfs3_mdir_lookupnext
- lfs3_mdir_lookup
Like btree lookups, mdir lookups are also tag-inspection heavy, so we
see some nice savings:
code stack ctx
before: 36520 2352 656
after: 36348 (-0.5%) 2336 (-0.7%) 656 (+0.0%)
lfs3_mdir_lookup also highlights how tag-returns help reduce API noise
around the tag mask bits. lfs3_mdir_lookup's tag out-pointer doesn't
really make sense with the default non-masked tags, and moving it to the
return position hides it aways a bit.
- lfs3_btree_lookupleaf
- lfs3_btree_lookupnext
- lfs3_btree_lookup
- lfs3_btree_traverse
- NOT lfs3_btree_namelookup
Looks like we're starting to claw back stack usage a bit. This makes
sense as the btree logic involves the most layers -- with out-pointers
it needs more temporary copies to inspect tags along the way:
code stack ctx
before: 36576 2376 656
after: 36520 (-0.2%) 2352 (-1.0%) 656 (+0.0%)
This is the start of a big refactor to try to move tag out-pointers into
the return position of functions, muxing with error codes via the
sign-bit when necessary.
So instead of:
lfs3_tag_t tag_;
lfs3_data_t data_;
int err = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&tag_, &data_);
if (err) {
return err;
}
We now do:
lfs3_data_t data_;
lfs3_stag_t tag_ = lfs3_rbyd_lookup(&lfs3, &rbyd, rid, tag,
&data_);
if (tag_ < 0) {
return tag_;
}
In theory, removing an out-pointer saves both code and stack, though it
will be interesting to actually see how much of an affect this has after
the dust has settled.
littlefs v2 used this technique heavily for its 32-bit tags, but we
never did a comparison with/without tags in the return position.
This is a big rewrite in the test code, so hopefully this ends up worth
it :)
Lots of regex.
Note this implicitly limits error codes to 16-bits, but supported error
codes are already a bit limited because we're using int everywhere
(instead of int32_t). If we need 32-bit error codes we can always add
another type to represent the mux in the future (lfs3_etag_t?).
---
So far the code savings look promising:
code stack ctx
before: 36828 2368 656
after: 36576 (-0.7%) 2376 (+0.3%) 656 (+0.0%)
Stack usage is a big disappointing, but hopefully that is just a
temporary cost due to the internal scaffolding between different API
types while the refactor is ongoing.
This replaces the `bool align` parameter that goes through all the prog
layers with an optional prog-aligned cksum stored in the lfs3_t struct.
Normally ignored, this prog-aligned cksum can be requested by setting
cksum=&lfs3->pcksum in any prog call.
Does this work? Yes. Is it a great solution? Ehhhh...
I've been tinkering with other solutions that avoid the `bool align`
parameter, but with no luck.
- `bool align`, or previously two cksum arguments, work, but create a
bit of a messy API. I'd like to find an alternative solution.
- Changing the cksum pointer to a richer lfs3_cksum_t struct with flags
also works, but would be an even messier API.
- Adding an lfs3_t side-channel, lfs3->pcache could include a pointer to
an optional prog-aligned cksum. But this would be the same/more cost
as just storing the pcksum in lfs3_t. And then we'd need to worry
about disentangling the cksum pointer on errors, etc.
- We could set a flag in lfs3->flags for alignment. This avoids the
extra 4 bytes of ctx, but still suffers from the risk of entangled
state on errors, etc.
- We could unconditionally calculate lfs3->pcksum. But then we'd be
calculating a lot of cksums we don't use (every metadata commit), and
still using the extra 4 bytes of ctx.
Lacking a good solution, using cksum=&lfs3->pcksum to indicate a
prog-aligned cksum is at least an ok solution.
I will happily change this if an alternative comes up in the future.
Another way of viewing this is that `&lfs3->pcksum` acts as a special
magic pointer value to tell the prog layers to calculate lfs3->pcksum.
A different non-NULL constant value could have worked just as well, but
those are a bit trickier to create in C.
---
Actually, there is a "better" cursed solution:
- Rely on pointer alignment to sneak a flag into the cksum pointer's
lower bits.
But, while clever, this is is outside of C's machine model and would
limit portability.
---
This trades 4 bytes of ctx for 58 bytes of code and simpler (debatable)
internal prog APIs:
code stack ctx
before: 36860 2384 652
after: 36832 (-0.1%) 2384 (+0.0%) 656 (+0.6%)
In theory this also saves stack in all the prog APIs, but none of prog
APIs end up on the stack hot-path. In our codebase the read APIs
dominate the stack thanks to block allocator traversals.
LFS3_CKDATACKSUMREADS is just too much.
The downside is it may not be clear how LFS3_CKDATACKSUMREADS interacts
with the future planned LFS3_CKREADS (LFS3_CKREADS implies
LFS3_CKDATACKSUMS + LFS3_CKMETAREDUND), but on the flip side you may
actually be able to type LFS3_CKDATACKSUMS on the first try.
Not sure how this was missed. The whole tradeoff of shrinking
rattr.count was that by default lfs3_rattr_t would take up less space,
but user-provided buffers would need an indirect lfs3_data_t to support
arbitrary buffer sizes.
This managed to scrape by with a 16-bit count (15-bit really), but
fortunately failed test_attrs_fattr_resync_receive with an 8-bit count.
And only barely! 256 is the smallest possible custom attr that
overflows.
I guess a point towards making internal limitation as tight as possible
to catch mistakes like these earlier.
---
Added test_attrs_setattr_big and test_attrs_fattr_big to catch this in
the future.
Note that while this added some code, stack is unaffected. This is
because custom attribute handling is off the hot-path, which is why the
lfs3_rattr_t -> lfs3_rattr_t+lfs3_data_t split is worth it:
code stack ctx
before: 37016 2416 652
after: 37052 (+0.1%) 2416 (+0.0%) 652 (+0.0%)
This breaks down the previously 16-bit rattr.count field into two 8-bit
rattr.from and rattr.count fields. Now, instead of using a mixture of
rattr.tag and sign(rattr.count) to determine rattr encoding, we just
jump based on rattr.from:
lfs3_rattr_t:
.---+---+---+---.
| tag |frm|cnt| -+-> 16-bit tag - on-disk encoding + rbyd flags
+---+---+---+---+ +-> 8-bit from - in-RAM encoding
| weight | '-> 8-bit count - from-specific count
+---+---+---+---+
| ptr |
'---+---+---+---'
The internal appendrattr_ ctx also saw a bit of rework, and now uses a
big union with multiple buffers instead of stacking a ridiculous number
of LFS_MAX calls. Expanding the LFS_MAX stack grows O(n^2), so this is
probably good for compile times.
And all rattr.from branches now generate an lfs3_data_t*. This was
already a side-effect of all the internal lfs3_data_from* functions, and
it simplifies the tail end of appendrattr_. No more relying on
data_count's sign bit.
Also rearranged rattr.from encoders to match source code order.
---
Unfortunately, while this did simplify the source code, it didn't really
lead to much improvement in code size:
code stack ctx
before: 37024 2416 652
after: 37016 (-0.0%) 2416 (+0.0%) 652 (+0.0%)
I guess jump tables are more a performance optimization than a code size
one. That and the benefit of cheaper appendrattr_ logic is likely
overshadowed by the extra constants needed to populate rattr.from in
every LFS3_RATTR_* macro.
Also test_attrs_fattr_resync_receive is now failing, but I think that's
just because of an unrelated bug exposed by the shrinking count field.
In theory rattr.count should be limited to internal fixed-size buffers.
Limited to nested struct fields where the names don't really matter:
- bptr.data -> bptr.d
- mdir.rbyd -> mdir.r
Ok it actually just ended up those two.
This is on the tail end of some optimization work that ended up
abandoned because of maintainability concerns. But it did highlight that
struct nesting gets a bit out-of-control when trying to both optimize
stack allocations and respect C99's strict aliasing.
Consider further fragmenting lfs3_rbyd_t for fine-grain stack
allocations:
typedef struct lfs3_rbyd {
struct lfs3_rtrunkcksum {
struct lfs3_rtrunk {
lfs3_rid_t weight;
struct lfs3_rtrunktrunk {
lfs3_block_t blocks[2];
lfs3_size_t trunk;
} rtrunktrunk;
} rtrunk;
uint32_t cksum;
} rtrunkcksum;
lfs3_size_t eoff;
} lfs3_rbyd_t;
Accessing fields just starts to get silly:
rbyd.rtrunkcksum.rtrunk.trunktrunk.trunk
At least single-char field names keeps a little bit of readability:
rbyd.ck.t.t.trunk
Or for some real examples:
- file->b.o.mdir.rbyd.weight -> file->b.o.mdir.r.weight
- bptr->data.u.disk.block -> bptr->d.u.disk.block
So we now keep blocks around until they can be replaced with a single
fragment. This is simpler, cheaper, and reduces the number of commits
needed to graft (though note arbitrary range removals still keep this
unbounded).
---
So, this is a delicate tradeoff.
On one hand, not fully fragmenting blocks risks keeping around bptrs
containing very little data, depending on fragment_size.
On the other hand:
- It's expensive, and disk utilization during random _deletes_ is not
the biggest of concerns.
Note our crystallization algorithm should still clean up partial
blocks _eventually_, so this doesn't really impact random writes.
The main concerns are lfs3_file_truncate/fruncate, and in the future
collapserange/punchhole.
- Fragmenting bptrs introduces more commits, which have their own
prog/erase cost, and it's unclear how this impacts logging operations.
There's no point in fragmenting blocks at the head of a log if we're
going to fruncate them eventually.
I figure lets err on minimizing complexity/code size for now, and if
this turns out to be a mistake, we can always revert or introduce
fragmenting >1 fragment blocks as an optional feature in the future.
---
Saves a big chunk of code, stack, and even some ctx (no more
fragment_thresh):
code stack ctx
before: 37504 2448 656
after: 37024 (-1.3%) 2416 (-1.3%) 652 (-0.6%)
Not sure why we weren't already, it doesn't really make sense to return
bid without weight, and this matches lfs3_btree/bshrub_lookupnext.
Sure we don't need weight currently, but this is useful to include in
case we need it in the future (lfs3_bptr_fetch during traversal?).
And while we're not using it, the compiler is happy to optimize it out,
so no code changes:
code stack ctx
before: 37964 2424 636
after 37964 (+0.0%) 2424 (+0.0%) 636 (+0.0%)
Like the bshrub/btree dedup, this add lfs3_bptr_fetch to help dedup
bptr/data fetching.
The original plan was to eliminate bptrs from lfs3_file_lookupnext and
lfs3_file_traverse, and just return tagged data like the other
lookup/traverse functions. But this didn't work out very well. We return
arbitrary attrs from lfs3_file_traverse, so all this would've
accomplished is making every lfs3_file_lookupnext call messier.
But I think I'm still going to keep lfs3_bptr_fetch around as it
provides a nice place to deduplicate some other bits of logic:
- It makes sense to limit bptrs to compressed weights here, as opposed
to the somewhat arbitrary lfs3_file_lookupnext function.
- And it would be a bit silly to not put the bptr's LFS3_CKFETCHES logic
in lfs3_bptr_fetch.
This may fetch more than previously (during crystallization pokes?),
but better safe than sorry. LFS3_CKFETCHES will likely be a relatively
niche feature anyways.
As for lfs3_file_traverse, I got rid of it completely.
We already have special logic in lfs3_mtree_traverse_ and lfs3_file_ck
for bptrs anyways, since bptrs, unlike data fragments, reference actual
blocks. And this disentangles lfs3_mtree_traverse_ from the file APIs,
which was a bit of an awkward design.
---
This adds a bit of code to the default build, but I think it's worth it
for the better code organization:
code stack ctx
before: 37896 2424 636
after: 37964 (+0.2%) 2424 (+0.0%) 636 (+0.0%)
It also saves some code in LFS3_CKFETCHES mode, thanks to deduping all
the fetch ckfetches fetch checkhes:
code stack ctx
ckfetches before: 38144 2464 636
ckfetches after: 38072 (-0.2%) 2472 (+0.3%) 636 (+0.0%)
One of the ideas behind the key-value API is that it is potentially much
cheaper than a full file API. With the key-value API, we get the
guarantee that all data must fit in RAM, and avoid headaches like
random reads/writes and needing to broadcast file state.
For an example of just how much complexity is avoided, the see the
difference between lfs3_file_flushonce_ vs the mess that is
lfs3_file_flush_ + lfs3_file_crystallize + lfs3_file_graft.
However, littlefs is designed around files, and a couple design
decisions hold back how much code saving is possible:
1. littlefs's shrubs are designed around being enrolled in the omdir
linked-list, so internally we still have most of the file open/close
code lumbering around.
2. Directories and traversals still exist, so we'd need the omdir
linked-list anyways, and we still need to broadcast _some_ changes.
3. Despite being intended for small amounts of data, lfs3_set/get can
still be used to create arbitrarily large files. So we still need all
of the bshrub/btree logic.
Which we still need for the mtree anyways, so this isn't really that
much of a downside.
It also may be possible to save more code by aggressively rewriting the
_entire_ read/write path for lfs3_set/get, to not reuse any of the
existing file logic in LFS3_KVONLY mode. But I decided against this due
to concerns around maintainability.
The duplicate lfs3_file_read + lfs3_file_readonce and lfs3_file_flush_ +
lfs3_file_flushonce_ are already enough of a concern.
Anyways, here's LFS3_KVONLY:
code stack ctx
default: 37824 2416 636
kvonly: 30936 (-18.2%) 2168 (-10.3%) 636 (+0.0%)
LFS3_RDONLY + LFS3_KVONLY is also interesting:
code stack ctx
rdonly: 10776 856 508
rdonly+kvonly: 9904 (-8.1%) 888 (+3.7%) 508 (+0.0%)
---
This also added some noise to the default build's code, mainly due to
tweaks in lfs3_file_readnext to allow better reuse in LFS3_KVONLY:
code stack ctx
before: 37824 2416 636
after: 37860 (+0.1%) 2416 (+0.0%) 636 (+0.0%)
These are high-risk corner cases for the key-value API, so we should
test them.
At one point I was relying on an optional buffer parameter in
lfs3_file_sync_, but that would have broken if lfs3_set's buffer was
NULL.
This adds a couple functions that treat files as simple key-value pairs:
- lfs3_get - Read a file
- lfs3_size - Get the size of a file
- lfs3_set - Write a file
- lfs3_remove - Remove a file (this one already exists!)
The idea is the only real difference between a filesystem and key-value
store in the microcontroller space is the API, and the key-value API
_is_ much easier to use.
It also opens the door to making the file API opt-out in the future to
trade code cost for feature set. littlefs will probably never be
competitive with other microcontroller-scale key-value stores, but it
may be interesting for systems already using littlefs for other storage.
And don't worry, these are still files, so they can always be opened
with the full file API when more advanced operations are needed.
These APIs also matches the custom attribute APIs, which makes sense
because they're both key-values. Any mismatch should be considered an
API bug, because the best user interface is a consistent one.
This new API is tested in tests/test_kv.toml.
---
At the moment the implementation is naive, just sitting on top of the
file API. This works remarkably well thanks to littlefs's cache
bypassing logic, but does have some downsides:
- lfs3_set always writes two commits: one for the stickynote and one for
the file sync.
Unfortunately this is a fundamental limitation of littlefs's file API.
One nice benefit of lfs3_set is in theory we can bypass this
limitation, but not if we just sit on top of the file API.
- There may be code savings from more tightly integrating the key-value
code.
This also highlighted an awkward corner case with per-file cache
configuration in which the buffer needs to be non-null even if zero. Not
the end of the world, but just a bit awkward. Maybe this deserves
revisiting in the config API rework?
---
Code changes were relatively minimal given that this is a whole new API,
unfortunately the stack took quite a hit:
code stack ctx
before: 37352 2280 636
after: 37644 (+0.8%) 2448 (+7.4%) 636 (+0.0%)
The stack surprised me, but in hindsight it makes sense. In sitting on
top of the reset of the codebase, the key-value API adds very little
code, but every stack allocation in these functions add to the stack
hot-path.
This isn't the end of the world, and it's actually probably a good thing
to have an lfs3_file_t allocated in the stack hot-path. lfs3_file_t's
size has been a bit difficult to track thanks to struct lfs3_info
dominating ctx measurements...
These mimic the relevant LFS_O_* flags, and allow users to assert
whether or not a traversal will mutate the filesystem:
LFS_T_MODE 0x00000001 The traversal's access mode
LFS_T_RDWR 0x00000000 Open traversal as read and write
LFS_T_RDONLY 0x00000001 Open traversal as read only
In theory, these could also change internal allocations, but littlefs
doesn't really work that way.
Note we _don't_ add related LFS_GC_RDONLY, LFS_GC_RDWR, etc flags. These
are sort of implied by the relevant LFS_M_* flags.
Adds a bit more code, probably because of the slightly more complicated
internal constants for the internal traversals. But I think the
self-documentingness is worth it:
code stack ctx
before: 37200 2288 636
after: 37220 (+0.1%) 2288 (+0.0%) 636 (+0.0%)
- LFS_CKPARITY -> LFS_CKMETAPARITY
- LFS_CKDATACKSUMS -> LFS_CKDATACKSUMREADS
The goal here is to provide hints for 1. what is being checked (META,
DATA, etc), and 2. on what operation (FETCHES, PROGS, READS, etc).
Note that LFS_CKDATACKSUMREADS is intended to eventually be a part of a
set of flags that can pull off closed fully-checked reads:
- LFS_CKMETAREDUNDREADS - Check data checksums on reads
- LFS_CKDATACKSUMREADS - Check metadata redund blocks on reads
- LFS_CKREADS - LFS_CKMETAREDUNDREADS + LFS_CKDATACKSUMREADS
Also it's probably not a bad idea for LFS_CKMETAPARITY to be harder to
use. It's really not worth enabling unless you understand its
limitations (<1 bit of error detection, yay).
No code changes.
See the relevant commit for why. These just added surprisingly little
performance benefit for the code/stack cost.
Maybe in a future performance-preferring littlefs driver.
The idea here, is we give each lfsr_btree_t an optional leaf rbyd, in
addition to the root rbyd. This leaf rbyd acts as a cache for the most
recent leaf, allowing nearby btree lookups to skip the full btree walk.
Unfortunately, this failed on pretty much every measurable metric...
---
The motivation for this is that we often do a bunch of nearby btree
lookups:
- Btree iteration via lfsr_btree_lookupnext is a bit naive, walking from
the root every step.
- Our crystallization algorithm requires a bunch of nearby lookups to
figure out our crystallization heuristic. Currently at most 4, when
you need to lookup both crystal neighbors and then _also_ both
fragment neighbors for coalescing.
- Checksum collision resolution for dids and (FUTURE) ddkeys can require
an unbounded number of sequential lookups.
Though to be fair, this is an exceptional case if our checksum is any
good.
- Bids with multiple rattrs require nearby lookups to resolve.
Though currently this can be explicitly avoided via
lfsr_btree_lookupleaf + lfsr_rbyd_lookup.
The theory was that cases like these could explicitly keep track of the
leaf rbyd to avoid full btree walks, but in practice this never really
worked out. Tracking if we're still in the relevant leaf rbyd just adds
too much logic/code cost.
But if this leaf tracking logic was implemented once in the btree
layer...
The other theoretical benefit was being able to move more rbyds off the
stack. Sure our btrees take up more RAM, but if that results in stack
savings, that may be a win.
Oh, and this would let our btree API and rbyd API converge without
performance concerns. Internal users could in theory call
lfsr_btree_lookupnext + lfsr_btree_lookup with the same performance as
explicitly tracking the rbyd.
---
But this was a complete failure!
First the good news: There was a modest speedup of around ~2x to linear
reads.
And that's the good news.
Now the bad news:
1. There was no noticeable performance gain in any other benchmarks.
To be fair, we're at the early stages of benchmarking, so the
benchmarks may not be the most thorough, but thinking about it, there
are some explanations:
- In any benchmark that writes, fetch + erase + prog dominates. Being
able to skip fetches during lookups makes our btree lookups
surprisingly cheap!
- Any random read heavy benchmark is likely thrashing this cache,
which is to be expected.
- For small 1-block btrees, the leaf cache is useless because the
entire btree is cache in the root rbyd.
And keep in mind, our blocks are BIG. "Small" here could be on
the order of ~128KiB-1MiB for NAND flash.
- For the mtree, fetched mdirs actually already act as a sort of leaf
cache.
The extra btree leaf cache isn't doing _nothing_, but each layer of
the mtree has diminishing returns due to btree's ridiculous
branching factor.
- For file btrees, we're explicitly caching the leaf fragments/
blocks, so the extra btree leaf cache has diminishing returns for
the same reason.
2. Code cost was bad, stack cost was worse:
code stack ctx
before: 37172 2288 636
after: 38068 (+2.4%) 2416 (+5.6%) 664 (+4.4%)
Tracking the leaf required more code, that's expected. And, to be
fair, the current code has had a lot more time to congeal.
What wasn't expected was the stack cost.
Unfortunately these caches didn't really take any rbyds off the stack
hot-path:
- We _can_ get rid of the rbyd in lfsr_btree_lookup/namelookup, but
we were already hacking our way around the critical one in
lfsr_mtree_lookup/namelookup by reusing the mdir's rbyd!
- We can't even abuse the leaf rbyd in the commit logic, since the
target btree can end up iterated/traversed by lfs_alloc.
That was a fun bug.
And the addition of a second rbyd to lfsr_btree_t increases both ctx
and stack anywhere btrees are allocated.
Maybe this will make more sense when we add the auxiliary btrees, or
after more benchmarking, but for now the theoretical performance
improvements just aren't worth it.
Will probably revert this, but I wanted to commit it in case the idea is
worth resurrecting in the future, if in the future nearby btree lookups
are a bigger penalty than they are now.
Christopher Haster