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.
Unreachable tag holes, null tags that _should_ be unreachable but
actually are reachable, are an unfortunate quirk to our alt tag
encoding. Because we only have an altgt, not altge, our "unreachable"
tag ends up encoded with an altgt 0, an alt, which you may notice, does
not guarantee unreachability.
Fortunately, tag 0, the null tag, should intentionally be unused. So as
long as we never lookup tag 0, nothing should break.
If you do lookup tag 0, you end up with spurious null tags, which can
complicate things.
The solution here is a tag_ = max(tag, 1) in lfsr_rbyd_lookupnext.
---
One interesting thing to note, as I was writing these tests I discovered
that setting tag=max(tag,1) in lfsr_rbyd_appendattr had no effect.
appendattr needs zip the rbyd tree to keep everything connected during
range removals, so tag=0/tag=1 both end up with the same tree.
So might as well drop the tag=max(tag,1) in lfsr_rbyd_appendattr.
A side effect of this, both before and after this commit, is that any
null tag holes created during range removals sort of stick around until
the next compaction.
---
Why altgt and not altge? altgt is the inverse of altle, requiring only
a single bit flip to flip between the two. And trust me, it would be
much more costly to make altle/altgt flips more complicated than a bit
flip.
---
Why altgt/altle and not altge/altlt? This is because our rbyds are
right-leaning, that is, lookups always find the requested rid+tag, or
the next smallest rid+tag.
Consider a simple tree:
<5
.----'|
>=2 |
|'-. |
1 2 5
What should lookup(3) return? If we are right-leaning, the answer
_should_ be 5. But we need to take the <5 branch to determine if there
is a hidden 3 or 4 in that subtree.
altgt/altle does not have that problem:
<=2
.----'|
>1 |
|'-. |
1 2 5
It might seem like you can workaround this by conservatively using the
neighbor +1 as the alt target, but this runs into tag overflow problems.
UATTR(0xff)+1 (0x057f+1) becomes UATTR(0x100) (0x0580) which is not
allowed due to reserving bit 7 for future subtype extensions.
Maybe you can workaround this workaround by using (tag+0x81)&~0x80
anywhere you need to increment (including lookupnext/iteration calls!),
but this becomes a bit of a mess. And there are still concerns about
overflows at the 0x77f boundary and 0xf7f boundary.
Like SUBWIDE, SUPWIDE allows for "mask-like" operation during rbyd
commits, where you replace an entire subrange of tags with a single tag.
- SUBWIDE - Replace all subtypes of the given suptype - Useful for
changing the subtype of an attr, for example replacing a BTREE with a
BSHRUB.
- SUPWIDE - Replace all suptypes of the given rid - Useful for changing
the suptype of an attr, for example replacing a REG file with an
ORPHAN file.
These are effectively the same modifier, just with different ranges.
One benefit is this simplifies mid-level operations a bit, rename,
remove, etc, and decreases the stack cost of the related attr lists.
Though this isn't on the hot-path, so not measurable:
code stack
before: 33956 2912
after: 33928 (-0.1%) 2912 (+0.0%)
But the real motivation for this change is to remove cases where
lfsr_mdir_commit needs to operate on multiple mids. There may be an API
simplification here.
We still have an lfsr_shrub_t, it's just a simple alias of lfsr_rbyd_t.
The only difference between these two structs was that lfsr_rbyd_t had
the eoff/cksum fields, to enable incremental commits, and lfsr_shrub_t
had the estimate field, to keep track of the current shrub estimate so
we evict before overflow.
Unfortunately C makes this overlap a bit annoying. We can either add a
union, making a mess of field accesses, or use probably problematic
casting of structs with common initial sequences.
Instead of dealing with this headache, I'm just going to shove the
shrub estimate into the rbyd's eoff field and ignore the name abuse.
In normal rbyd use, eoff does effectively contain the on-disk size of
the rbyd, so it's not too far from its intended use...
This does move our estimate to overlap the eoff field instead of the
cksum field, which means we need to be a bit more careful about setting
erased state for btrees. This adds a small code cost:
code stack
before: 33928 2912
after: 33956 (+0.1%) 2912 (+0.0%)
Shrubness should have always been a property of lfsr_rbyd_t.
You know you've made a good design decision when things just sort of
fall into place and the code somehow becomes cleaner.
The downside of this change is accessing rbyd trunks requires a mask,
which is annoying, but the upside is we don't need to signal shrubness
via extra booleans in internal functions anymore.
The funny thing is, the actual motivation for this change is was just to
free up a bit in our tag encoding. Simplifying some of the internal
functions was just a nice side effect.
code stack
before: 33940 2928
after: 33928 (-0.0%) 2912 (-0.5%)
So:
switch (cond) {
case 0:;
// first case
break;
case 1:;
// second case
break;
default:;
// default case
break;
}
This basically adopts our current label style for the case statements in
switch statements. It initially looks like quite a monstrosity, but I
think it does a good job at highlighting that case statements in C are
no safer than labels and gotos.
I would not use this style in a language with better scoping in switch
statements.
I'd prefer not to use switch statements, their scoping rules in C are
just too error-prone, and the compiler usually optimizes things out
anyways, but there are some places where switch statements are clearly
the correct organization -- state machines such as lfsr_traversal_read
for example.
If you're curious about the ':;' ending, this is used in our current
style for labels to avoid "declaration is not a statement" warnings.
Which I think is just a bit of leftover from C historically not having
mixed statements/declarations.
So:
x = (cond) ? yes : no;
Where there are always parentheses around the condition, even if not
required for disambiguity. Additional parentheses are always allowed,
but the parenthesized condition helps signal that a ternary operator is
coming earlier in the expression.
This style has grown on me as I think it helps code readability. It
reminds me of the required parentheses for if/while statements.
Might as well adopt codebase-wide.
If it's convenient, _and_ saves a bit of code, I don't really see the
reasoning to drop it.
At least added an LFSR_ATTR_IF macro to try to reduce the C expression
noise.
Also I'm keeping the explicit noop grows in lfsr_btree_commit. I think
it's a good decision to prevent accidental non-nooping in the future. If
these grows didn't noop, it wouldn't be an error, but a minor parasitic
drain on performance/wear. Not great...
code stack
before: 34052 2928
after: 33940 (-0.3%) 2928 (+0.0%)
So instead of using C's ternary operator everywhere:
(condition)
? LFSR_ATTR(rid, tag, delta, data)
: LFSR_ATTR_NOOP
Use incremental attr allocation instead:
lfsr_attr_t attrs[1];
lfs_size_t attr_count = 0;
if (condition) {
attrs[attr_count++] = LFSR_ATTR(rid, tag, delta, data);
}
LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t));
Incremental attr allocation is more flexible, allowing nested conditions
and conditions that span multiple attrs without sacrificing readability,
though at a verbosity cost.
We already need this for lfsr_btree_commit and lfsr_file_carve, adopting
it everywhere we need conditional attrs allows us to drop the noop attr
and avoid messy and hard-to-read C expressions.
This also changes the lfsr_btree_commit to explicitly omit noop grows.
We were relying on lfsr_rbyd_appendattr implicitly skipping these to
avoid unnecessary attr commits, but I think it's probably better to make
these noops explicit.
This does add some code cost though, I'm guessing sequential conditional
attrs landing at different offsets complicates code generation a bit:
code stack
before: 33940 2928
after: 34052 (+0.3%) 2928 (+0.0%)
Unfortunately we can't apply the valuable overflow assertions we use
elsewhere, classic C array arguments devolving into pointers before
sizeof. It's unfortunate there's no way to get this information even
for static-sized array arguments.
code stack
before: 33976 2944
after: 33940 (-0.1%) 2928 (-0.5%)
This is just too useful a namespace to not have in the low-level file
code.
This also replaces the ftree namespace with bshrub, which is a bit of a
more concrete term?
Note that some of the bshrub functions still take lfsr_file_t instead
of lfsr_bshrub_t. In _theory_ these could take 3 pointers (mdir+bshrub+
bshrub_), but this adds a surprising amount of code cost and we really
don't gain anything.
- Rely on rbyd lookup ENOENT now that that is cheap
- Adopt internal traverse_ function with shrub bool to indicate if root
should be included, similar to btree_commit_
Code changes:
code stack
before: 33956 2944
after: 33976 (+0.1%) 2944 (+0.0%)
We really shouldn't go to disk in cases like these, it's not worth the
code tradeoff. This led to concious decisions to avoid out-of-bound
lookups in higher-layers, which sort of defeats any benefit of this
potential optimization.
code stack
before: 33948 2944
after: 33956 (+0.0%) 2944 (+0.0%)
The size field in lfs_info doesn't really make sense for stat/dir_read
when the file is a directory. Still, we should probably set it to 0 os
it's not uninitialized.
Fortunately we were already setting size=0 in _most_ cases, this commit
is mostly just checking for size=0 in more test cases.
Christopher Haster