- 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%)
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%)
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%)
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
With lfsr_mdir_t being a logical cursor pointing to a specific metadata
entry in the on-disk mdir, we don't really need the mid to be provided
on every lookup call (may have jumped the gun a bit in the attr-list
changes).
In the rare case we need to lookup unrelated mids, we call always call
lfsr_rbyd_lookup on the underlying rbyd.
This saves a little bit of code/stack:
code stack
before: 33964 2896
after: 33852 (-0.3%) 2888 (-0.3%)
Changing insert tags to append seems to have broken insertion into named
btrees in a subtle way.
Consider what happens when we insert immediately before a bid that
splits the btree:
1. namelookup returns the right rbyd, with rid=-1
2. converting this into a bid gives us the left rbyd, with rid=weight
3. the commit to insert the bid ends up inserting into the left rbyd
This doesn't initially seem like an issue, both entries are effectively
the same right? Well, not when you have names. The split name tells you
what _follows_, so this unintentional flipping causes the new name to
get placed in the wrong bucket.
It's not clear if it's possible to fix this, at least not without
inverting the split names to indicate what precedes, but that's a step
too far.
This was not detected earlier because I disabled the low-level
rbyd/btree/mtree tests temporarily due to high porting cost. Guess that
goes to show there's a cost to deferring test ports for too long.
---
This issue, along with being inconsistencies between rids/bids and mids,
and being a relatively unintuitive pattern, is the final nail in the
coffin for insert tags inserting after.
Now, insert tags insert before, like in most other systems, and insert
tags in attr-list just have an implicit +1 before them to allow splits
in attr-lists to work.
This is not a pure revert, as some of the changes with all the code
moving around revealed some better detail-level ideas.
And yes, rbyd/btree tests are up to date now. Unfortunately the mtree
tests require a bit more work.
---
One thing definitely worth noting, btree merges were broken! A mistake
in the has-parent condition meant we were never attempting to merge
btrees!
This hid some bugs in the actual btree merge code caused by mixing the
implicit swap of child rbyds to deduplicate code paths with btree commit
now needing to track bid/rid separately from the attr-list.
This should be fixed now. Interesting to note this bug has been in
lfsr_btree_commit_ for a while now! I think ever since we switched to
using trunks for the has-parent check. We just haven't been merging
btree nodes at all. But since not-merging isn't technically an error,
it's difficult to test for.
Code changes:
code stack
before: 33808 2896
after: 33964 (+0.5%) 2896 (+0.0%)
Found a bug, and maybe a fundamental issue:
- The lfs_btree_lookupnext_ in lfsr_btree_commit_ no longer needs the
min32, since we never commit with bid pointing past the end of the
btree anymore.
This was mixing the unsigned min32 with our now-signed bid type,
causing the wrong btree leaf to be fetched when inserting at bid=-1 in
a non-empty btree.
Easy fix.
- lfsr_btree_commit_ with bid!=-1, rid=-1 (inserting at the beginning of
not-the-first rbyd) now actually appends to the leaf to the left of
the rbyd instead of inserting into the expected rbyd because of how
lfs_btree_lookup_ works.
Initially, this doesn't seem like it would be an issue, these should
be more-or-less equivalent, but this doesn't match
lfsr_btree_namelookup! This is a big problem!
This wasn't noticed because it's rare for the high-level tests to
trigger that many btree splits with names. Named btrees are only used
for the mtree, and we need mdirs to split before the mtree even splits
once.
Not an easy fix.
On the upside, these low-level tests continue to prove themselves
valuable, if tedious to maintain...
This is an interesting optimization made possible by our attr-lists now
only operating on one mid. We can now discard the entire attr-list based
on if that mid is in the commit's filter range.
Unfortunately, while I had hoped this would lead to more
simplifications, we can't really push this up through many functions:
- While this eager discard works for splits, lfsr_btree_commit can also
merge, which affects two separate bids. The attrs on these bids need
to be split over the new btree inner-nodes, so we end up still needing
filtering in lfsr_rbyd_appendattrs.
We could move the filtering up into lfsr_btree_commit, but would that
really gain anything?
- lfsr_mdir_commit_ needs the filter range because we leverage this in
higher-layers to for lfsr_mdir_commit_ to omit non--1 attrs when
committing to newly hollow mroots.
In theory it might still be possible to push this up into
lfsr_mdir_commit, but we would still need to unconditionally commit
during mdir splits to append the cksum. This ends up with duplicate
function calls which ends up annoyingly expensive. Though maybe there
is a conditional count trick that could avoid this?
At least the code changes are ok:
code stack
before: 33884 2896
after: 33808 (-0.2%) 2896 (+0.0%)
This unfortunately makes inserts inconsistent between rbyd/btrees:
insert(rid=-1) => rid=0
insert(bid=-1) => bid=0
insert(mid=0) => mid=0
But seems to integrate the best throughout the rest of the codebase:
- No awkward rid=-1 encoding in the mid, mid=1.2 => bid=1, rid=2
- No need to tweak mid encoding when writing grms to disk
- Behavior of unrelated files in the mdir behave consistently
irregardless of if our tag is an insert or not:
- mid' >= mid => mid'=mid'+delta
- mid' < mid => mid'=mid'
This is convenient because only the mid updates trigger tweaks of
unrelated mids, rbyds/btrees don't really have this problem.
- We already have to do a bit of tweaking in lfsr_mdir_namelookup, since
we're converting from "buckets" in the rbyd to ids we'd insert into.
Mainly namelookup of left-most name returns rid/bid=0, but for mdirs
should return mid=-1 (now mid=0):
left-most left-most+1 left-most+2
rid/bid: 0 0 1
mid (before): -1 0 1
mid (after): 0 1 2
I think this may be a reasonable compromise between allowing splits in
rbyd/btrees, and intuitive behavior for insertions in the mdirs.
That, or I've just been staring at this code for too long...
code stack
before: 33876 2896
after: 33888 (+0.0%) 2896 (+0.0%)
The recent change to internally track mids as mid=mid+1 leaked onto disk
through the grm. This is currently the only place we actually write mids
to disk.
The mid=mid+1 encoding is a bit of a hack and probably should not be the
actual on-disk representation, since there are other ways to encode this
internally.
I did try to write some tests for this, but because the bug is on both
the encoding and decoding side it's difficult without reading the mdir
directly. I only noticed with the dbg scripts started throwing random
errors. Fortunately a regression here is unlikely.
This atypical but not unreasonable behavior (most array insert functions
I've ran into like to insert _before_ the current index) makes split
commits no longer special behavior of appendattrs/commit, and seems to
fit better into rbyd append logic (though admittedly, some of the rbyd
append logic gets really weird with the whole right-leaning business).
Though this does come with a couple downsides:
- All rbyd-based data structures need to be able to represent a -1 id
so we can insert into the first id. This is not a problems for
rids/bids, but we need to tweak mids to support mid.rid=-1.
The best solution I could come up with was to just increment rid by
one, so, assuming mbits=8:
- mid=0x100 => bid=0x100, rid=-1
- mid=0x101 => bid=0x100, rid=0
- mid=-1 => bid=-1, rid=-1
- We need to be really careful with splits over our attr-list, since
these can line up between the rid create tags reference and other
following tags intended to stick to the new rid.
This required some special handling in lfsr_rbyd_appendattrs and
lfsr_mdir_commit__.
Other than that this change is quite promising, and removed what felt
like a bunch of hacks adjusting mids in lfsr_file_carve.
code stack
before: 33992 2904
after: 33868 (-0.4%) 2896 (-0.3%)
It turns out we never really need to commit to two unrelated rids in a
single commit. And some data structures, mainly btrees/bshrubs, don't
even allow commits to unrelated rids.
Well, sort of. There are some cases that seem to require unrelated rids,
but these are easy enough to work around:
1. btree/mdir splits/merges end up with two rids - but these either
converge or diverge from one rid, so as long as we assume sequential
inserts/deletes operate on the _neighboring_ rid, things work out.
2. grms/etc commit to mid=-1 irregardless of the file mid - but these
are also very special flags that are already handled differently to
manage the global state updates, nothing new was needed here.
So, in theory, we can move the rids out of the lfsr_attr_t struct and
infer and rid changes as we play out the attr-list, saving 4 bytes
(~17%) from every attr we allocate on the stack.
As a plus, we remove the need to manually calculate the changes to the
rid in the attr-list, reducing the likelihood of bugs here and saving a
decent amount of code.
Unfortunately the code/stack savings from this change were a bit
disappointing. The extra rid parameter in every commit function added
quite a bit of overhead, and we have to do some funky memmoves in
lfsr_file_carve to account for the new strict attr-list order:
code stack lfsr_attr_t
before: 33924 2912 24
after: 33992 (+0.2%) 2904 (-0.3%) 20 (-16.7%)
Still, this decreases the amount of code that can contain bugs, and more
closely matches the actual behavior of lfsr_btree/bshrub_commit.
Someone should really get around to updating the rbyd/btree/mtree
tests... Well, at least the non-internal (dir/dread/file/fwrite/etc)
tests are working.
Christopher Haster