The core problem is that we weren't updating dropped mdirs with weight=0
if the mdir was compacted at the same time. This is hard to notice,
because most operations that can drop don't care about the mdir
afterwards, but in lfsr_fs_fixorphans this caused the fixorphan loop to
think it might still have orphans it could remove.
The implementation is very subtle here:
- In lfsr_mdir_commit_, if an error occurs during lfsr_mdir_compact__,
we need to revert to the original mdir state to allow fallback to mdir
split.
- In lfsr_mdir_commit_, if an error occurs during lfsr_mdir_commit__
(even after a compact), we need to update the mdir in case a drop
reduced the mdir weight to zero.
We also need to update the mdir for things like erased state, but this
doesn't come into play in the compaction route.
Fixed the bug by updating the mdir copy before lfsr_mdir_commit__.
Also added asserts to all insert/delete operations in test_mtree.toml.
We already had drop-during-compaction tests, but these didn't check that
the mdir was updated correctly. The new asserts catch this bug and
should prevent a regression.
This rearranges diverged pruning in lfsr_rbyd_appendattr a bit to try to
avoid unnecessary r->b recoloring during range removals.
Two tweaks:
1. lfsr_rbyd_appendattr now only alternates diverged paths on a black
edge.
This is equivalent to alternating on the underlying 2-3-4 tree,
and means we don't have to worry about overlapping red edges from
the two diverged paths interacting with each other in weird ways.
2. Thanks to alternating on a black edge, we can now prune diverged
paths before applying our red-black-yellow operations.
This means we can avoid the somewhat-hack that was r->b recoloring
(if you paint it black, red-yellow operations are skipped so nothing
breaks trivially, but you also break your tree balance invariants).
So the two diverged paths should remain strictly 2(log n)+1, at least
in isolation.
A nice side-effect of moving the diverged-prune code is we can
deduplicate pruning with yellow-edge prunning. This saves a branch of
code, though it will makes things a bit more confusing if anyone
tries to use lfsr_rbyd_appendattr as a template for an rbyd
implementation without range removals.
The new pruning conditions are more complex, resulting in more code
cost, but this will be worth it if it results in better tree balance
after range removals:
before: 33912 2880
after: 34064 (+0.4%) 2880 (+0.0%)
Some quick, non-rigorous benchmarks showed a noticable, but tiny
improvement in random write performance. Though I'm not sure random
writes are likely to hit degenerate range removals...
Now back to our naive compaction algorithm:
a_0 = 3t + 4
Why? The main reason is simplicity:
- In order to connect the first layer, we need to maintain some state
external to lfsr_rbyd_appendcompactattr, mainly rbyd.trunk which needs
to be zeroed before compaction.
This is a great recipe for bugs.
- lfsr_rbyd_appendcompaction needs a couple hacks to avoid redundantly
connecting the first layer, while also not missing any lingering
attrs/trunks in non-power-of-two trees. Though these hacks at least
fall into the stupid + works = not stupid category.
- Our interaction with the rcache is bad.
Before, our compaction algorithm sort of operated in two modes:
1. Append attrs - Without any rbyd structure, we only need the pcache
to append, allowing the rcache to be used entirely to, you know,
cache whatever is sourcing our attrs.
2. Build the balanced rbyd tree - At this point we need the rcache to
read rbyd trunks to connect everything, but we are also probably
done with the attr source.
But our improved compaction algorithm muddies step 1., now we need
the rcache to connect layer 1, which may thrash our attr source.
To be clear, this only happens because we reread the previous trunk
for the tag and weight. If we cached everything needed to build the
alt-pointer in RAM, we could avoid this, but we currently don't have
enough space in our rbyd struct.
So I'm reverting for now. Though I've left a comment noting some
theoreticals.
I think a better design would be to change lfsr_rbyd_appendcompactattr
to take an optional array of trunks (along with tags+weights), which
could be user configurable to trade RAM cost for metadata density.
It would be extra interesting to see such a configurable array tuned to
the current maximum stack depth, in theory making it effectively free.
But I think this is out-of-scope for now.
Now that tag-estimate has gotten complicated, here's a quick matrix to
note how attr-estimate changes with this revert (a_0):
block-size a_0 a_1 a_inf
512B => 31 25 18 (bytes)
16KiB => 34 27 20 (bytes)
2MiB => 37 30 22 (bytes)
256MiB => 40 32 24 (bytes)
Code changes:
code stack
before: 34132 2880
after: 33912 (-0.6%) 2880 (+0.0%)
This shouldn't impact most systems, but it's not unreasonable to allow
size-limit to be tweaked for the sole purpose of better metadata
density.
As a plus, if we mount a smaller filesystem, say a 16-bit littlefs, we'd
naturaly inherit its attr estimate/metadata density. This is probably
the more important side-effect.
This math is complicated enough to come with a bit of a code cost:
code stack
before: 34104 2880
after: 34132 (+0.1%) 2880 (+0.0%)
Though if we move size-limit + block-size to compile-time in the future,
this actually becomes free.
Otherwise this risks overflowing 31-bit leb128 limits elsewhere in the
system.
I think at some point I was considering allowing _some_ types to be
full 32-bit leb128s, mainly lfs_block_t and lfs_did_t, but at this point
it doesn't seem worth the tradeoffs (especially if you can jump to
64-bits/63-bits in the future). It also never worked to be clear.
Note no code actually changed.
The new compaction algorithm _does_ bring the directory estimate down
from ~96 -> ~72 bytes, but because we want a power-of-two for cheap
division, we floor both of these to ~64 bytes.
It's interesting to note that a perfect compaction algorithm _could_
bring the directory estimate down across the power-of-two boundary:
~72 -> ~48 bytes. But fortunately we're not perfect so we don't have
care about that.
The concern right now is small-block filesystems, anything in the 512B
to <4KiB range. With such small blocks, and rbyd's relatively high
per-attr overhead, there's a real risk that littlefs may just not be
able to function without quickly running to metadata limits.
I realize these are pretty rare geometries for flash, but they are still
common for anything that 1. pretends to be a spinny disks, SD cards,
FTLs, eMMCs, etc, and 2. mapping into RAM, which is surprisingly common.
It is possible to require this sort of geometry to pretend to be a
larger logical block-size, but since this is a regression from the
previous version of littlefs, it would be nice to avoid this if
possible.
Anyways, what actually is this commit. Consider our tag encoding:
.---+---+---+- -+- -+- -+- -+---+- -+- -+- -. tag: 2 bytes
| tag | weight | size | weight: <=5 bytes
'---+---+---+- -+- -+- -+- -+---+- -+- -+- -' size: <=4 bytes
total: <=11 bytes
With our current 32-bit (really 31-bit) version of littlefs, the worst
case tag encoding is 11 bytes.
This doesn't sound that bad, but with our current compaction algorithm we
need ~2.5 tags for each attr:
5t 5*11
a_1 = -- + 2 = ---- = 30 bytes
2 2
Are there any additional assumptions we can make to push our attr
estimate lower?
- tag - Ignoring a complete redesign of our tag encoding (which has
already been heavily iterated over), this just needs 2 bytes, which is
not that bad.
- weight - This is the real painful one because, for the most part,
weight=0. But weight _can_ store a full size, in the case it is the
root of a file's btree. So this is pretty much stuck at an annoying
5 bytes.
I suppose this could be tied to our size-limit. I hadn't thought about
that until writing this commit message. Maybe that can be a future
improvement, though it won't really have a big effect on most systems.
- size/jump - Now this field is interesting. When expressing both the
size of tag payloads, and the relative jump offset for alt-pointers,
this field should never exceed a single block.
We've already pushed this down to 4 bytes at compile time, by assuming
at most 28-bit block-sizes, but if we know the block-size, we could in
theory push this even lower.
This is extra enticing, because the block-sizes where the size/jump
field can be shrunk, are _also_ the block-sizes where the metadata
density is so critical!
So that's what this commit does. For the purpose of compaction estimates
(not stack allocations!) we calculate attr estimate based on our
runtime-determined block_size.
Here are some cutoff points for our new attr estimate:
block-size tag-estimate attr-estimate
512B => 9 bytes 25 bytes
16KiB => 10 bytes 27 bytes
2MiB => 11 bytes 30 bytes
256MiB => 12 bytes 32 bytes
There is a question of when to actually do this calculation. We always
know our block-size, so we could recalculate the attr-estimate every
time we need to estimate a compaction. But for now I'm just
precalculating the attr estimate in lfs_init and storing in the lfs_t
struct. It's only a byte after all.
If I did my math correctly, we won't exceed a byte until we have a
block-size of 2^1750, at which point we may have other problems.
Code changes:
code stack lfs_t
before: 34068 2880 216
after: 34104 (-0.1%) 2880 (+0.0%) 220 (+1.9%)
The jump in lfs_t cost is probably just from a word alignment boundary.
In the future, if we have compile-time block-sizes, the entire
attr-estimate could even be compile-time.
Now that we're assuming a perfect compaction algorithm, and an
infinitely compatible mleaf-bits, there really shouldn't be any reason
to support non-standard mleaf-bits in our scripts, right?
If a configurable mleaf-bits becomes necessary, we can always add this
back in the future.
As defined previously, mleaf-bits depended on the attr estimate, which
depended on the details of our compaction algorithm:
block_size
m = ----------
a_0
Assuming t=4, the _minimum_ tag encoding:
block_size block_size
m = ---------- = ----------
3*4 + 4 16
However, with our new compaction algorithm, our attr estimate changes:
block_size block_size block_size
m = ---------- = ----------- = ----------
a_1 (5/2)*4 + 2 12
But tying our mleaf-bits to our attr estimate is a bit fragile. Unlike
attr estimate, the calculated mleaf-bits MUST be the same across all
littlefs implementations, or else the filesystem may not be mountable.
We _could_ store mleaf-bits as an fs attr in the mroot, like we do with
name-limit, size-limit, block-size, etc, but I'd prefer to not add fs
attrs unless strictly required. Each fs attr adds complexity to mounting,
which has a non-zero cost and headache.
Instead, we can assume our compaction algorithm is perfect:
block_size block_size block_size
m = ---------- = ---------- = ----------
a_inf 2*4 8
This isn't actually achievable without unbounded RAM. But just because
our current implementation is limited to bounded RAM, does not prevent
some other implementation from pushing things further with unbounded
RAM.
In theory, since this is a perfect compaction algorithm, and builds
perfect rbyd trunks, this should be the maximum possible mleaf-bits
achievable in littlefs's current design, and should be compatible with
any future implementation.
---
Worst case, we can always add mleaf-bits as an fs attr retroactively
without breaking backwards compatibility. You would just need to assume
the above block_size-dependent value if the hypothetical mleaf-bits attr
is missing.
This is one nice thing about our fs attr system, it's very flexible.
The motivation for this is that the rbyd inner node encoding during
compaction is kind-of not that great.
Our alt encoding is great when the trunk terminates in a tag, which is
how it was originally designed to be used:
00000004: data w1 1 61 a <.
00000020: altble 0x300 w1 0x4 -' <-- trunk
00000024: data w1 1 62 b
But when used to create an arbitrary binary-tree inner node, the best
encoding I can think of is 2 alts + a terminating null tag, which is not
that great:
00000004: data w1 1 61 a <.
00000009: data w1 1 62 b <--.
0000000e: altble 0x300 w1 0x4 -' | <-- trunk
00000012: altble 0x300 w1 0x9 ---'
00000016: null
This effects our attr estimate, which is defined as the worst-case
on-disk cost of an attr after compaction. This is an important value,
as it determines when we split rbyds. And it effectively determines how
densely we can store metadata without needing to worry about block
overflow issues.
In our current compaction algorithm, we connect each attr with a 2 alt +
null inner node. Since we are creating a perfectly balanced binary tree,
this works out to ~1 inner node per attr. Including the attr's data tag,
this gives us:
a_0 = 3t + 4
Where t is the tag estimate, currently a 2 byte tag, <=5 byte weight,
<=4 byte size, t = 2+5+4 = 11 bytes:
a_0 = 3*11 + 4 = 37 bytes
Though this may be vary across different littlefs configurations,
16-bit, 64-bit, etc.
---
It would be great if our compaction algorithm could build each trunk
perfectly, as each attr is written. In such a case, each attr
theoretically only needs ~1 alt and the attr's data tag:
a_inf = 2t
Or, assuming t = 11 bytes:
a_inf = 2*11 = 22 bytes
Unfortunately, as far as I can tell, this fundamentally requires
unbounded RAM. You need to keep track of log n previous trunks in order
to always build the next trunk perfectly, and log n is > 1.
I suppose in theory you could implement a O(n^2) algorithm that
repeatedly scans for the previous trunks... But that would be a
hilarious regression since the whole point of this work was to reduce
compaction from O(n^2) -> O(log n).
---
However, we can meet halfway. Consider what happens if we build perfect
trunks for only the bottom layer of the rbyd.
This may not seem like it will gain much, but remember that in a binary
tree, the bottom layer contains ~1/2 of the total nodes in the tree:
3t + 4 2t 5t
a_1 = ------ + -- = -- + 2
2 2 2
Or, assuming t = 11 bytes:
5*11
a_1 = ---- + 2 = 30 bytes
2
Not too shabby for a constant amount of RAM.
In theory this could be extended to n layers, by keeping a
(configurable?) array of previous trunks in RAM during compaction, but
this would have diminishing results as we move up the tree.
With only needing to keep track of one other trunk, we can even store
this in rybd.trunk, which is currently unused during compaction. So zero
extra RAM.
The resulting compaction looks like the following:
00000004: data w1 1 61 a <.
00000020: altble 0x300 w1 0x4 <--.
00000024: data w1 1 62 b |
00000029: data w1 1 63 c <. |
0000002e: altble 0x300 w1 0x29 <. |
00000032: data w1 1 64 d | |
00000037: altble 0x300 w2 0x20 -' | <-- trunk
0000003b: altble 0x300 w2 0x2e ---'
0000003f: null
This does make compaction a bit more complicated, which is reflected in
the code size:
code stack
before: 33856 2880
after: 34068 (+0.6%) 2880 (+0.0%)
Some implementation things to note:
- Since lfsr_rbyd_appendcompactattr now has state, we need to make sure
to zero the trunk before compacting, which adds an annoying bit of
bookkeeping everywhere.
- I didn't want to add extra state to manage in
lfsr_rbyd_appendcompactattr calls, so this implementation only tracks
the previous trunk offset, and needs a readtag call to get the
tag+weight necessary to build the actual alt pointer.
This may have some unpleasant interactions with the rcache, and may be
worth revisiting.
To match the names of other flags like this, LFSR_DATA_ISIMM,
LFSR_MTREE_ISMPTR, LFSR_BSHRUB_ISBNULLORBSPROUTORBPTR, etc.
LFSR_RBYD_ISSHRUB was probably just missed in a refactor at some point.
The reason for the IS* prefix is to avoid conflicts with related macros.
I think at some point we were calculating the compact weight in the
initial tag layer, but we don't actually use this at all. We recalculate
the weight on every layer of our compaction algorithm anyways.
Code changes:
before: 33864 2880
after: 33856 (-0.0%) 2880 (+0.0%)
- It didn't save code.
- An inlined buffer is potentially more useful, even if only marginally,
and, uh, unproven yet.
- Requiring lfs_toleb128 in a readonly implementation is a hard ask.
The idea is that we can save on the cost of calling lfs_toleb128
everywhere we commit leb128s, by lazily encoding during progdata.
I original thought this would have too many small problems, but:
1. We can actually implement slice surprisingly easily by just shifting
the internal word 7 bits. This emulates byte-level slicing in the
encoded leb128.
This enables read/cmp, so we can implement all of the lfsr_data_t
functions, though it does make lfs_toleb128 required for a readonly
implementation, which isn't great. Sufficient creativity with ifdefs
likely makes this a non-problem though.
2. There's really very limited use cases for non-leb128 inlined datas.
We can use it to encode the version and compatflags during
lfs_format, but that's about it. And lfs_format is definitely not on
the stack hot-path, so there's no reason to not use on-stack buffers
for these.
The original motivation for this change was noticing a surprising amount
of code savings related to lazy leb128 encoding in another lfsr_data_t
refactor. Unfortunately this savings does not seem reproducible:
code stack
before: 33864 2880
after: 33912 (+0.1%) 2888 (+0.3%)
But that's ok, this is closer to what I expected. The lfs_sizeleb128
call we need to predict the leb128 size is close to the same cost as
calling lfs_toleb128 so the savings isn't really that much.
This doesn't really help us all that much right now, but will be useful
for the future-planned block map and being able to cache pre-erased
blocks.
Though the lack of erasing when allocating new mdirs raises some
questions... Oh well, future problems.
Code changes:
code stack
before: 33856 2880
after: 33864 (+0.0%) 2880 (+0.0%)
Topologically, this isn't really much of a change. We just moved the
flcksum -> lfs.pcksum and made the internal API a bit better.
But hey, a better internal API at ~no cost is always a good thing:
code stack lfs_t
before: 33868 2880 212
after: 33856 (-0.0%) 2880 (+0.0%) 216 (+1.9%)
Basically we now assume we will also concatenate data, even if there is
only a single data. But stack cost is worst-case anyways, so this
doesn't have any tangible downside.
code stack
before: 33944 2888
after: 33868 (-0.2%) 2880 (-0.3%)
The reason for carving up the right sibling before appending our new
data is because we 1. want to carve both left+right siblings in a single
lookup if possible, and 2. we don't want to keep unnecessary lookup state
around as much as possible.
But is keeping some lookup state around cheaper than the attr_tnuoc +
memmove mess? The answer is yes:
code stack
before: 34028 2896
after: 33944 (-0.2%) 2888 (-0.3%)
attr_tnuoc is one of those "if it's stupid and it works it's not stupid"
solutions, but that doesn't make it not stupid. (I'm joking a bit, but
the new code is cleaner + more readable, which was the original
motivation for looking at this function again)
There wasn't really a collision with this, and I think it's clear what
these flags are doing.
Also fixed a missed renamed of lfsr_tag_issup/subwide ->
lfsr_tag_issup/sub
Implementing raw-byte name comparisons ended up having more negative
effects on implementation requirements than I thought it would:
1. We would never actually concatenate the did + name, as that would
require dynamic memory. Instead we need to express the concatenated
relationship using our internal lfsr_data_t representation.
I thought this wouldn't be too bad since we already have a
concatenated lfsr_data_t representation, but:
1. It was limited in scope, specifically only lfsr_data_prog was
supported. It's actually not even possible to implement
lfsr_data_read (I think) since we can't mutate the indirect
lfsr_data_ts.
2. It's not actually required. We really only use our concatenated
representation to coalesce file fragments. You could in theory
omit this representation at the cost of not being able to limit
inlined shrub overhead.
Asking all future littlefs implementations to implement a
concatenated data representation (or dynamically allocate D:) for the
basic task of file-name lookup is sort of a big ask.
2. A readonly implementation suddenly needs a toleb128 function.
Which is an unexpected implication of requiring raw-byte leb128
comparisons for file-name lookup.
3. Raw-byte comparisons require that dids are always stored in their
canonical encoding (smallest leb128), though this is probably a good
idea anyways.
And for what? A theoretical future-planned feature (content-tree)?
Let's think about the hypothetical content-tree for a second:
1. It's an advanced, opt-in feature. Which means higher code/storage-cost
should be expected.
2. Basicall all littlefs implementations need file-name lookup, so
keeping file-name lookup cheap is a much higher priority than the
opt-int content-tree.
3. Worst case, the content-tree, and any future named trees, can just
set did=0. This will cost one byte per name (and may leave room for
future extensions).
So I'm reverting this for now.
There is still time before stabilization, so if it becomes clear there
is a better way to implement name lookups, we can still change this.
(Optimistically, the content-tree may be implemented before
stabilization, since it currently looks like it's required for data
redundancy).
Code changes:
code stack
before: 34292 2896
after: 34028 (-0.8%) 2896 (+0.0%)
Thanks to poor compound literal optimization, it's actually cheaper to
pass lfsr_data_t by value everywhere, than to make all LFSR_DATA_*
macros lvalues:
before: 34340 2896
after: 34292 (-0.1%) 2896 (+0.0%)
Why are these two design choices linked? If lfsr_data_t is
pass-by-address, the rvalue/lvalue disinction is important because we
need to take the address of LFSR_DATA_* macros. If lfsr_data_t is
pass-by-value, rvalue/lvalue doesn't really matter because we, well,
pass by value.
To be honest, this is a bit of an excuse for better lfsr_data_t
ergonomics. It _is_ generally worse code-size wise to pass lfsr_data_t
by value, because most ABI optimizations stop at 2 words and
lfsr_data_t requires 3 words. But always passing lfsr_data_t by value
even if it is suboptimal makes for more consistent internal interfaces.
This also helps side-step a mistake I made earlier where I though
cat/fromimm/fromleb128 were the only LFSR_DATA_* macros that needed to
be lvalues to be consistent. THERE ARE MANY MORE LFSR_DATA_* macros,
every LFSR_DATA_FROMBLAH macro to be specific, and the resulting code
cost would be MUCH WORSE.
---
This also add lfsr_sprout_t to complement lfsr_bptr_t/lfsr_shrub_t/etc.
Unlike lfsr_data_t, lfsr_sprout_t _is_ pass-by-address
Actually that's the only difference, haha. lfsr_sprout_t is a typedef.
Though to be fair, by being pass-by-addres, lfsr_sprout_t keeps the
internal sprout/shrub/bptr/btree inferfaces consistent, and saves a bit
of code.
This is a simplification of the rbyd/btree layers, but implies
behavioral changes to the mtree/mdir layers.
Instead of ordering by leb128 did + name:
82 02 61 61 61 < 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
We now order by the raw encoding, lexicographically:
82 02 61 61 61 > 81 04 62 62 62
(0x102, "aaa") (0x201, "bbb")
This may be unintuitive, but note:
1. Files _within_ a directory are still ordered, since they share a did
prefix.
2. We don't really care about the relative ordering of dids, just
that they are unique. Changing the ordering at this level does not
interfere with any of our did-related functions.
3. The only thing we may care about is that the root, did=0, is the
first mtree entry. This is still true. No leb128 encoding is < 0x00
even after encoding.
The motivation for this change is to allow for other named-btrees in the
system that may used non-did-prefixed names. At least one of these makes
sense for a sort of "content-tree" (cksum -> data block mapping).
As a plus, this change makes it possible to compare names and do btree
namelookups without needing to decode the leb128 prefix. Although I'm
struggling a bit to figure out exactly where this is useful...
One downside, this ordering only works if dids are always stored in
their canonical encoding, that is, the smallest leb128 encoding possible
for a given did. I think this is a reasonable requirement for just our
dids.
Another downside is this did add a decent chunk of code.
I did try limiting the changes to lfsr_data_namecmp, but it didn't have
much impact. I guess most of the cost comes from the reworked
lfsr_data_cmp function, which, to be fair, is quite a bit more
complicated now (it now supports limited data<=>data comparisons):
code stack
before: 34148 2896
namecmp: 34324 (+0.5%) 2896 (+0.0%)
after: 34340 (+0.6%) 2896 (+0.0%)
Most of the LFSR_DATA_* macros were already lvalues due to compound
literals:
#define LFSR_DATA_BUF(blablabla) \
((lfsr_data_t){blablabla})
The exception was when an (inlinable) function call was needed,
currently LFSR_DATA_CAT, LFSR_DATA_IMM, and LFSR_DATA_LEB128:
#define LFSR_DATA_CAT(blablabla) \
lfsr_data_fromcat(blablabla)
This gets a bit annoying when you want to pass the result of an
LFSR_DATA_* macro by address, sometimes it works, sometimes it doesn't:
lfsr_data_size(&LFSR_DATA_BUF(blablabla)); // works
lfsr_data_size(&LFSR_DATA_CAT(blablabla)); // doesn't work
This may seem like a minor annoyance, but not being able to pass the
result of LFSR_DATA_* macros by address becomes a real pain:
1. Most functions accept lfsr_data_t* because it's cheaper.
2. Most of the LFSR_DATA_* macros have compound-literal scope, so
creating a temporary requires creating temporaries for all arguments
recursively.
The solution is to wrap any functions with a compound literal, in this
can an array (I tested a struct but it had the same overhead):
#define LFSR_DATA_CAT(blablabla) \
((lfsr_data_t[]){lfsr_data_fromcat(blablabla)}[0])
---
What's really annoying is this introduces a surprising non-zero
code-cost:
code stack
before: 34016 2896
after: 34148 (+0.4%) 2896 (+0.0%)
Note this is just adding the above wrappers, not actually using their
lvalues yet. I wanted this on a separate commit because the added
code-cost is surprising. Unless I'm missing something, the semantics
haven't changed, so in theory a perfect compiler should optimize away
any in-stack moves? I'm not sure why it fails here.
I don't know how I feel about changing code just because of a compiler
idiosyncrasy. So I'm going to keep this for now.
At some point in the future, it may be worth considering alternatives
to our use of compound literals if they really interact with compiler
optimizations so poorly...
Before:
LFSR_ATTR(RM(SUBMASK(REG)), 0, BUF("hi", 2))
Now:
LFSR_ATTR(
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, 0,
LFSR_DATA_BUF("hi", 2))
Yes, it's more verbose now.
But there were a couple reasons for dropping the idea:
- The implicit prefixing is a bit magical, and not really all that
common in C code. It would likely confuse new users on first read.
- The implicitly prefixing macros did not play will with macro expansion
rules.
In particular, because the nested not-yet-prefixed macros aren't
really macros, they aren't expanded as a part of argument prescan.
This led to surprising compile-time errors, and prevented recursive
attr-lists (which may be useful for shrubs).
- Implicit prefixes is not very C-like, and in particular it gets in the
way of sed/grep operations on source files.
- RM(SUBMASK(REG)) for combining tags is (IMO) ugly, compared to
LFSR_TAG_RM | LFSR_TAG_SUBMASK | LFSR_TAG_REG, even if the latter
requires more typing.
- Sometimes you need runtime-dependent TAG/DATA values, which implicit
prefixing gets in the way of. The LFSR_TAG_TAG(tag)/
LFSR_DATA_DATA(tag) backdoors worked around this, but they are even
more magical, and added noise to a not-actually-all-that-uncommon use
case.
And it's really not _that_ much extra effort to write out the prefixes
everywhere.
lfs.c:
lines bytes
before: 16894 537171
after: 16907 (+0.1%) 538340 (+0.2%)
tests/*.toml:
lines bytes
before: 53306 1811035
after: 54517 (+2.3%) 1851006 (+2.2%)
qadte came in quite handy again for refactoring the tests without
completely losing my sanity.
So instead of:
uint8_t mptr_buf[LFSR_MPTR_DSIZE];
int err = lfsr_btree_commit(lfs, &mtree_.u.btree, 0, LFSR_ATTRS(
LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
FROMMPTR(lfsr_mdir_mptr(&mdir_), &mptr_buf))));
This can be written as:
int err = lfsr_btree_commit(lfs, &mtree_.u.btree, 0, LFSR_ATTRS(
LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
FROMMPTR(lfsr_mdir_mptr(&mdir_)))));
Explicit stack allocation is still possible with the DATA hole, though a
bit more annoying:
attrs[attr_count++] = LFSR_ATTR(
MDIR, +lfsr_mleafweight(lfs),
DATA(lfsr_data_frommptr(
lfsr_mdir_mptr(&mdir_),
&buf[buf_size])));
buf_size += LFSR_MPTR_DSIZE;
The main motivation for this change is to be consistent with
LFSR_DATA_CAT, which was already implicitly stack allocating. The macros
that take arrays are relatively error-prone otherwise (LFSR_DATA_CAT,
LFSR_ATTRS, etc).
This does come with the benefit that the required buffer size is
implicitly provided by the macro, so no worry of it falling out-of-sync
externally. However this does come with the tradeoff of compound literal
lifetimes, which requires the result to live only as long as the current
expression.
Hopefully the fact that these are MACROs signal that they need special
care to any new developers...
Unfortunately, the use of compound literals also brings a surprising
code/stack cost:
code stack
before: 33912 2872
after: 34016 (+0.3%) 2896 (+0.8%)
Currently I can think of two reasons:
1. It's not possible to declare an uninitialized compound literal.
This probably sounds like a good thing to memory-safety fans, and
initialized is probably a good default for variable declaration, but
the reality is the required initialization does add useless code.
This specific use of compound literals is also low-risk given that we
immediately pass the literal to an lfsr_data_from* function, which
does the initialization.
2. We sometimes share on-stack buffers between branches of ternary
expressions since we know their use is exclusive. These macros sort
of get in the way of that.
What I find a bit curious is GCC doesn't seem capable of optimizating
away these overheads, which I would think would be possible given that
GCC knows all the information of how these buffers end up used.
I've noticed in general compound literals add overhead when the
underlying semantics don't really change. I wonder if this is because
compound literals are relatively new/unused, or some required
side-effects I'm missing. Maybe this will improve in the future?
Anyways, I'm keeping this change for now, since it does improve the
internal attr-list ergonomics/safety. Though these sort of changes are
always open to be revisited in the future.
Interestingly, the future-theoretical transpilation to c89 may save
code/stack because of this, which raises some questions...
Now that in-block fields are limited to 28-bits, we have a few more bits
in our lfsr_data_t size field to encoding things.
This commit uses the top 2-bits to encode one of our 4 different
lfsr_data_t encodings:
- 00--nnnn nnnnnnnn nnnnnnnn nnnnnnnn => buffer poiner
- 01--nnnn nnnnnnnn nnnnnnnn nnnnnnnn => inlined data
- 10--nnnn nnnnnnnn nnnnnnnn nnnnnnnn => on-disk reference
- 11--nnnn nnnnnnnn nnnnnnnn nnnnnnnn => concatenated data pointer
.--|-------|-------|-------'
| | .-|-------'
| | | '------.
| '-----|--------|--------.
v v v v
1nnnnnnn 1nnnnnnn 1nnnnnnn 0nnnnnnn <= leb128
Note this still works with a hypothetical 12-bit/10-bit littlefs
variant, where we'd only have 2 spare bits:
- 00nnnnnn nnnnnnnn => buffer poiner
- 01nnnnnn nnnnnnnn => inlined data
- 10nnnnnn nnnnnnnn => on-disk reference
- 11nnnnnn nnnnnnnn => concatenated data pointer
.|-------'
|'-------.
v v
1nnnnnnn 0nnnnnnn <= leb128
We don't really care about 8-bit/7-bit, can we even fit an rbyd in a
127-byte block?
The main benefit of this encoding is that lfsr_data_t's pointer fields
get the same space as the two words used to encode on-disk block+off.
This may be useful on systems where ptr=2-word, such as some 16-bit
word/32-bit address devices, and some 2-word CHERI pointer devices.
One interesting thing to note: This encoding is only possible thanks to
the observation that total data size is sufficent information to write
out concatendated datas. We don't really need to know the exact number
until prog time, and during prog we can just iterate over datas until
size is exhausted.
So the size field turns out to be sufficient enough for indicating how
many datas are referenced, saving a data-count field.
Code changes are negligible. It should be noted that _most_ machines
won't benefit from ptr=2-word optimizations, including Thumb, our
benchmark ISA:
code stack
before: 33948 2872
after: 33912 (-0.1%) 2872 (+0.0%)
- YES_COV -> COVGEN
- YES_PERF -> PERFGEN
- YES_PERFBD -> PERFBDGEN
- YES_TESTMARKS -> TESTMARKS
- YES_BENCHMARKS -> BENCHMARKS
For a lack of better naming, may change in the future. Couldn't really
find any consistent prior art.
Note that no-suf/prefix doesn't work because of conflicts with the tool
overrides (PERF, COV, etc).
I may have been slightly nerd sniped.
I did start to worry about where evicting the rcache could lead to
performance pitfalls. One concerning, and not out-there case:
- Consider converting an inlined sparse file into a block. If the file
is sparse, we may end up with a number of lfsr_bd_set calls to fill
holes, but these holes may be quite small.
If rcache is quite big, we benefit greatly from keeping it in memory
during this operation. If rcache == block_size, we can even get away
with a single read.
But lfsr_bd_set hijacking the rcache would through a wrench in this,
forcing rcache eviction and a reread for every hole.
That and after sitting on it for a bit, trading IO for CPU feels wrong.
Even if the IO penalty is rare.
So decided to revisit and implement the same optimization we have for
bd read utils for bd prog utils.
---
Implementation wise is basically the same as the read case, with some
small differences:
- We need to flush the pcache in both caching and bypassing progs,
fortunately lfsr_bd_flush is already its own function.
- It's up to the caller the evaluate the eager cksum.
So there is now an explicit crc32c call in both lfsr_bd_prog and
lfsr_bd_set.
Though lfsr_bd_set never actually uses the eager cksum. We let
cross-function const propagation optimize this out in case we do need
it in the future.
- lfsr_bd_prognext assumes the prog succeeds in the calling bd util,
even though the data has not been written yet. If the bd util errors
before writing the data, the prog MUST be dropped or garbage will be
written.
- lfsr_bd_prognext only works because we lazily flush our pcache
So I guess the lazy flushing is a requirement now, instead of an
implementation quirk.
At least lfsr_bd_prog is off the stack-hot-path this time, so no stack
changes:
code stack
before: 33792 2872
after: 33948 (+0.5%) 2872 (+0.0%)
This is a compromise between using a small hardcoded buffer and
cache-access during progs. Instead of getting direct access to the
pcache during progs, we just hijack the rcache, forcefully evicting any
contents it might have.
This gets us cache-access (of at least some cache) without needing to
rewrite lfsr_bd_prog.
The downside is this may result in more rcache misses. Though the use of
lfsr_bd_set is fairly niche in littlefs, so hopefully this doesn't
become a problem.
Code changes:
code stack
before: 33796 2880
after: 33792 (-0.0%) 2872 (-0.3%)
For some definition of efficient.
Like lfsr_bd_cmp/cpy, this is intended to mirror memcmp/cpy/set/etc,
though it might get a bit confusing with lfs_set/setattr/etc meaning
something a bit different in the codebase...
You may notice this reintroduces the small hardcoded buffers we just put
in the effort to remove. Unfortunately the rcache access,
lfsr_bd_readnext, is really only useful for, well, reading, and
lfsr_bd_set is a prog util.
Implementing cache-access for progs would require as just as much
effort/cost as for reads, but gets a bit messy with calculating
checksums, and has less of a use case. We really only need this to fill
holes when compacting file data blocks. So, at least for now, I don't
think prog cache-access is worth it.
Though this can always be tweaked in the future.
Code changes:
code stack
before: 33744 2872
after: 33796 (+0.2%) 2880 (+0.3%)
lfsr_bd_readnext and lfsr_bd_read are almost the same function, with the
significant exception of cache-bypassing reads.
Bypassing reads are an interesting optimization in littlefs. Since we're
dealing with very constrained amounts of RAM, it's not uncommon for read
calls to have more RAM available than our internal caches. In this case
bypassing the cache 1. avoids copies, 2. reduces bus transaction, and 3.
leaves data in the rcache which may be useful for ongoing smaller
queries.
But bypassing reads make no sense for lfsr_bd_readnext, since
lfsr_bd_readnext calls have no buffer by definition.
This leads to a bit of a mess when you try to make lfsr_bd_read call
lfsr_bd_readnext, bypassing reads are lfsr_bd_read specific, but we need
to check for rcache/pcache prioritization first, which is the same in
both lfsr_bd_read and lfsr_bd_readnext.
The solution here is to duplicate the rcache/pcache prioritization
checks as a precondition for bypassing reads, at least deduplicating the
actual rcache/pcache memcpy. This isn't the greatest because memcpy is
actually pretty cheap in terms of code cost. But I don't see a better
organization.
The result is less code savings than expected.
Unfortunately this also comes with a high stack cost, just because of
the additional read->readnext stack frame. lfsr_bd_read is usually the
leaf on the hot path stack-wise, making the worst-case stack quite
sensitive to any changes to this function:
code stack
before readnext: 33584 2792
dup read/readnext: 33804 (+0.7%) 2808 (+0.7%)
rec read/readnext: 33744 (+0.5%) 2872 (+2.9%)
The use of small hardcoded buffers for non-buffering bd operations (cmp,
cksum, now cpy, etc), has been a common performance concern raised by
users.
It should be noted that thanks to our hint system, these are _only_ a CPU
bottleneck, which we usually don't care about (IO >> CPU). But back when
these were byte-level operation, on MCUs with low clock speeds this was
enough to make the filesystem CPU bound.
Since then, the practical bump up to 8-byte buffers seems to have mostly
avoided this bottleneck, or at least moved attention to other
performance-related issues. But still, it would be nice to have a better
solution. We have the caches after all, why aren't we using them?
This becomes more important as littlefs is jumping a bit in complexity
and we are relying more on the higher-level bd utils.
---
The solution implemented here is to add the function lfsr_bd_readnext,
which returns a buffer to one of the caches and amount of bytes
available, which may be less than requested. If the requested data is
not in any cache, the rcache is evicted and used to load the data from
disk, just like in lfsr_bd_read.
This unfortunately duplicates most of lfsr_bd_read, but makes it
possible to implement higher-level bd utils with zero copying.
This adds both minor code and stack costs (I guess our hardcoded buffers
really were small), but the motivation is reduced CPU usage:
code stack
before: 33584 2792
after: 33804 (+0.7%) 2808 (+0.6%)
This matches other functions where we may accept unbounded ranges, e.g.,
lfsr_rbyd_appendattrs, lfsr_data_slice, etc.
The motivation is that these constants, all zeros and all ones, often
have special encodings in ISAs due to their commonality. That and
constants are cheaper than runtime-dependent values such as block_size.
(block_size may be a compile-time constant at some point, but we will
still need to support runtime-determined block_sizes)
I thought this would be a quick change, but it led to an interesting
overflow condition in lfsr_bd_read when we calculate the cache
alignment/limit.
Fortunately, the rewritten expression is quite a bit cleaner.
The expression rewrite did drown out any code cost benefit, but I'm
keeping this change because it makes the code a bit more readable/
writeable when there's a simple "unbounded" value:
code stack
before: 33572 2800
after: 33584 (+0.0%) 2792 (-0.3%)
This logic previous lived in lfsr_bd_progdata, but really should be its
own bd function.
Hardware support can be a future thing-to-do. Maybe.
This currently uses the small-hardcoded-buffer approach used to
implement lfsr_bd_cmp/cksum, which isn't great, but gets the job done
for now.
code stack
before: 33544 2800
after: 33572 (+0.1%) 2800 (+0.0%)
Previous versions of littlefs saw very little pcache/rcache interaction,
which was a nice simplification for the bd layer. But now, with rbyds,
we rely overlapping pcaches/rcaches heavily. This is because building
each rbyd trunk requires reading the previous rbyd trunk, which may have
not made it to disk yet.
The main issue this presents, is that reads always need to prioritize
data in the pcache, even if it doesn't exist on disk yet.
This gets a bit annoying with read/prog alignment requirements, which
may require disk-reads that overlap the pcache.
And even more annoying when you consider that after a flush, the rcache
should reflect the new data even if pcache is dropped.
The fact that the current impl works at all is because of tests and
sweat...
---
To solve these problems, the bd layer would overwrite the rcache on
prog. This alone wasn't sufficient however, as we also need to overwrite
the rcache on reads because of the above alignment issue.
So:
pcache rcache
................ ................
read(0..4) ................ aaaa............
prog(6..10) ......bbbb...... aaaa..bbbb......
read(0..8) ......bbbb...... aaaaccbbbb...... => aaaaccbb
flush() ................ aaaaccbbbb......
read(0..8) ................ aaaaccbbbb...... => aaaacbbb
Note we can't just not overwrite the rcache, since flushing the pcache
leaves us with out-of-date information:
pcache rcache
................ ................
read(0..4) ................ aaaa............
prog(6..10) ......bbbb...... aaaa............
read(0..8) ......bbbb...... aaaacccc........ => aaaaccbb
flush() ................ aaaacccc........
read(0..8) ................ aaaacccc........ => aaaacccc !!!
This commit adopts a slightly different strategy: overwrite when we
flush:
pcache rcache
................ ................
read(0..4) ................ aaaa............
prog(6..10) ......bbbb...... aaaa............
read(0..8) ......bbbb...... aaaacccc........ => aaaaccbb
flush() ................ aaaaccbbbb......
read(0..8) ................ aaaaccbbbb...... => aaaaccbb
This keeps the rcache always in sync with disk (we don't care if pcache
is dropped without a flush), leaving unflushed pcache overwrites up to
lfsr_bd_read, which it needs to handle correctly anyways because of the
above alingment issue.
This saves a single overwrite.
Which isn't really that much when it comes to code cost:
code stack
before: 33560 2808
after: 33544 (-0.0%) 2800 (-0.3%)
But hey at least we're doing fewer copies? And no one should be tempted
to remove the overwrite-on-read code thinking it's redundant now (wasn't
me!).
We really had ~2 duplicate bd layers for a bit there.
This also involved a sort of rewrite of these low-level functions to see
if there were simplifications that could be made.
A couple tweaks:
- Added small low-level lfsr_bd_read/prog/erase/sync_ functions to
only wrap the bd callbacks and apply any relevant asserts.
These should be the only place we call the bd callbacks to make it
easy to read/audit/insert hooks in the future.
- Changed pcache flush lazily, rather than eagerly flushing when full.
This isn't for any real performance reason, it just makes the code
simpler. It's not like we can shove more data into the pcache once
full.
It's _probably_ a good idea to flush eagerly, to avoid delay more work
until sync, but I couldn't figure out how to make this work cleanly
without code duplication...
- Deduplicated read pcache overwrites via lfsr_bd_read__.
This logic is a bit annoying, but we need the pcache to take priority
whenever we read from disk, which happens when we both fill our
rcache, and bypass our rcache. Since these code paths go different
places, another internal function was the only way I could think to
deduplicate this.
It may appear that our pcache/rcache prioritization loop will make
this happen naturally, as it does in lfs_file_read for example, but
this doesn't quite work as read-alignment requirements may force us to
read past the pcache... Keep in mind read_size may be > prog_size.
- Dropped LFS_BLOCK_NULL, now using cache.size=0 to indicate a cache is
unused.
This avoids a special lfs_block_t value.
- Dropped lfsr_bd_readcksum, we never used this.
We can always add it back if necessary.
In total, the caching bd prog/read functions now look quite a bit more
like our file read/write functions, so hopefully that's a good thing.
By the virtue of not have ~2 duplicate bd layers, this saves a bit of
code:
code stack
before: 33700 2800
after: 33560 (-0.4%) 2808 (+0.3%)
- NO_COV -> YES_COV
- NO_PERF -> YES_PERF
- NO_PERFBD -> YES_PERFBD
Previously, COV defaulted to yes for tests, and PERFBD defaulted to yes
for benches. This is sometimes useful, but much less often than I
originally thought. Might as well not pay for what we don't use.
With this, the build features of the test/bench runners are consistent
by default, which is probably a good thing.
This _does_ have a noticable, if minor, impact on test runtime:
YES_COV: 674.15s
NO_COV: 584.97s (-13.2%)
As for the naming, the YES_* prefix is needed to avoid conflicts with
the tool variables themselves. I'm not sure what the best approach to
variable naming is here...
$ YES_PERF=1 PERF=~/my_perf/my_perf make test-runner -j
So instead of preprocessing lfs.t.a.c -> lfs.t.c, we preprocess
lfs.t.c -> lfs.t.a.c.
Really the extension should indicate what tool it was generated by, not
what tool it should be consumed by.
The previous commit that changed this states:
> Changed prettyasserts.py rule to .a.c => .c, allowing other .a.c files
> in the future.
But I'm not really sure why we would ever not just run prettyasserts.py
on every C file...
External use of prettyasserts.py made it clear the previous naming was a
bit weird.
Unfortunately, prettyasserts.py is having a hard time keeping up with
the constantly increasing number of tests. This is creating real
friction when debugging, as it now takes ~6x the time to preprocess
asserts as it does to actually compile the thing:
$ time ./scripts/prettyasserts.py \
-a LFS_ASSERT -u LFS_UNREACHABLE \
lfs.t.a.c -o lfs.t.c
real 0m16.187s
user 0m16.163s
sys 0m0.025s
$ time gcc -c -O0 -I. lfs.t.c -o lfs.o
real 0m2.466s
user 0m2.345s
sys 0m0.105s
Externally, I've rewritten prettyasserts.py in Rust, with more attention
towards performance (prettyasserts.py does quite a number of string
allocations). The result is quite satisfying:
$ time ~/prettyasserts/prettyasserts \
-a LFS_ASSERT -u LFS_UNREACHABLE \
lfs.t.a.c -o lfs.t.c
real 0m0.504s
user 0m0.464s
sys 0m0.040s
However, adding Rust as a requirement to test littlefs would be, uh,
quite a big jump.
So instead, littlefs keeps prettyassert.py, so only Python is needed out
of the box, and if the slow preprocessing is too much users are welcome
to provide their own prettyasserts binary via the PRETTYASSERTS env
variable:
$ time \
DEBUG=1 \
make test-runner -j
real 0m22.204s
user 0m44.841s
sys 0m1.478s
$ time \
DEBUG=1 PRETTYASSERTS=~/prettyasserts/prettyasserts \
make test-runner -j
real 0m5.699s
user 0m23.590s
sys 0m1.151s
Previously, the intention of upper case -Z was the match -W/--width and
-H/--height, which are uppercase to avoid conflicts with -h/--help.
But -z/--depth isn't _really_ related to -W/-H.
This avoids a conflict with -Z/--lebesgue, but may conflict with
-z/--cat. Fortunately we don't currently have any conflicts with the
latter. Since -z/--depth and -Z/--lebesgue are both disk-layout related,
the risk of conflicts are probably much higher there.
So now these should be invoked like so:
$ ./scripts/dbglfs.py -b4096x256 disk
The motivation for this change is to better match other filesystem
tooling. Some prior art:
- mkfs.btrfs
- -n/--nodesize => node size in bytes, power of 2 >= sector
- -s/--sectorsize => sector size in bytes, power of 2
- zfs create
- -b => block size in bytes
- mkfs.xfs
- -b => block size in bytes, power of 2 >= sector
- -s => sector size in bytes, power of 2 >= 512
- mkfs.ext[234]
- -b => block size in bytes, power of 2 >= 1024
- mkfs.ntfs
- -c/--cluster-size => cluster size in bytes, power of 2 >= sector
- -s/--sector-size => sector size in bytes, power of 2 >= 256
- mkfs.fat
- -s => cluster size in sectors, power of 2
- -S => sector size in bytes, power of 2 >= 512
Why care so much about the flag naming for internal scripts? The
intention is for external tooling to eventually use the same set of
flags. And maybe even create publically consumable versions of the dbg
scripts. It's important that if/when this happens flags stay consistent.
Everyone familiar with the ssh -p/scp -P situation knows how annoying
this can be.
It's especially important for littlefs's -b/--block-size flag, since
this will likely end up used everywhere. Unlike other filesystems,
littlefs can't mount without knowing the block-size, so any tool that
mounts littlefs is going to need the -b/--block-size flag.
---
The original motivation for -B was to avoid conflicts with the -b/--by
flag that was already in use in all of the measurement scripts. But
these are internal, and not really littlefs-related, so I don't think
that's a good reason any more. Worst case we can just make the --by flag
-B, or just not have a short form (--by is only 4 letters after all).
Somehow we ended up with no scripts needing both -b/--block-size and
-b/--by so far.
Some other conflicts/inconsistencies tweaks were needed, here are all
the flag changes:
- -B/--block-size -> -b/--block-size
- -M/--mleaf-weight -> -m/--mleaf-weight
- -b/--btree -> -B/--btree
- -C/--block-cycles -> -c/--block-cycles (in tracebd.py)
- -c/--coalesce -> -S/--coalesce (in tracebd.py)
- -m/--mdirs -> -M/--mdirs (in dbgbmap.py)
- -b/--btrees -> -B/--btrees (in dbgbmap.py)
- -d/--datas -> -D/--datas (in dbgbmap.py)
- -n/--no-defaults - disable default patterns
The default patterns can be brought back explicitly with:
- -a/--assert - enable assert pattern
- -u/--unreachable - enable unreachable pattern
- -A/--arrow - enable arrow patterns
Technically the default configuration is equivalent to the follow:
$ ./scripts/prettyasserts.py \
-a assert \
-a __builtin_assert \
-u unreachable \
-u __builtin_unreachable \
-A \
input.a.c -o output.c
This isn't really useful for littlefs, but may be useful elsewhere
The main benefit is control over error reporting and avoiding the dive
into stdlib layers when debugging thanks to __builtin_trap().
This changes -p/--pattern -> -a/--assert
And adds -u/--unreachable
The main star of the show is the adoption of __builtin_trap() for
aborting on assert failure. I discovered this GCC/Clang extension
recently and it integrates much, _much_ better with GDB.
With stdlib's abort(), GDB drops you off in several layers of internal
stdlib functions, which is a pain to navigate out of to get to where the
assert actually happened. With __builtin_trap(), GDB stops immediately,
making debugging quick and easy.
This is great! The pain of debugging needs to come from understanding
the error, not just getting to it.
---
Also tweaked a few things with the internal print functions to make
reading the generated source easier, though I realize this is a rare
thing to do.
These just take normal paths now, we weren't even using the magic
test/bench suite finding logic since it's easier to just pass everything
explicitly in our Makefile.
The original test/bench suite finding logic was a bad idea anyways. This
is what globs are for, and having custom path chasing logic is
inconsistent and risks confusion.
Motivation:
- Debuggability. Accessing the current test/bench defines from inside
gdb was basically impossible for some dumb macro-debug-info reason I
can't figure out.
In theory, GCC provides a .debug_macro section when compiled with -g3.
I can see this section with objdump --dwarf=macro, but somehow gdb
can't seem to find any definitions? I'm guess the #line source
remapping is causing things to break somehow...
Though even if macro-debugging gets fixed, which would be valuable,
accessing defines in the current test/bench runner can trigger quite
a bit of hidden machinery. This risks side-effects, which is never
great when debugging.
All of this is quite annoying because the test/bench defines is
usually the most important piece of information when debugging!
This replaces the previous hidden define machinery with simple global
variables, which gdb can access no problem.
- Also when debugging we no longer awkwardly step into the test_define
function all the time!
- In theory, global variables, being a simple memory access, should be
quite a bit faster than the hidden define machinery. This does matter
because running tests _is_ a dev bottleneck.
In practice though, any performance benefit is below the noise floor,
which isn't too surprising (~630s +-~20s).
- Using global variables for defines simplifies the test/bench runner
quite a bit.
Though some of the previous complexity was due to a whole internal
define caching system, which was supposed to lazily evaluate test
defines to avoid evaluating defines we don't use. This all proved to
be useless because the first thing we do when running each test is
evaluate all defines to generate the test id (lol).
So now, instead of lazily evaluating and caching defines, we just
generate global variables during compilation and evaluate all defines
for each test permutation immediately before running.
This relies heavily on __attribute__((weak)) symbols, and lets the
linker really shine.
As a funny perk this also effectively interns all test/bench defines by
the address of the resulting global variable. So we don't even need to
do string comparisons when mapping suite-level defines to the
runner-level defines.
---
Perhaps the more interesting thing to note, is the change in strategy in
how we actually evaluate the test defines.
This ends up being a surprisingly tricky problem, due to the potential
of mutual recursion between our defines.
Previously, because our define machinery was lazy, we could just
evaluate each define on demand. If a define required another define, it
would lazily trigger another evaluation, implicitly recursing through
C's stack. If cyclic, this would eventually lead to a stack overflow,
but that's ok because it's a user error to let this happen.
The "correct" way, at least in terms of being computationally optimal,
would be to topologically sort the defines and evaluate the resulting
tree from the leaves up.
But I ain't got time for that, so the solution here is equal parts
hacky, simple, and effective.
Basically, we just evaluate the defines repeatedly until they stop
changing:
- Initially, mutually recursive defines may read the uninitialized
values of their dependencies, and end up with some arbitrarily wrong
result. But as the defines are repeatedly evaluated, assuming no
cycles, the correct results should eventually bubble up the tree until
all defines converge to the correct value.
- This is O(n*e) vs O(n+e), but our define graph is usually quite
shallow.
- To prevent non-halting, we error after an arbitrary 1000 iterations.
If you hit this, it's likely because there is a cycle in the define
graph.
This is runtime configurable via the new --define-depth flag.
- To keep things consistent and reproducible, we zero initialize all
defines before the first evaluation.
I don't think this is strictly necessary, but it's important for the
test runner to have the exact same results on every run. No one wants
a "works on my machine" situation when the tests are involved.
Experimentation shows we only need an evaluation depth of 2 to
successfully evaluate the current set of defines:
$ ./runners/test_runner --list-defines --define-depth=2
And any performance impact is negligible (~630s +-~20s).
So instead of:
lfs_cmp(cmp) <= 0
You can do:
cmp <= LFS_CMP_EQ
This is much simpler and still preserves the ability to use all of C's
comparison operators on the results of disk comparisons.
The duplicate decoder for little-leb128 avoided extra stack allocation
for the unaligned worst-case leb128 encoding, but did result in a
duplicate function and extra code cost.
Reasons for deduplicating:
- We'd definitely want to deduplicate these functions if they end up
with the same encoding cost (28-bit littlefs mode?).
- Less code is less code.
- I noticed the stack savings are arch dependent because
lfsr_data_readlleb128 only sometimes ends up on the "hot-path".
thumb calls lfsr_data_readlleb128 on the hot-path, but x86 ends up in
lfsr_bd_readtag. So it's not clear this stack savings is really
valuable vs buffer reductions higher up the stack.
Though I'm not really sure how much I trust stack.py based analysis
right now...
- 8 bytes of RAM is more likely to be compiler noise than 100 bytes of
code. Still, both are somewhat negligible and I should probably move
on from this...
I did also try an internally deduplicated version, with an
lfsr_data_readleb128_ that takes a buffer provided by both
lfsr_data_readleb128 and lfsr_data_readlleb128, but this ended up the
worst of both worlds likely just due to compiler overhead. Abstractions
have cost!
code stack
duplicated: 33808 2792
little-calls-big: 33700 (-0.3%) 2800 (+0.3%)
dedup-via-buffer: 33796 (-0.0%) 2816 (+0.9%)
This avoids the extra stack allocation for the unaligned worst-case
leb128 encoding by duplicating most of the "big-leb128" decoder. The
upside is less stack usage, but at a code cost, since we basically have
two copies of this function now.
This is a bit of a tough call, the percentage change is basically the
same:
code stack
before: 33700 2800
after: 33808 (+0.3%) 2792 (-0.3%)
On one hand, we would want to deduplicate these functions if they end up
with the same encoding cost (28-bit littlefs mode?), and less code is
less code, on the other hand, RAM is in general more valuable than
code...
This may be worth reverting in the future...
Christopher Haster