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.
- 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.
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.
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.
Maybe it's because they are relatively new, but compound literals seem
to do more harm then good.
I'm still keeping the LFS3_DATA_SLICE macro around in case it's useful
(for tests?), but now prefering lfs3_data_slice where possible.
---
This doesn't really impact the default build, but it saves a big chunk
of code/stack when compiling with LFS3_CKDATACKSUMREADS:
code stack ctx
before: 36956 2392 652
after: 36960 (+0.0%) 2392 (+0.0%) 652 (+0.0%)
code stack ctx
ckdatacksumreads before: 38576 2744 660
ckdatacksumreads after: 38368 (-0.5%) 2720 (-0.9%) 660 (+0.0%)
LFS3_CKDATACKSUMREADS adds cksize/cksum fields to lfs3_data_t, so it's
very sensitive lfs3_data_t function changes.
Though to be far, at 5-words, lfs3_data_t really shouldn't be a
pass-by-value struct. We only keep lfs3_data_t a pass-by-value struct
because LFS3_CKDATACKSUMREADS is low-priority/best-effort and it would
make the codebase a mess.
These can be accomplished with LFS3_DATA_SLICE, and I think the
TRUNCATE/FRUNCATE variants just muddy things and make the math harder to
read.
LFS3_DATA_TRUNCATE is already basically a noop. The only non-trivial
transformation is LFS3_DATA_FRUNCATE, and LFS3_DATA_FRUNCATE is the
confusing one.
---
I have no idea why _removing_ code is adding so much stack. This needs
investigation:
code stack ctx
before: 36944 2384 652
after: 36956 (+0.0%) 2392 (+0.3%) 652 (+0.0%)
I think these were copied from the initial fragment slice calculation,
but we're already checking for <=fragment_size, so the extra lfs3_min is
unnecessary.
Saves a bit of code:
code stack ctx
before: 36952 2376 652
after: 36944 (-0.0%) 2384 (+0.3%) 652 (+0.0%)
Not sure why this added stack, compiler noise?
This was missed when reverting the trailing underscores_ in other
unconditional out-pointers.
The trailing underscore now just hints at the parameter being an
out-pointer, optionality is no longer implied.
This tweaks LFS3_DBGRBYDBALANCE to be a bit less intrusive, by putting
the relevant heights in the single lfs3_rheight_t struct.
Also added ifdefs to lfs3_rbyd_lookupnext_ just to make it clear this
code is opt-in.
No code changes.
This may have changed during some refactor, but we can reuse the entire
right branch logic, and at least deduplicate the lfs3_data_frombranch
call on the left branch.
Saves a nice bit of code:
code stack ctx
before: 37020 2392 652
after: 36952 (-0.2%) 2376 (-0.7%) 652 (+0.0%)
Also deduplicating the lfs3_data_t allocations saved stack, though that
is more concerning than anything else...
Also adopted l/r_buf names in lfs3_bcommit_t. This better matches names
in lfs3_file_graft_ and elsewhere.
A bit of an abuse of this error code, but this is more explicit than the
previous rattr_count > 0 condition.
Forgetting to set rattr_count=0 on a normal exit has introduced bugs
before.
---
Though I'm not sure why this adds code. Somehow, _removing_ the
rattr_count=0 statements when lfs3_btree_commit_ collapses the root
added code?
code stack ctx
before: 36996 2392 652
after: 37020 (+0.1%) 2392 (+0.0%) 652 (+0.0%)
Seriously, add bcommit->rattr_count = 0 to lfs3_btree_commit_ and the
lfs3_btree_commit_'s code cost shrinks by 8 bytes. Is the compiler
hiding stuff in bcommit?
I'm just going to chalk this up to compiler noise for now...
This somewhat replaces lfs3_bctx_t. Really lfs3_bctx_t consumed the
previously separate bid, rattr, and rattr_count out-pointers and
underwent a slight name change. The previous contents of lfs3_bctx_t are
all available under bcommit.ctx, with some minor tweaks.
The main motivation for this was to get rid of the mess that was the
bid/rattr out-pointers. They represent a side-channel of internal btree
state that is probably better implemented as a single struct.
Hopefully this makes the logic of lfs3_btree_commit_ callers -- and
expected action on non-zero rattr_count -- more obvious.
---
Some other tweaks:
- Separated ctx.buf into bcommit.ctx.branch_l_buf/branch_r_buf.
I realized this informs the compiler that the lfs3_data_frombranch
calls should not overflow.
This may need to be reverted if we ever commit different data types in
lfs3_btree_commit_, but that's not the end of the world. Right now
this is bound to whatever split needs (2 branches + name).
- Added rattr_count <= rattrs assert after each btree commit builder.
These asserts were just adopted after the btree code was written. The
extra safeguards are good to have in case of future refactor.
Shaves off a bit more code/stack while also (hopefully) improving code
readability:
code stack ctx
before: 37048 2416 652
after: 36996 (-0.1%) 2392 (-1.0%) 652 (+0.0%)
This moves the default recurse logic (previously the commit label) back
up before the compact/relocate/split/merge branches.
I know the general rule is to try to limit gotos to foward jumps, but in
this case, placing the default recurse logic at the end of
lfs3_btree_commit_ disrupts the default "happy" path and makes
refactoring more difficult than it needs to be.
Contextually, the default recurse logic is a part of the default commit
logic, and split, merge, etc, are exceptional branches that just happen
to sometimes converge.
---
I think the real problem is that all of the gotos in lfs3_btree_commit_
are modeling mutually recursive functions, but in a context where we
can't actually recurse.
_Technically_, it is possible to transform any tail-recursive function
into loops and if statements (structured program theorem), but doing so
risks significant code duplication. We could duplicate this recurse
logic everywhere it's needed for example. But this is also something we
want to avoid in littlefs.
So goto soup it is.
---
Some code changes, but probably just compiler noise:
code stack ctx
before: 37052 2416 652
after: 37048 (-0.0%) 2416 (+0.0%) 652 (+0.0%)
I also added some more informative-only labels now that we've adopted
-Wno-unused-label. These are useful for documenting independent chunks
of logic in a large function like this, and as debugging targets.
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%)
This was the main culprit behind our stack increase. Inlining
lfs3_file_crystallize into lfs3_file_flush_ adds a bit of code, but as a
tradeoff:
- Keeps all lfs3_file_crystallization_ calls at the same abstraction
level, which is generally easier to reason about and avoids issues
with things like lfs3_alloc_ckpoints.
- Makes some low-level interactions, such as LFS3_o_UNCRYST masking,
more obvious.
- Reduces the stack hot-path by the cost of lfs3_file_flush_
Saves some stack at a code cost:
code stack ctx
before: 37492 2464 656
after: 37504 (+0.0%) 2448 (-0.6%) 656 (+0.0%)
Now that the dust has settled a bit, we can also compare the lazy
grafting vs lazy crystallization builds:
code stack ctx
lazy-graft: 38020 2456 656
lazycryst: 37504 (-1.4%) 2448 (-0.3%) 656 (+0.0%)
It worked well for file leaves, so we might as well adopt the same
post-truncate/fruncate logic for caches.
This moves checks for cache.size==0 from lfs3_file_write into
lfs3_file_truncate/fruncate.
Note that lfs3_file_truncate/fruncate are the only functions (for now)
that can reduce the size of a file.
Adds a bit of code, which is probably why this wasn't adopted earlier,
but it reduces the state we need to worry about and makes things easier
to understand:
code stack ctx
before: 37468 2464 656
after: 37492 (+0.1%) 2464 (+0.0%) 656 (+0.0%)
Now that we don't need to worry about losing data due to ungrafted
state, we can decide whether or not to discard leaves after
truncate/fruncate.
This simplifies lfs3_file_truncate/fruncate (and makes them much more
readable as a plus), but also lets us simplify lfs3_file_crystallize
since we no longer need to worry about implicit flushes.
lfs3_file_crystallize's call sites:
- lfs3_file_flush_ - We've already committed to flushing, so
opportunistically clearing LFS3_o_UNFLUSH has no effect.
lfs3_file_flush_'s logic should already take advantage of possible
flushes anyways.
- lfs3_file_flush - We only call lfs3_file_crystallize _after_
lfs3_file_flush_, so this has no effect.
This saves a bit more code and stack:
code stack ctx
before: 37588 2472 656
after: 37468 (-0.3%) 2464 (-0.3%) 656 (+0.0%)
Seeing as the generalized lfs3_file_crystallize_ API had a much lower
cost than I thought, we might as well keep it around a bit longer.
Though I at least tweaked it to hopefully be easier for compilers to
optimize: By accepting crystal_min=-1 as an alias for
crystal_min=crystal_max, compilers should always by able to const
propagate this.
---
Note sure why this still adds 8 bytes of code, it just looks like
compiler noise in lfs3_file_crystallize__? Is the LFS3_NOINLINE
attribute messing with compiler optimizations?
code stack ctx
before: 37580 2472 656
after: 37588 (+0.0%) 2472 (+0.0%) 656 (+0.0%)
Christopher Haster