The main change is moving away from applying gstate changes via special
attrs. Instead, gstate changes are applied implicitly, whenever the
relevant field in lfs_t differs from the gstate on-disk.
How do we recover from errors then? Well, we already need to track the
exact on-disk encoding of any gstate (grm_p) to avoid issues with minor
encoding differences, so if we encounter an error, we can revert any
changes to gstate by re-decoding the on-disk gstate. This is more
fragile: 1. all error paths in lfsr_mdir_commit need to revert gstate,
2. logic must not error between gstate updates and lfsr_mdir_commit, but
it gets the job done.
The benefit of this approach is that it's much easier to manipulate
gstate inside of lfsr_mdir_commit. No more hacky attr-list scanning to
patch grms mid-commit! It also in theory saves stack usage by dropping
an attr, but none of these attrs were on our stack hot-path.
Other gstate changes:
- Moved all grm adjustments into lfsr_mdir_commit.
This should deduplicate the messy grm adjust logic and make grms
easier to work with.
One hiccup though is the temporarily self-removing bookmark created in
lfsr_mkdir, which needs to create a grm referencing an mid that
doesn't exist yet. To work around this, lfsr_mdir_commit now
automatically creates grms for new bookmarks.
This might be a problem if we ever elide same-mdir mkdirs, but if so
we can solve that problem then.
- Dropped lfsr_data_t xoring, the added complexity wasn't really worth
it since all gstate should be small enough to buffer on the stack.
- Renamed several things:
- lfsr_grm_push/poprm -> lfsr_grm_push/pop
- lfsr_grm_isrm -> lfsr_grm_ispending
- grm_g -> grm_p
- grm.rms -> grm.mids
- Moved things around so grm/gstate logic is grouped together.
Unfortunately none of these attrs were on our stack hot-path, so no
stack savings. But thanks to the simpler logic, this does save quite a
bit of code:
code stack
before: 33514 2632
after: 33338 (+0.5%) 2640 (+0.3%)
- cat_count < 0 => single in-RAM buffer
- cat_count >= 0 => multiple concatenated datas
Note that cat_count=0 has the same effect whether or not you interpret
the cat as single or multiple datas.
Unlike, say, lfsr_data_t's size, the cat count does not mean the same
thing in both modes, so it doesn't really make sense to operate on the
count with bits masked off. This makes cat_count more like the signed
size/err union we use often.
The hope was better code generation for single/multiple cat checks. I
noticed some questionable code generation around checking the uint16_t's
sign bit and realized this might be a bit messy on 32-bit thumb. Sign
extension is in theory more common/cheaper on 32-bit ISAs, but I don't
know if the results are really conclusive:
before: 33538 2632
after: 33514 (-0.1%) 2632 (+0.0%)
This better matches test_dirs/test_files.
I guess the rule is singular for filesystem building blocks (test_rbyd,
test_btree, test_mtree, etc), plural for filesystem entries (test_dirs,
test_files, test_forphans, etc)?
So for example:
file->m.mdir.mid => file->o.mdir.mid
We already use "o" in opened-list iterations, so this is a bit more
consistent. And it doesn't increase the already obnoxious
file->o.mdir.rbyd.blocks[0] field names...
Now that lfsr_dir_t contains a single lfsr_opened_t, it makes sense for
lfsr_opened_t to always come first in lfsr_dir_t/lfsr_file_t for
consistency.
This also allows cheaper lfsr_file_t <-> lfsr_opened_t casts (noops),
which saves a bit of code:
code stack
before: 33582 2632
after: 33538 (-0.1%) 2632 (+0.0%)
This simplification comes from the observation that we don't actually
need to know the bookmark's mid to know if a given operation is in a
dir's range, just the dir's did. And since dids are immutable, we don't
need another opened-list entry or other shenanigans.
A dir's did is a bit harder to access, requiring a name lookup, but we
conveniently already fetch these in all relevant functions as a part of
path resolution.
This does mean more opened-list logic in the high-level functions:
function can zombie can create can remove
lfsr_mkdir y y n
lfsr_rename y y y
lfsr_remove y n y
lfsr_file_opencfg y y n
But I think this actually results in better code readability, since the
opened-list logic and high-level logic are closely related. I went ahead
and lifted the similar orphan/zombie opened-list logic up to this level
for this reason.
Unfortunately lifting this logic does result in a higher code cost, but
I think this is worth it for better readability and a significantly
reduced RAM cost for lfsr_dir_ts. Keep in mind these will probably
become very common for the future planned openat/*at functions:
code stack lfsr_dir_t
before: 33402 2632 80
after: 33582 (+0.5%) 2632 (+0.0%) 44 (-45.0%)
Also added a new test case, test_dread_read_rm_remkdir, to catch the
mistake of thinking the did is unique even when the dir is removed,
since that is now a concern.
We use the lfsr_attr_t struct for multiple purposes now, including some
situations where it holds the total weight, not the delta weight.
"delta" is also getting increasingly overloaded in littlefs, referring
also to offset changes ("d"), and gstate deltas...
Because of the invasive linked-lists, this was a bit more complicated
than the related move in lfs_t. But we already have similar
field-relative offsets in lfsr_dir_t for the dir + bookmark mdirs.
Added some helpers to help with this:
- lfsr_opened_dir
- lfsr_opened_constdir
- lfsr_opened_bookmark
- lfsr_opened_constbookmark
- lfsr_opened_file
- lfsr_opened_constfile
Unfortunately this resulted in less savings than in lfs_t, and actually
costs us code, likely because of how often we go from lfsr_file_t <->
lfsr_opened_t:
code stack
before: 33358 2632
after: 33402 (+0.1%) 2632 (+0.0%)
- Removed no longer used fields.
- Commented out related field asserts in lfs_init.
- Commented out pre-lfsr structs and function decls.
- Moved cfg to the first field in lfs_t.
Note that most of the code saves actually came from that last point.
Moving lfs.cfg, probably the currently most accessed field, resulted in
a surprising amount of code savings:
code stack lfs_t
before: 33702 2640 220
after+cfg last: 33592 (-0.3%) 2632 (-0.3%) 164 (-25.5%)
after+cfg first: 33358 (-1.0%) 2632 (-0.3%) 164 (-25.5%)
Maybe we should take a more rigorous/analytical approach to field
placement?
These seem fitting here, even if the test defines aren't "real defines".
The duplicate expressions should still be side-effect free and easy to
optimize out.
This should also avoid future lfs_min32 vs intmax_t issues.
When we introduced erased-state agnostic rbyd cksums, we added a cksums
to the dbg scripts since their cksums were actually useful now.
Unfortunately this missed the explicit/hacky Rbyd constructions in
dbglfs.py used to render file structs. Fixed by falling by using
cksum=0 in these cases.
This would have been more valuable if the extra lfsr_data_t stack
allocation (12 bytes) wasn't already unioned with the btree's encoding
buffer allocation (18 bytes):
code stack
before: 33714 2640
after: 33702 (-0.0%) 2640 (+0.0%)
Oh well, this still might save some stack in the future if things shift
around.
A much requested feature, this allows much finer control of how RAM is
allocated for the system.
It was difficult to introduce this in previous versions of littlefs due
to how we steal caches during certain file operations, but now we don't
do that and treat the caches much more transparently.
Managing separate cache sizes does add a bit of code, but this is well
worth the potential for RAM savings due to increased flexibility:
code stack
before: 33656 2632
after: 33714 (+0.2%) 2640 (+0.3%)
Also interesting to note this reduces alignment requirements for the
rcache/pcache, since they don't need to share alignment, and completely
removes any alignment requirement from the file buffers.
This limits the maximum size of a file, which is also implies the
maximum integer size required to mount.
The exact name is a bit of a toss-up. I originally went with size_limit
to avoid confusion around if file_limit reflected the file size or the
number of files, but since this ends up mapping to lfs_off_t and _not_
lfs_size_t, I think size_limit may be a bit of a bad choice.
An easy mistake to make, we were incorrectly checking the non-staging
shrub to see if our shrub had been copied over. Copying over the shrub
updates the staging shrub, so this wasn't actually doing anything
useful, resulting in a bunch of duplicate shrubs.
The fix is to use the staging shrub.
This was found thanks to test_fsync_wwrr, but only after bumping our
fragment_size up from cache_size (16 bytes) -> block_size/8 (512 bytes).
I'm guessing because this allowed our shrubs to be more overcommitted.
The concern with block_size/4 is that it limits fragments to a single
fragment per-block. But while this may be inefficient, it's technically
not wrong, and may still work with other metadata (bptrs, file names,
uattrs, etc) taking up the remaining space.
This deserves benchmarking, but even if this ends up being a terrible
configuration, we should just discourage this via good defaults and
documentation.
Also added related asserts to lfs_init.
Note the fragment_size <= block_size/8 limit is to avoid wasteful corner
cases where only one fragment can fit in a block. The shrub_size <=
block_size/4 limit is looser because of how shrubs temporarily
overcommit.
As for the other limits, inline_size is bounded by shrub_size, and
crystal_thresh technically doesn't have a limit, though values >
block_size stop having an effect.
This matches how diff percentages are rendered, and simplifies the
internal table rendering by making Frac less of a special case. It also
allows for other type notes in the future.
One concern is how all the notes are shoved to the side, which may make
it a bit harder to find related percentages. If this becomes annoying we
should probably look into interspersing all notes (including diff
percentages) between the relevant columns.
Before:
function lines branches
lfsr_rbyd_appendattr 230/231 99.6% 172/192 89.6%
lfsr_rbyd_p_recolor 33/34 97.1% 11/12 91.7%
lfs_alloc 40/42 95.2% 21/24 87.5%
lfsr_rbyd_appendcompaction 54/57 94.7% 39/42 92.9%
...
After:
function lines branches
lfsr_rbyd_appendattr 230/231 172/192 (99.6%, 89.6%)
lfsr_rbyd_p_recolor 33/34 11/12 (97.1%, 91.7%)
lfs_alloc 40/42 21/24 (95.2%, 87.5%)
lfsr_rbyd_appendcompaction 54/57 39/42 (94.7%, 92.9%)
...
Previously, with -d/--diff, we would only show non-zero percentages. But
this was ambiguous/confusing when dealing with multiple results
(stack.py, summary.py, etc).
To help with this, I've switched to showing all percentages unless all
percentages are zero (no change). This matches the -d/--diff row-hiding
logic, so by default all rows should show all percentages.
Note -p/--percent did not change, as it already showed all percentages
all of the time.
Noticed weird slowness when summarizing test results by suite vs case.
Turns out the way we accumulate results by overloading Python's __add__
quickly leads to O(n^2) behavior as we repeatedly concatenate
increasingly large lists.
Instead of doing anything sane, I've added a second, immutable length to
each list such that we can opportunistically reuse/mutate/append lists
in __add__. The end result should be O(n) most of the time.
Observe:
lines bytes
test.csv: 537749 64551874 62MiB
./scripts/summary.py test.csv -ftest_time -S
before after
-bcase: 0m51.772s 0m9.302s (-82.0%)
-bsuite: 10m29.067s 0m9.357s (-98.5%)
The main idea here is that diverse tests are better than many similar
tests.
Sure, if we throw fuzz tests at the system all day we'll eventually find
more bugs, but if a developer is in the loop that time is going to be
better spent writing specific tests targeting the fragile parts of the
system.
And don't worry, we can still throw fuzz tests at the system all day by
specifying explicit seeds with -DSEED=blah.
Changes:
- Limited dir-related powerloss fuzz testing to N <= 16.
These tests were the biggest culprit of excessive test runtime,
requiring O(n^2) redundant operations to recover from powerlosses
(they just replay the full sequence on powerloss).
- As a tradeoff, bumped most fuzz tests to a minimum of 20 seeds.
The big exception being the test_fwrite tests, which are heavily
parameterized and already take the most time to run. Each parameter
combination also multiplies the effective number of seeds, so
increasing the number of base seeds will probably have diminishing
returns.
- Limited test_fwrite_reversed to SIZE <= 4*1024*CHUNK.
Writing a file backwards is just about the worst way you could write a
file, since all buffering/coalescing expect writes to eventually make
forward progress. On the flip side, because it's uncommon, writing a
file backwards is also a great way to find bugs. But at some point a
compromise needs to be made.
Impacted test runtimes:
case otime ntime dtime
test_btree_push_fuzz 0.3 0.5 +0.2 (+60.2%)
test_btree_push_sparse_fuzz 0.4 3.3 +2.9 (+720.4%)
test_btree_update_fuzz 0.4 0.9 +0.6 (+141.6%)
test_btree_update_sparse_fuzz 0.5 4.5 +4.1 (+857.4%)
test_btree_pop_fuzz 0.6 2.3 +1.7 (+314.7%)
test_btree_pop_sparse_fuzz 1.2 5.7 +4.4 (+356.2%)
test_btree_split_fuzz 0.5 1.4 +0.8 (+150.2%)
test_btree_split_sparse_fuzz 0.4 5.6 +5.1 (+1163.2%)
test_btree_find_fuzz 0.5 0.7 +0.2 (+50.7%)
test_btree_find_sparse_fuzz 1.0 3.0 +2.0 (+189.8%)
test_btree_traversal_fuzz 0.6 2.3 +1.6 (+260.4%)
test_dirs_mkdir_many 3.3 2.1 -1.3 (-37.8%)
test_dirs_mkdir_many_backwards 3.5 2.1 -1.4 (-39.9%)
test_dirs_mkdir_fuzz 115.3 106.4 -8.9 (-7.7%)
test_dirs_rm_many 283.9 76.8 -207.0 (-72.9%)
test_dirs_rm_many_backwards 216.1 80.6 -135.5 (-62.7%)
test_dirs_rm_fuzz 647.0 68.5 -578.5 (-89.4%)
test_dirs_mv_many 14.2 15.4 +1.1 (+7.9%)
test_dirs_mv_many_backwards 16.5 14.5 -2.1 (-12.5%)
test_dirs_mv_fuzz 1932.5 156.7 -1775.8 (-91.9%)
test_dirs_general_fuzz 561.9 74.5 -487.4 (-86.7%)
test_dread_recursive_rm 336.6 46.2 -290.4 (-86.3%)
test_dread_recursive_mv 55.5 44.6 -11.0 (-19.8%)
test_fsync_rrrr_fuzz 0.4 0.3 -0.1 (-18.4%)
test_fsync_wrrr_fuzz 8.0 12.4 +4.5 (+56.0%)
test_fsync_wwww_fuzz 13.2 33.4 +20.2 (+152.6%)
test_fsync_wwrr_fuzz 5.4 50.9 +45.5 (+841.6%)
test_fsync_rwrw_fuzz 2.4 8.4 +6.0 (+253.9%)
test_fsync_rwrw_sparse_fuzz 3.2 7.5 +4.2 (+129.9%)
test_fsync_rwtfrwtf_sparse_fuzz 6.1 8.5 +2.4 (+39.3%)
test_fsync_drrr_fuzz 11.8 9.2 -2.6 (-21.8%)
test_fsync_wddd_fuzz 9.3 11.9 +2.6 (+28.0%)
test_fsync_rwdrwd_fuzz 1.6 33.1 +31.5 (+1963.4%)
test_fsync_rwdrwd_sparse_fuzz 0.3 1.8 +1.4 (+418.8%)
test_fsync_rwtfdrwtfd_sparse_fuzz 0.3 1.1 +0.8 (+260.2%)
test_fwrite_reversed 728.5 345.2 -383.3 (-52.6%)
TOTAL 7587.5 3792.3 -3795.2 (-50.0%)
Now that we have ~20 minutes of tests, it's good to know _why_ the tests
take ~20 minutes, and if this time is being spent well.
This adds the field test_time to test.py's -o/--output, which reports
the runtime of each test in seconds. This can be organized by suite,
case, etc, with our existing csv scripts.
Note I've limited the precision to only milliseconds (%.6f).
Realistically, this is plenty of precision, and with the number of tests
we have extra digits can really add up!
lines bytes
test.csv before: 525593 58432541 56MiB
test.csv full precision: 525593 (+0.0%) 69817693 67MiB (+19.5%)
test.csv milli precision: 525593 (+0.0%) 63162935 60MiB (+8.1%)
It still takes a bit of time to process this (50.3s), but now we can see
the biggest culprits of our ~20 minute test time:
$ ./scripts/summary.py test.csv -bcase -ftest_time -S
case test_time
...
test_fwrite_hole_compaction 74.4
test_fwrite_incr 109.7
test_dirs_mkdir_fuzz 115.3
test_fwrite_overwrite_compaction 132.4
test_rbyd_fuzz_append_removes 134.0
test_rbyd_fuzz_mixed 136.3
test_rbyd_fuzz_sparse 137.4
test_fwrite_w_seek 144.1
test_rbyd_fuzz_create_deletes 144.8
test_dirs_rm_many_backwards 208.4
test_dirs_rm_many 273.8
test_fwrite_fuzz_unaligned 283.2
test_dread_recursive_rm 316.7
test_fwrite_fuzz_aligned 551.0
test_dirs_general_fuzz 552.8
test_dirs_rm_fuzz 632.7
test_fwrite_reversed 719.0
test_dirs_mv_fuzz 1984.8
TOTAL 7471.3
Note this machine has 6 cores, 12 hthreads, 7471.3/60/6 => 20.8m, which
is why I don't run these tests single threaded.
It's really frustrating that it's impossible to create an uninitialized
expression with the scope of a compound-literal...
(I'm going to ignore that this is technically possible with alloca.)
The lack of uninitialized compound-literals forces each of our attribute
lists to make a decision: 1. Use an implicit buffer and pay for
zero-initialization? or 2. Use an explicit buffer, adding code noising
and risking out-of-date buffer sizes.
As a compromise, this commit adopts explicit buffers in most of the
low/mid-level layers. Where the code is already pretty noisy, but also
heavily scrutinized and iterated over to reduce code/stack costs. This
leaves the high-level layers with the hopefully safer and more readable
implicit buffers.
You can see this zero initializing has a surprisingly high code cost,
for what is otherwise a noop:
code stack
before: 33828 2632
after: 33656 (-0.5%) 2632 (+0.0%)
Now it is fit for purpose and can replace the explicit tag comparison +
assert in lfsr_rbyd_appendattr. Previously we had to check if delta==0,
but now we just assert that delta!=0 is invalid for noops.
Unfortunately this added a couple bytes of code. The disassembly for
lfsr_rbyd_appendattr is all shuffled up, so I guess this is just
compiler noise. At least it's better than an explicit delta check:
code stack
before: 33820 2632
check delta: 33832 (+0.0%) 2632 (+0.0%)
assert delta: 33828 (+0.0%) 2632 (+0.0%)
Turns out temporary struct-literals have a slightly better code/stack
footprint than array-literals. I guess because nuances around arrays in
C can cause problems for optimization passes?
This makes forcing lvalues for macro consistency much more appealing:
code stack
sometimes rvalues: 33780 2640
array lvalues (before): 33868 (+0.3%) 2640 (+0.0%)
struct lvalues (after): 33820 (+0.1%) 2632 (-0.3%)
I think what may be going on with the unexpected stack cost related to
struct passing, is something to do with scoping and how it interacts
with function inlining + shrink wrapping.
Compound-literals have a scope limited by the current statement, and
while temporary structs _should_ have a scope limited to the current
expressions, maybe this scope is getting messed up due to function
inlining?
Still smells like a compiler bug, but if this is true, wrapping the
struct-generating function calls with compound-literals should be more
robust at preventing unexpected stack increases in the future.
As a plus, this makes all LFSR_DATA/ATTR macros lvalues, which is nice
for consistency.
---
Unfortunately, it does seem like GCC 11 is not able to elide moving
compound-literals all that well. Repeatedly nesting trivial
compound-literals results in a measurable increase in code cost, even
though it should theoretically be a noop with optimizations.
This results in an unfortunate code size increase:
code stack
before: 33780 2640
after: 33868 (+0.3%) 2640 (+0.0%)
But at some point you have to give up trying to work around
insufficiencies in the compiler. I'll take 100 bytes of code over 100
bytes of stack any day.
This turned into a sort of system-wide refactor based on learned
knowledge of what we can do with lfsr_attr_t.
The big changes:
- Reverted LFSR_ATTR to mainly take lfsr_data_t again, keeping
lfsr_data_t as the default data representation in the codebase.
Now that we know
LFSR_ATTR_CAT_ still provides concatenation mechanics, and LFSR_ATTR_
provides a way to edit in-flight lfsr_attr_ts.
- Dropped lfsr_cat_t, replaced with explicit const void* + uint16_t,
tried to limit to low-level operations and prefer passing aroud
lfsr_attr_t and lfsr_data_t at a high-level.
Note this cat + cat_count pair is quite similar to the common attrs +
attr_count and buffer + size arguments.
- Adopted lfsr_attr_t more in mid-level functions, lfsr_rbyd_appendattr,
lfsr_rbyd_appendcompactattr, lfsr_file_carve, etc. This is a bit more
ergonomical, allows for use of LFSR_ATTR* macros, and in theory might
even save a bit of stack.
Unfortunately this seems to have resulted in a net hit to code cost,
though I still think it's worth it for the internal ergonomics:
code stack
before: 33652 2624
after: 33780 (+0.4%) 2640 (+0.4%)
Investigating further suggests this may just be the result of compiler
noise and changes to argument placement. lfsr_attr_t does touch a lot of
code...
It's interesting to note the adoption of lfsr_attr_t in
lfsr_rbyd_appendattr* and friends prevents their transformation into
.isra functions, though this doesn't seem to impact code cost too much:
function (5 added, 5 removed) osize nsize dsize
lfsr_cat_size - 48 +48 (+100.0%)
lfsr_file_carve - 1600 +1600 (+100.0%)
lfsr_rbyd_appendattr - 2120 +2120 (+100.0%)
lfsr_rbyd_appendattr_ - 244 +244 (+100.0%)
lfsr_rbyd_appendcompactattr - 68 +68 (+100.0%)
lfsr_rbyd_appendcompactrbyd 144 152 +8 (+5.6%)
lfsr_file_truncate 298 314 +16 (+5.4%)
lfsr_mdir_commit__ 1056 1112 +56 (+5.3%)
lfsr_mdir_compact__ 502 526 +24 (+4.8%)
lfsr_rbyd_appendattrs 132 138 +6 (+4.5%)
lfsr_file_fruncate 386 402 +16 (+4.1%)
lfsr_data_frombtree 84 86 +2 (+2.4%)
lfsr_rbyd_appendcksum 512 520 +8 (+1.6%)
lfsr_file_opencfg 572 580 +8 (+1.4%)
lfsr_rename 608 616 +8 (+1.3%)
lfsr_mkdir 500 504 +4 (+0.8%)
lfsr_bd_prog 278 280 +2 (+0.7%)
lfsr_mdir_commit 2364 2360 -4 (-0.2%)
lfsr_bshrub_commit 716 712 -4 (-0.6%)
lfsr_file_sync 526 514 -12 (-2.3%)
lfsr_file_flush_ 1868 1820 -48 (-2.6%)
lfsr_remove 456 436 -20 (-4.4%)
lfsr_fs_fixgrm 168 160 -8 (-4.8%)
lfsr_cat_size.isra.0 42 - -42 (-100.0%)
lfsr_file_carve.isra.0 1596 - -1596 (-100.0%)
lfsr_rbyd_appendattr.isra.0 2088 - -2088 (-100.0%)
lfsr_rbyd_appendattr_.isra.0 232 - -232 (-100.0%)
lfsr_rbyd_appendcompactattr.isra.0 56 - -56 (-100.0%)
TOTAL 33652 33780 +128 (+0.4%)
If we have control of the struct, such as in lfsr_data_t and lfsr_cat_t,
moving the common tag outside of the union avoids naming ambiguities.
Counter-example: This doesn't work for lfsr_bshrub_t, since the contents
of that union are also used as separate types elsewhere. Fortunately the
common initial sequence union rules kick in here.
No code changes, which is good:
code stack
before: 33652 2624
after: 33652 (+0.0%) 2624 (+0.0%)
Now that lfsr_attr_t is "small", or at least the same size as
lfsr_data_t, it makes sense to change the helper functions to take
lfsr_attr_t by value for consistency. These should all be inlined
anyways.
It's interesting to note there _are_ appendattr/progattr functions, but
these don't take lfsr_attr_t directly since we usually do some
last-minute modification to the attr's weight/tag.
Cost cost is mostly unchanged, actually shaves off a few bytes, which is
a good sign:
code stack
before: 33664 2624
after: 33652 (-0.0%) 2624 (+0.0%)
Compound-literals weren't the culprit after all! It was... RVO
interactions with inlined function arguments?
To be honest I still don't quite understand what's going on, but I
present to you this madness:
code stack
before: 33664 2624
after: 33664 (+0.0%) 2624 (+0.0%)
I think I'm understanding a bit more how RVO interacts with inline
functions. And by that I mean I'm learning that the way RVO interacts
with inline functions is unfortunately very cursed...
Just take a look at this diff. This change should be a noop. But somehow
it saves 200 bytes of RAM:
code stack
before: 33684 2824
after: 33664 (-0.1%) 2624 (-7.1%)
I think what's happening is passing the result of lfsr_data_from* into
lfsr_data_cat is somehow preventing RVO, because the parameter would
need to be copied into the right argument slot? (argument registers?)
But we really don't need a copy, because lfsr_data_cat should end up
inlined. By inserting a compound literal, we force RVO, and all of these
unnecessary copies get cleaned up after lfsr_data_cat is inlined.
Keep in mind, in a perfect world, lfsr_data_cat should be a noop.
But I could be wrong about all of this. It's not really clear what the
compiler is doing, and I haven't dived that far into the disassembly...
The name is not super important, but note lfsr_data_cat matches
lfsr_attr_cat, which is a nice bit of consistency.
The main change here is the adoption of correct field assignments
instead of a hacky cast forcing lfsr_data_t -> lfsr_cat_t. Tests were
passing even with optimizations, but I was concerned about the longevity
of this approach.
As a plus, we can actually assert on size fitting into a uint16_t thanks
to the inline function.
Unfortunately, this creates a surprising stack penalty:
code stack
before: 33756 2624
after: 33684 (-0.2%) 2824 (+7.6%)
I've also played around with instead reverting lfsr_data_from* ->
lfsr_cat_from*, and providing the inverse lfsr_cat_data, but nothing
gets us quite back to LFSR_CAT_DAT stack:
code stack
before: 33756 2624
lfsr_data_cat: 33684 (-0.2%) 2824 (+7.6%)
lfsr_cat_data: 33872 (+0.3%) 2736 (+4.3%)
This needs more investigation. Unfortunately I don't think we can revert
this, since correctness wins over code/stack costs...
Now that compound-literals have been identified as the culprit, we can
actually adopt this smaller lfsr_attr_t representation without a random
code/stack increase.
This limits lfsr_cat_t's size field to 16-bits (15-bit size + 1-bit
for concatenated datas), allowing simple small attrs (the most common)
to save a word of RAM:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---.
| tag |-----------+--->| tag |c|size |
'---+---' | +---+---+---+---+
.-|--->| delta |
lfsr_srid_t | | +---+---+---+---+
.---+---+---+---. | | .->| ptr |
| delta |-' | | '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
|c|size |-----------' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
The non-trivial mapping of lfsr_cat_t to lfsr_attr_t does mean a bit
more complexity on lfsr_cat_t access, but now that we figured out the
compound-literal cost it seems the compiler is able to mostly elide
these.
The end result is some nice stack savings:
code stack
before: 33812 2712
after: 33756 (-0.2%) 2624 (-3.2%)
I've been fiddling around with our LFSR_ATTR macro to try to understand
why making it an inline function costs so much, and it seems like it's
not actually the inline function, but the compound literal that is the
problem. Specifically, returning a compound literal from an inline
function results in surprisingly poor code/stack costs!
I don't really know why this happens. Compiler bug/oversight related to
lvalues/rvalues? Compound literals interfering with RVO? Unsure.
I tried a few other struct initializers just in case it was related to
constness, but it seems the problem is the compound literal:
Inlined comp-lit:
return (lfsr_attr_t){tag, delta, cat};
Inlined const comp-lit:
return (const lfsr_attr_t){tag, delta, cat};
Inlined no-init:
lfsr_attr_t attr;
attr.tag = tag;
attr.delta = delta;
attr.cat = cat;
return attr;
Inlined init:
lfsr_attr_t attr = {tag, delta, cat};
return attr;
Code/stack sizes:
code stack
macro (before): 33852 2776
inline comp-lit: 34140 (+0.9%) 2760 (-0.6%)
inline const comp-list: 34140 (+0.9%) 2760 (-0.6%)
inline no-init (after): 33812 (-0.1%) 2712 (-2.3%)
inline init: 33812 (-0.1%) 2712 (-2.3%)
The good news is this at least offers a route forward for crammed 15-bit
attrs.
I guess we should also go reasses other uses of compound literals in the
codebase...
When we're not dealing with cats/attrs, it seems useful to still have
lfsr_data_from* functions that return our general purpose lfsr_data_t
type.
Added LFSR_CAT_DAT to cheaply convert from lfsr_data_t -> lfsr_cat_t
(at least for simple buffers), and used this to keep the LFSR_CAT_*
macros, which are useful for attr-list construction.
Unfortunately, because lfsr_data_t is 3-words vs lfsr_cat_t 2-words,
this does add both code and stack cost:
code stack
before: 33672 2744
after: 33852 (+0.5%) 2776 (+1.2%)
It's interesting to note this is _not_ because of any LFSR_CAT_* usage!
I tested this explicitly and lfsr_data_from* -> LFSR_CAT_DAT adds no
cost over the previous lfsr_cat_from* functions. A win for GCC. This
cost only comes from the direct usage of the returns lfsr_data_t types
in our grm handling and branch -> btree encoding.
Still it's an annoying cost... Maybe this should be reverted? The
nuances of lfsr_cat_t vs lfsr_data_t is a bit annoying.
So, for example, these are equivalent:
lfsr_cat_t cat = LFSR_CAT_BPTR(bptr);
uint8_t buf[LFSR_BPTR_DSIZE];
lfsr_cat_t cat = LFSR_CAT_BPTR_(bptr, buf);
The first leads to more readable code, but of course sometimes you need
explicit memory allocations.
This replaces lfsr_cat_frombptr, etc, though those functions are still
available. This name change is more relevant for LFSR_CAT_DATA/DATAS,
which involve bit more complicated macros.
I think this was becoming too over-engineered. That and if you're
fighting the compiler too much, sometimes it's best to just let the
compiler win.
There is also the minor benefit of not needing to worry about uint16_t
overflows in lfsr_cat_t. One concern is it's easy for missed lfsr_data_t
indirections to go unnoticed.
This reverts lfsr_attr_t from 3 words -> 4 words, but allows easy
mapping to the lfsr_cat_t part of the lfsr_attr_t:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---. . . .---+---+---+---.
| tag |--------------->| tag | | | tag | |
'---+---' +---+---+---+---+ +---+---+---+---+
.----->| delta | | delta |
lfsr_srid_t | +---+---+---+---+ +---+---+---+---+
.---+---+---+---. | .--->|c| size | | cat |
| delta |-' | +---+---+---+---+ | |
'---+---+---+---' | .->| ptr | | |
| | '---+---+---+---' . . '---+---+---+---'
lfsr_cat_t | |
.---+---+---+---. | |
|c| size |---' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
This doesn't impact stack as much as you would expect due to compiler
overhead around inline functions operating on multi-word structs:
before 15-bit: 33672 2776
best 15-bit: 33900 (+0.7%) 2736 (-1.4%)
prev 15-bit: 34486 (+2.4%) 2912 (+4.9%)
after: 33672 (+0.0%) 2744 (-1.2%)
I think the only big thing not reverted was switching lfsr_cat_t from
storing the sum of the lfsr_data_ts sizes to just storing the number of
lfsr_data_ts. This is cheaper to initialize (because inline functions
are weirdly costly!), cheaper to iterate over, and we don't actually
need lfsr_cat_size that often, unlike lfsr_data_size.
Eventually we'll probably need to revisit this to optimize 31/15-bit,
etc, littlefs configurations. But that's a problem for another day...
The idea here was to carve out explicit space for the lfsr_attr_t tag in
lfsr_cat_t, so that when we create lfsr_attr_ts we don't need multiple
references to lfsr_cat_t and can revert back to a macro:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---. . . .---+---+---+---.
| tag |-------. .----->| delta | | delta |
'---+---' | | +---+---+---+---+ +---+---+---+---+
'-|-+--->| tag |c|size | | cat |
lfsr_srid_t | | +---+---+---+---+ | |
.---+---+---+---. | | .->| ptr | | |
| delta |-' | | '---+---+---+---' . . '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
| (tag) |c|size |---' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
But as a part of creating the lfsr_attr_t we need to mutate the tag in
lfsr_cat_t, which ended up still needing a static inline function, and
ended up making the code/stack size even worse!
code stack
before: 33900 2736
after: 34486 (+1.7%) 2912 (+6.4%)
I was hoping to leverage the last-update rule of C99's designated
initializers, but this weird rule didn't quite work how I expected. The
compiler is free to omit earlier struct initializers, even if a later
initializer only partially initializes the struct.
Another interesting observation about lfsr_cat_t: We rarely actually
need to represent the full range of data:
- lfsr_file_write:block - Block writes call lfsr_bd_prog directly, don't
need to be represented as attrs.
- lfsr_file_write:fragment - Fragments already need the full
concatenated representation because of, uh, potential concatenation.
- lfsr_file_sync:sprouts - Inlined sprouts may use the full range.
Fortunately we can union an lfsr_data_t with the btree buffer, so
no extra stack cost.
- lfsr_getuattr/sattr (planned) - User/sys attributes may use the full
range. But these will probably not be on the stack hot-path.
Most attrs that use the simple buffer lfsr_cat_t representation are used
to encode internal structs, such as leb128s, ecksum, bptr, mptr, etc.
The largest of these right now is our bptr encoding, at 21 bytes, so
these easily fit in a short or a byte.
The choice of 15-bits (reserving one bit for cat/buf representation), is
convenient as it allows us to fit the cat size next to our 16-bit
lfsr_tag_t for free in 32-bit aligned systems:
lfsr_attr_t
.---+---+---+---.
| tag |c|size |
+---+---+---+---+
| delta |
+---+---+---+---+
| ptr |
'---+---+---+---'
Unfortunately, it seems like C _really_ wants to fight us on this one.
Not really because of the struct packing, but because of how we want
lfsr_attr_t to interact with lfsr_cat_t:
lfsr_tag_t lfsr_attr_t
.---+---. .---+---+---+---.
| tag |-----------+--->| tag |c|size |
'---+---' | +---+---+---+---+
.-|--->| delta |
lfsr_srid_t | | +---+---+---+---+
.---+---+---+---. | | .->| ptr |
| delta |-' | | '---+---+---+---'
'---+---+---+---' | |
| |
lfsr_cat_t | |
.---+---+---+---. | |
|c|size |-----------' |
+---+---+---+---+ |
| ptr |-----'
'---+---+---+---'
Initializing two fields with one argument is frustratingly impossible in
C99 unless you want to duplicate the argument tree, which we
_definitely_ don't want to do because this includes the actual data
encoding steps.
The only option is to use a static inline function. You might say "oh,
but static inline costs the same as a macro". But no. Switching to a
static inline function heavily penalizes this approach (6% of stack!):
code stack
macro: 33672 2776
inline: 34512 (+2.5%) 2952 (+6.3%)
after: 33900 (+0.7%) 2736 (-1.4%)
At least this does result in net stack savings, even with the inline
function penalty.
I'm committing this temporarily mostly just to record some _very_
interesting code/stack measurements.
This explores replacing the LFSR_ATTR macro with an inline function that
does the same thing:
#define LFSR_ATTR(_tag, _delta, _cat) \
((const lfsr_attr_t){_tag, _delta, _cat})
vs:
#define LFSR_ATTR(_tag, _delta, _cat) \
lfsr_attr(_tag, _delta, _cat)
static inline lfsr_attr_t lfsr_attr(
lfsr_tag_t tag, lfsr_srid_t delta, lfsr_cat_t cat) {
return (lfsr_attr_t){tag, delta, cat};
}
The motivation for this is to eventually support more complex
lfsr_attr_t layouts. Specifically, it would be nice if we could break up
the lfsr_cat_t into separate size/ptr fields. Unfortunately we can't
declare temporaries in macros (I wish we had statement expressions), and
we really don't want to duplicate the entire cat tree, so an inline
function seems like the only way to accomplish this...
But static inline functions have the same cost as a macro you say?
No. This assumes a perfect compiler. And it's pretty unfair to compiler
developers to expect a perfect compiler.
To be fair, this is an extremely harsh test. We use LFSR_ATTR _heavily_,
which is why it's getting this much scrutiny. lfsr_attr also both takes
in a 2-word struct, and returns a _4_-word struct, which probably makes
things messy.
Still, the results are concerning:
code stack
macro: 33672 2776
inline: 34512 (+2.5%) 2952 (+6.3%)
always_inline: 34512 (+2.5%) 2952 (+6.3%)
noinline: 33920 (+0.7%) 2888 (+4.0%)
Measured with GCC 11 -mthumb -Os. I also measured with
__attribute__((always_inline/noinline)) just to see how that affected
things.
With LFSR_CAT_DATAS for explicit arrays of datas, and this biggest use
of concatenated data being a rather explicit construction in
lfsr_file_carve, I don't think we really need LFSR_CAT_CAT.
The only non-hacky use was to define LFSR_CAT_NAME. But we know names
always use exactly 2 datas, so this might as well use LFSR_CAT_DATAS.
I am going to use this soapbox to complain a bit about compound struct
literals. Why do we need an array declaration to elevate temporary
structs to automatic storage duration? I wish you could init a compound
literal with the struct itself...
✗ &f()
✓ &(uint32_t){f()}
✓ (uint32_t[]){f()}
✗ &f()
✗ &(lfsr_data_t){f()} :(
✓ (lfsr_data_t[]){f()}
Some hacky compound array literals were needed to replace the hacky
LFSR_CAT_CATs in lfsr_file_carve for this reason, but I guess it's a
hack for a hack so...
Code unchanged:
code stack
before: 33728 2776
after: 33728 (+0.0%) 2776 (+0.0%)
This attr-list allocator stuff is becoming over-engineered, these
allocations really aren't that complex...
This may have simplified after removing becksums, but if we need that
complexity again we can cross that bridge when we get to it.
Hand-allocating, dropping buf_size/data_count tracking, and refactoring
lfsr_file_carve to pre-encode the right sibling gives us a nice bit of
code/stack savings:
code stack
before: 33796 2808
after: 33728 (-0.2%) 2776 (-1.1%)
lfsr_data_t datas[d]; => union {
lfs_size_t data_count; lfsr_data_t data;
uint8_t buf[b]; uint8_t buf[b'];
lfs_size_t buf_size; } datas[d+b];
lfs_size_t data_count;
This trades off extra bookeeping (data_count + buf_size vs data_count)
for less-tight stack overhead.
But this also saves a significant amount of RAM in lfsr_file_carve,
where we have exclusive fragments/bptrs for our left and right siblings.
So the end stack cost/savings mostly cancel out.
The end result seems like a net benefit for code cost:
code stack
before: 33872 2816
after: 33796 (-0.2%) 2808 (-0.3%)
Christopher Haster