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...
One downside of leb128 encoding is that the worst case encoded size is
not that well aligned due to a relatively underutilized last byte:
0xffffffff => 0xff 0xff 0xff 0xff 0x0f
This normally doesn't really matter, the whole point of leb128 is that
larger encodings are statistically less likely. But in littlefs we need
to allocate the worst-case buffer size in order to encode/decode
leb128s, and these buffers need to stick around on the stack during
metadata commit calls, which are also the point of highest stack usage
in the system.
But 32-bits is somewhat arbitrary, it just happens to be our register
size. In fact, we're not really using 32-bits, but instead only 31-bits
to take advantage of the sign bit for ad-hoc sum types:
0x7fffffff => 0xff 0xff 0xff 0xff 0x07
In theory, if we limit this further to 28-bits, we could save some stack
space:
0x7fffffff => 0xff 0xff 0xff 0xff 0x07
0x0fffffff => 0xff 0xff 0xff 0x7f
This may seem like a small amount of savings, but it also restores
alignment to the encoding, and should result in less wasted padding
around buffers.
Though it's important to note these are the most valuable bits, as the
range grows exponentially with each bit added. Reducing 31-bits to
28-bits reduces the range from ~2GiB to ~256MiB:
0x7fffffff => 2,147,483,647
0x0fffffff => 268,435,455
---
At the moment I'm hesistant to reduce _all_ on-disk leb128s to 28-bits.
The signed-32-bit limit of ~2GiB is fairly well understood in this
space, mainly thanks to FAT, and reducing this to ~256MiB risks quite a
surprise to users (it's also a regression from the current littlefs
version).
But one type where this limit is pretty reasonable is our block_size.
I don't think we'll see devices with erase blocks >256MiB for a while,
and at the very least those devices will probably need a 64-bit
filesystem for other reasons anyways...
And limiting block_size to <=256MiB has a surprising number of knock-on
effects:
- The tag size/jump field never exceeds 28-bits, reducing worst-case tag
dsize from 12 bytes -> 11 bytes.
The also reduces our worst-case attr-estimate from 40 bytes ->
37 bytes
- rbyd/btree trunks never exceed 28-bits, saving space in shrub/branch/
btree encodings.
- The bptr encoding is reduced from 24 bytes -> 21 bytes, since several
of its fields are in-block (size, off, cksize).
- The commit checksum encoding is reduced by a byte for every commit,
from 12 bytes -> 11 bytes.
This is due to needing to expand the cksum tag's size field to the
worst possible leb128 encoding due to a catch-22 situation.
Unfortunately the actual stack savings is a bit underwhelming:
code stack
before: 33688 2808
after: 33700 (+0.0%) 2800 (-0.3%)
This may be because, by adopting 28-bits in only some fields, most
buffers still end up unaligned and the on-stack size doesn't change due
to padding. Or it could just be that I'm overestimating the cost of our
on-stack buffers.
Still, I think the change is worth keeping if only for the reducing
attr-estimate and saved byte on every on-disk commit.
In the future it would be interesting to explore additional
configurations, e.g. a 28-bit flavor of littlefs to compliment this
31-bit flavor. You could imagine the fitting into other register sized
flavors for different capacity/code cost/device compat tradeoffs:
flavor register leb128 size-limit
14-bit littlefs => 16-bit 2 bytes ~16KiB
15-bit littlefs => 16-bit 3 bytes ~32KiB
28-bit littlefs => 32-bit 4 bytes ~256MiB
31-bit littlefs => 32-bit 5 bytes ~2GiB
56-bit littlefs => 64-bit 8 bytes ~64PiB
63-bit littlefs => 64-bit 9 bytes ~8ExiB
This is where the on-disk size-limit attr would really shine.
---
Note we don't need an additional on-disk limit attr for the block_size.
We already store the block_size in the superblock, so we just need to
error if attempting to mount a filesystem with block_size >256MiB.
This matches bptr's cksize/cksum a bit better and helps avoids confusion
when discussing the various size fields used to encode a commit's
various checksum tags.
I find these little diagrams useful for visualizing the actual on-disk
encoding, which doesn't really exist in the code outside of the
lfsr_data_from* and lfsr_data_read* functions.
This should really be unsigned, rbyd weights can not be negative.
Note this is different than data.size, etc, since the signedness there
is used to differentiate the underlying encoding. Accessing data.size
directly is usually an error, though we do access it directly in several
places when assuming the underlying encoding. Signedness warnings are
actually a good thing in that case.
So unsigned instead of signed. The original intention of using int32_t
was to hint that the sign bit should be reserved, but this may just
confuse if our leb128 encoding has a sign representation, which it does
not.
Yes these offered a tiny bit of typing savings, but they are used so
infrequently (really just lfsr_mdir_commit and friends) that they aren't
really worth it.
They also sort of break the object-related function pattern, since they
operated gdeltas (uint8_t[]) instead of grms (lfsr_grm_t). It's easy
enough to pass LFSR_GRM_DSIZE where needed.
This would probably only get worse if we add more gstate types.
This had no impact on code/stack. These functions were probably already
inlined.
The original motiviation was to make the gstate-related logic a bit more
coherent, but it turns out lfsr_grm_xorgrm is quite useful for
simplifying gstate handling in lfsr_mdir_commit.
As a plus it looks like we save a surprisingly amount of stack cost, but
I think this may just be a symptom of our tooling not being able to
understand shrinkwrapped function calls:
code stack
before: 33716 2832
after 33692 (-0.1%) 2808 (-0.9%)
This code is a bit tricky since we need to reference the current mdir to
know how to update other opened mdirs, but then also update the current
mdir, which could also be in the list of opened mdirs. I think a hear a
functional language user laughing in the distance...
code stack
before: 33764 2832
after: 33716 (-0.1%) 2832 (+0.0%)
I think this is a case where separating the logic out into distinct
functions does more harm than good, by making it harder to understand
how all the different moving parts interact.
This is especially important for lfsr_mdir_commit, since this is where
all atomic operations in the filesystem get tied together. Having atomic
updates complete in different functions was particularly concerning
since it carries some implicit requirements (must not error after!).
The end result is a cumbersome function, but at least internally
relatively straightforward in how the commit propagates through the
mtree/mroot chain and internal state.
---
The other benefit of inlining is better code deduplication, since we
can treat the mroot as a normal mdir until it triggers a split or
relocation.
We can also deduplicate the grm patching, though there may be a better
way to implement this. There are still some awkward bits in the logic.
code stack
before: 33856 2888
after: 33764 (-0.3%) 2832 (-2.0%)
We just don't use this error since we assert. Having it in the error
enum may give the wrong impression we return it at points.
If we even end up needing it, it can be readded to the list.
Hey if it's good enough for iterators (i), it's good enough for our
other traversals/iterators:
- iterator(?) -> i
- traversal -> t
- opened -> o
Expressions involving these variable were getting quite long. At least
now our common opened-list iterator can take only one line.
This reduces lfs.c by 41 lines (16851 -> 16810).
I do wonder if the use of "o" as a variable will limit my future
employment opportunities though.
We have bleafs (bleaves?) now, so the mleaf name just makes too much
sense. Even though it's used nowhere else outside of mid decoding, and
may be a bit confusing.
After all this time it feels weird to use a const lfs_t parameter, but
that's really what the mid/mleaf functions should take. These functions
are a bit of a special case as lfsr_mleafweight really wants to just be
a constant.
Code size did not change.
Some relatively-annoying states to check for:
- lfsr_attr_isnoop
- lfsr_attr_isinsert
And some accessors for marshalled pointers used by internal tags:
- lfsr_attr_grm
- lfsr_attr_mdir
- lfsr_attr_shrubcommit
- lfsr_attr_shrubtrunk
Christopher Haster