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 idea behind separate ctrled+unctrled airspaces was to try to avoid
multiple interpretations of the on-disk bmap, but I'm starting to think
this adds more complexity than it solves.
The main conflict is the meaning of "in-flight" blocks. When using the
"uncontrolled" bmap algorithm, in-flight blocks need to be
double-checked by traversing the filesystem. But in the "controlled"
bmap algorithm, blocks are only marked as "in-flight" while they are
truly in-flight (in-use in RAM, but not yet in use on disk).
Representing these both with the same "in-flight" state risks
incompatible algorithms misinterpreting the bmap across different
mounts.
In theory the separate airspaces solve this, but now all the algorithms
need to know how to convert the bmap from different modes, adding
complexity and code cost.
Well, in theory at least. I'm unsure separate airspaces actually solves
this due to subtleties between what "in-flight" means in the different
algorithms (note both in-use and free blocks are "in-flight" in the
unknown airspace!). It really depends on how the "controlled" algorithm
actually works, which isn't implemented/fully designed yet.
---
Long story short, due to a time crunch, I'm ripping this out for now and
just storing the current algorithm in the wcompat flags:
LFS3_WCOMPAT_GBMAP 0x00006000 Global block-map in use
LFS3_WCOMPAT_GBMAPNONE 0x00000000 Gbmap not in use
LFS3_WCOMPAT_GBMAPCACHE 0x00002000 Gbmap in cache mode
LFS3_WCOMPAT_GBMAPVFR 0x00004000 Gbmap in VFR mode
LFS3_WCOMPAT_GBMAPIFR 0x00006000 Gbmap in IFR mode
Note GBMAPVFR/IFR != BMAPSLOW/FAST! At least BMAPSLOW/FAST can share
bmap representations:
- GBMAPVFR => Uncontrolled airspace, i.e. in-flight blocks may or may
not be in use, need to traverse open files.
- GBMAPIFR => Controlled airspace, i.e. in-flight blocks are in use,
at least until powerloss, no traversal needed, but requires more bmap
writes.
- BMAPSLOW => Treediff by checking what blocks are in B but not in A,
and what blocks are in A but not in B, O(n^2), but minimizes bmap
updates.
Can be optimized with a bloom filter.
- BMAPFAST => Treediff by clearing all blocks in A, and then setting all
blocks in B, O(n), but also writes all blocks to the bmap twice even
on small changes.
Can be optimized with a sliding bitmap window (or a block hashtable,
though a bitmap converges to the same thing in both algorithms when
>=disk_size).
It will probably be worth unifying the bmap representation later (the
more algorithm-specific flags there are, the harder interop becomes for
users, but for now this opens a path to implementing/experimenting with
bmap algorithms without dealing with this headache.
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 only works immediately after format, and only for one pass of the
disk, but it's a good way to test bmap lookups/allocation without
worrying about more complicated filesystem-wide interactions.
Except for niche file snapshotting, most btree updates until this point
are probably linear, i.e. a successful commit replaces any internal
rbyd state that has been claimed. In this model it makes sense to mark
claimed rbyd as "invalid", since failure to replace the claimed state
indicates something went wrong during the commit.
But this isn't necessarily true when snapshotting, since we don't
replace the state of claimed snapshots.
But wait, shouldn't snapshotted rbyds become readonly? Not necessarily!
Rbyds can have multiple trunks with unrelated (or in this case, shared)
histories, so there's nothing wrong with refetching an rbyd and
continuing to commit after another snapshot commits to tbe block.
Eventually both snapshots will need to compact and diverge into two
blocks, but until then sharing an rbyd makes the most of available
erased state.
At least in theory, experience will show us how well this works.
---
Also note this is not true for mdirs. We view mdirs as atomic and always
up-to-date, so snapshotting doesn't really make sense.
Fortunately the btree traversal logic is pretty reusable, so this just
required an additional tstate (LFS3_TSTATE_BMAP).
This raises an interesting question: _when_ do we traverse the bmap? We
need to wait until at least mtree traversal completes for gstate to be
reconstructed during lfs3_mount, but I think traversing before file
btrees makes sense.
This abandons the data-backed cache idea due to concerns around
readability and maintainability. Mixing const/mutable buffers in
lfs3_data_t was not great.
Instead, we now just allocate an indirect lfs3_data_t on the stack in
lfs3_file_sync_ to avoid the previous undefined behavior.
This actually results in less stack usage total, due to lfs3_file_t
allocations in lfs3_set/read, and avoid the more long-term memory cost
in lfs3_file_t:
code stack ctx
before: 36832 2376 684
after: 36840 (+0.0%) 2368 (-0.3%) 684 (+0.0%)
Oh. And lfs3_file_sync_ isn't even on the stack hot-path, so this is a
net benefit over the previous cache -> data cast:
code stack ctx
before sa: 36844 2368 684
after sa: 36840 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
Still less cool though.
This fixes a strict aliasing violation in lfs3_file_sync_, where we cast
the file cache -> lfs3_data_t to avoid an extra stack allocation, by
modifying the file's cache struct to use an lfs3_data_t directly.
- file.cache.pos -> file.cache.pos
- file.cache.buffer -> file.cache.d.u.buffer_
- file.cache.size -> file.cache.d.size
- (const lfs3_data_t*)&file->cache -> &file->cache.d
Note the underscore_ in file.cache.d.u.buffer_. This did not fit
together as well as I had hoped, due to different const expectation
between the file cache and lfs3_data_t.
Up until this point lfs3_data_t has only been used to refer to const
data (ignoring side-band pointer casting in lfs3_mtree_traverse*), while
the file cache very much contains mutable data. To work around this I
added data.u.buffer_ as a mutable variant, which works, but risks an
accidental const violation in the future.
---
Unfortunately this does come with a minor RAM cost, since we no longer
hide file.cache.pos in lfs3_data_t's buffer padding:
code stack ctx
before: 36844 2368 684
after: 36832 (-0.0%) 2376 (+0.3%) 684 (+0.0%)
lfs3_file_t before: 164
lfs3_file_t after: 168 (+2.4%)
I think it's pretty fair to call C's strict aliasing rules a real wet
blanket. It would be interesting to create a -fno-strict-aliasing
variant of littlefs in the future, to see how much code/RAM could be
saved if we were given free reign to abuse the available memory.
Probably not enough to justify the extra work, but it would be an
interesting experiment.
I think modern C simply doesn't let us do what we want to do here, so
I'm giving up, discarding the lfs3_mtortoise_t type, and just abusing
various unrelated shrub fields to implement the tortoise. This
sacrifices readability, but at least avoids undefined behavior without
a RAM penalty:
- shrub.blocks => tortoise blocks
- shrub.weight => cycle distance
- shrub.eoff => power-of-two bound
Note this keeps trunk=0, which is a nice safety net in case some code
ever tries to read from the shrub in the future.
Fortunately the mtortoise logic is fairly self-contained in
lfs3_mtree_traverse_, so with enough comments hopefully the code is not
too confusing.
---
Apparently shaves off a couple more bytes of code. I'm guessing this is
just because of the slightly different struct offsets (we're reusing the
root's rbyd instead of the leaf's rbyd now):
code stack ctx
before: 36852 2368 684
after: 36844 (-0.0%) 2368 (+0.0%) 684 (+0.0%)
This forces our cycle detection tortoise (previously trv.u.mtortoise),
into the unused shrub leaf via pointer shenanigans.
This reclaims the remaining stack (and apparently code) we theoretically
gained from the btree traversal rework, up until the compiler got in the
way:
code stack ctx
before: 36876 2384 684
after: 36852 (-0.1%) 2368 (-0.7%) 684 (+0.0%)
And it only required some _questionably_ defined behavior.
---
It's probably not well-defined behavior, but trying to understand what
the standard actually means on this is giving me a headache. I think I
have to agree C99+strict-aliasing lost the plot on this one. Note
mtortoise is only ever written/read through the same type.
What I want:
lfs3_trv_t: lfs3_bshrub_t: lfs3_handle_t:
.---+---+---+---. .. .---+---+---+---. .. .---+---+---+---.
| handle | | handle | | handle |
| | | | | |
+---+---+---+---+ +---+---+---+---+ .. '---+---+---+---'
| root rbyd | | root rbyd |
| | | | lfs3_mtortoise_t:
+---+---+---+---+ +---+---+---+---+ .. .---+---+---+---.
| leaf rbyd | | leaf rbyd | | mtortoise |
| | | | | |
+---+---+---+---+ +---+---+---+---+ .. '---+---+---+---'
| staging rbyd | | staging rbyd |
| | | |
+---+---+---+---+ .. '---+---+---+---'
| |
: :
But I'm starting to think this is simply not possible in modern C.
At least this shows what is theoretically possible if we didn't have to
fight the compiler.
Looks like these traversal states were missed in the omdir -> handle
rename. I think HANDLES and HBTREE states make sense:
- LFS3_TSTATE_OMDIRS -> LFS3_TSTATE_HANDLES
- LFS3_TSTATE_OBTREE -> LFS3_TSTATE_HBTREE
This matches other internal rbyds: btree.r, mdir.r, etc.
The intention of the single-char names is to reduce clutter around these
severely nested structs, both btrees and mdirs _are_ rbyds, so the name
doesn't really besides C-level type info.
I was hesitant on btree.leaf.rbyd, but decided consistency probably wins
here.
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.
I was confused, but this commit->bid update is used to limit the
commit->bid to the btree weight. Note we limit the bid after storing it
as the initial rid.
What a mouthful.
The unconditional bshrub leaf discarding in lfs3_mdir_commit was copied
from the previous btree leaf caching implementation, but discarding
_all_ bshrub leaves on _every_ mdir commit is a bit insane.
Really, the only bshrub leaves that ever need to be discarded here are
the shrub roots, which are already questionable leaf caching targets
because they're already cached as the root rbyd.
An alternative option would be to just never cache shrub roots, but
tinkering around with the idea showed it would be more costly that
conditionally discarding leaves in lfs3_mdir_commit. At least here we
can reuse some of the logic that discards file leaves.
I'm also probably overthinking what is only a small code cost:
code stack ctx
before: 36784 2400 684
after: 36792 (+0.0%) 2400 (+0.0%) 684 (+0.0%)
This doesn't take into account how much CPU time is spent creating rbyd
copies, but that is not something we are optimizing for.
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 --list-suite-paths was already skipping case-less suites! I think
only -Y/--summary was an outlier.
This is consistent with test.py's matching of suite ids when no cases
are found (test_runner itself doesn't really care, it just reports no
matching cases). Though we do still compile case-less suites and include
them in the test_suites array, which may be confusing in the future.
The --no-internal flag avoids building any internal tests/benches
(tests/benches with in="lfs3.c"), which can be useful for quickly
testing high-level things while refactoring. Refactors tend to break all
the internal tests, and it can be a real pain to update everything.
Note that --no-internal can be injected into the build with TESTCFLAGS:
TESTCFLAGS=--no-internal make test-runner -j \
&& ./scripts/test.py -j -b
For a curious data point, here's the current number of
internal/non-internal tests:
suites cases perms
total: 24 808 633968/776298
internal: 22 (91.7%) 532 (65.8%) 220316/310247 (34.8%)
non-internal: 2 ( 8.3%) 276 (34.2%) 413652/466051 (65.2%)
It's interesting to note that while internal tests have more test cases,
the non-internal tests generate a larger number of test permutations.
This is probably because internal tests tend to target specific corner
cases/known failure points, and don't invite much variants.
---
While --no-internal may be useful for high-level testing during a
refactor, I'm not sure it's a good idea to rely on it for _debugging_ a
refactor.
The whole point of internal testing is to catch low-level bugs early,
with as little unnecessary state as possible. Skipping these to debug
integration tests is a bit counterproductive!
- enum lfs3_scmp -> enum lfs3_cmp
- cmp -> cmp
lfs3_scmp_t is still used as the type, as the s prefix indicates the
type is signed, usually for muxing with error codes.
I think that led to the enum also being named lfs3_scmp, but that's not
quite right.
But none of this really matters because enums are so useless and broken
in C.
- enum lfs3_error -> enum lfs3_err
- err -> err
Really this just updates `enum lfs3_err` to match the prefixes used
everywhere else. And because enum types are kind of useless in C, this
has no effect on any other part of the codebase.
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.
Not sure how this got overlooked. Now that graft traversals are
implemented directly in lfs3_alloc, there's no reason to store this
state globally.
Fortunately this was in a union, so it didn't actually show up in our
ctx measurements.
No code changes.
- 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.
This just wraps up block allocation + struct initialization similarly to
lfs3_rbyd_alloc.
Also tweaked bptr updates in lfs3_file_crystallize__ to mutate the bptr
fields directly instead of going through lfs3_bptr_init.
Neither of these impacted code cost, everything ends up inlined in
lfs3_file_crystallize__ anyways. Hopefully it helps with readability at
least.
Also LFS3_KVONLY mode is completely broken, and fixing it is not a huge
priority. I don't think it makes sense to adopt lfs3_bptr_alloc in
lfs3_file_flushset_ anyways, it would just lead to us initializing the
lfs3_bptr_t struct twice for no real reason.
No code changes.
Currently this just has one flag the replaces the previous `erase`
argument:
LFS3_ALLOC_ERASE 0x00000001 Please erase the block
Benefits include:
- Slightly better readability at lfs3_alloc call sites.
- Possibility of more allocator flags in the future:
- LFS3_ALLOC_EMERGENCY - Use reserved blocks
- Uh, that's all I can think of right now
No code changes.
- Moved block allocator definitions into their own dedicated block
before the lfs3_bptr_t stuff:
- lfs3_alloc_discard
- lfs3_alloc_ckpoint
- lfs3_alloc
I'm looking into adding lfs3_alloc related flags, and these aren't
really predeclarable like C's function prototypes.
Predeclaring these before lfs3_bptr_t is relevant if we ever add
lfs3_bptr_alloc.
Also touched up relevant comments a bit.
- Also added inline to lfs3_alloc_ckpoint and lfs3_alloc_discard, which
was strangely missing?
The compiler figured it out anyways, so this has no impact on code
cost.
- Moved ecksum definitions below lfs3_bptr_t stuff.
I don't really know where to put these, but close to the lfs3_rbyd_t
stuff makes sense.
- And updated lazy appendrattr_ ordering to match source code order.
No code changes.
The main motivation for the `bool exists` pattern was to avoid issues
with err clobbering. But now that lfs3_mtree_pathlookup returns a
muxed tag + err, this is less of a concern.
In littlefs, the err variable is frequently used as a short lived
temporary for propagating error codes. So frequent that I really
wouldn't trust its state after a couple of lines. Quickly converting
err -> bool exists reduced the risk that some necessary err state ends
up clobbered in a refactor.
This risk is still present for the tag variable, but it's already more
common for tags to hold persistent state (they hold file types after
all), so I think this risk is manageable.
And why get rid of a variable that's arguably more self-documenting?
When you're trying to keep a program's state in your head, less state is
better than more state.
No code changes.
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 where I would've adopted tag-returns in lfs3_rbyd_namelookup,
but it turns out this isn't possible. We are already muxing error codes
with compare flags (lfs3_scmp_t)!
In theory we could merge err + lfs3_cmp_t + lfs3_tag_t into one big
16-bit ordered tag mux abomination, but I decided that was probably
overkill for now.
As a plus this avoids an awkward temporary tag copy in
lfs3_rbyd_namelookup as we search for a better tag. Turns out the
out-pointers in lfs3_rbyd_namelookup are quite useful for staging
things.
No code changes.
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 use of 0 here as a no-height indicator is probably not a good idea.
If this loop ever encounters height=0, it will reset progress, and
possibly calculate an incorrect min_height.
Fortunately this can't actually happen with our current rbyds. Only the
single element rbyd has height=0, and, lacking a tree, it is trivially
balanced. But this is the sort of sleeping bug that risks becoming a
real bug in the future, so might as well fix.
Using -1 (UINT_MAX) as a default value for min_height avoids this.
The child rbyd inherits all of the btree's root state when we hit the
root, including the shrub bit. This means we don't need to check
child.block == btree.block, since only the btree root can be shrubbed
(how would non-root shrubs even work? wait... they could work, but I
think it would just end up a worse balanced binary tree? anyways).
This lets us reorder things into the rare 3-case if statement, which
helps a bit with readability:
- !lfs3_rbyd_trunk(&child) || lfs3_rbyd_isshrub(&child) => need root
- child.blocks[0] == btree->blocks[0] => is root
- otherwise => not root
Shaved off some code:
code stack ctx
before: 36832 2368 656
after: 36828 (-0.0%) 2368 (+0.0%) 656 (+0.0%)
- LFS3_ERR_RANGE => need to split btree
- LFS3_ERR_EXIST => hit a shrub root
The distinct "hit shrub root" vs "split btree" error codes are a bit
more self documenting and let us assert during test time that we never
actually split bshrub roots.
Maybe this will be reverted after some use, but in the short term better
safe than sorry.
---
This comes at a small code cost, I guess loading from constant pools is
expensive (though, tbf, lfs3_btree_commit_ is a _big_ function, maybe
the size makes constant pools trickier?). I'm guessing it's the constant
pools because the changes in lfs3_bshrub_commit had no effect:
code stack ctx
before: 36800 2368 656
after: 36832 (+0.1%) 2368 (+0.0%) 656 (+0.0%)
Based on a few observations:
- Bshrubs never go straight to splitting.
Bshrubs are always converted to btrees first, which can't fail (shrub
< 1/2 block + commit < 1/2 block).
- This means we can rely on just the current shrub bit to determine if a
commitroot_ operation is a btree split or bshrub migration.
- This fully deduplicates the split/migrate logic, so we don't even need
LFS3_ERR_EXIST anymore. LFS3_ERR_RANGE now indicates both "split
btree" and "migrate bshrub".
There is an argument for keeping LFS3_ERR_EXIST around, as "migrate
bshrub" _is_ a conceptually distinct case from "split btree". But at
least this means one less error code that could be confusing.
Saves a nice bit of code and stack:
code stack ctx
before: 36832 2384 656
after: 36800 (-0.1%) 2368 (-0.7%) 656 (+0.0%)
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.
Helps with readability when we want to mutably slice a bptr.
Also saves a bit of code:
code stack ctx
before: 36936 2384 652
after: 36860 (-0.2%) 2384 (+0.0%) 652 (+0.0%)
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.
This use to save code/stack, but apparently not anymore:
code stack ctx
before: 36960 2392 652
after: 36936 (-0.1%) 2384 (-0.3%) 652 (+0.0%)
code stack ctx
ckdatacksumreads before: 38368 2720 660
ckdatacksumreads after: 38024 (-0.9%) 2624 (-3.5%) 660 (+0.0%)
The stack hot-path has changed significantly since then, with many
functions adopting LFS3_NOINLINE to get off the stack hot-path. Not sure
if that's related.
I'm also starting to think LFS3_FORCEINLINE is a symptom of
over-optimization. We shouldn't be doing the compilers job, if it can't
figure out the best inlining strategy so be it.
Christopher Haster