Still needs testing with LFS3_GC=1, and tests that intentionally test
preerasing, but this should at least fix most fsinfo.flag related issues.
Despite no intentional preerase testing, this already found a number of
issues. Most importantly: our ckpoint-agnostic gbmap zeroing was never
going to work with preerasing!
Main fixes:
- Adopted conservative zeroing of gbmap during rebuilds
This was the biggest change. Our previous lfs3_gbmap_zero impl was
never going to work with preerasing because it unconditionally
cleared BMERASED ranges.
Not entirely wrong, but a big waste of any preerase work.
It also causes the whole system to lock up when LFS3_GC_LOOKAHEAD and
LFS3_GC_PREERASE fight to make progress. With LFS3_GC_LOOKAHEAD
clearing BMERASED ranges, and LFS3_GC_PREERASE clearing the lookahead
flag, nothing gets done!
---
The fix was to rewrite lfs3_gbmap_zero[unknown] to only zero BMERASED
(and BMINUSE, though this isn't strictly necessary) ranges in the
unknown window. This keeps any known-preerased blocks around and
avoids throwing that information away.
This is also slightly different from BMBAD ranges, which we want to
keep around forever, even if in the unknown window.
Whether or not was should limit zeroing BMINUSE ranges is an
interesting question. If we already need this logic, I think extending
it to BMINUSE is a good idea because of how it limits gbmap commits
during rebuilds:
- Unfortunately, gbmap rebuilds require quite a few commits to both
(1) zero gbmap state, and (2) set all the in-use blocks to BMINUSE.
This is especially concerning when relying on aggressive gc, such as
gc_lookgbmap=-1, which may trigger rebuilds when only a couple
blocks are allocated.
Limiting zeroing limits gbmap commits in two ways:
1. We only need to update ranges in the unknown window, which
shrinks with more aggressive gbmap rebuilds.
2. By not clearing BMINUSE ranges in the known window, populating
those blocks during the lookgbmap scan should be a noop.
Together, this hopefully makes aggressive gbmap rebuilds relatively
cheap, at least in terms of progs/erases.
- It's slightly simpler if BMINUSE and BMERASED are handled the same.
- Actually increment the preeraser known window in lfs3_alloc_inc.
Otherwise our estimated preeraser.count only ever increases! There was
some trickiness to make sure preeraser.count is only ever decremented
when allocating erased blocks, but fortunately lfs3->gbmap.ecksum's
existence can tell us that information.
- Reset preeraser state during gbmap rebuilds.
Also necessary to avoid unbounded preeraser.count. The simplest
solution is to zero the preeraser, which forces it to rescan the gbmap
for BMERASED ranges. The preeraser strictly avoids redundant erases.
This does require extra gbmap lookups during LFS3_GC_PREERASE, but
that's not the end of the world.
- Avoid erasing corrupted preerased blocks in case there's other
preerased blocks available in our gbmap.
This happens when the ecksum check fails, implying a prog was
attempted, but power was lost.
Before this change (the continue in lfs3_alloc_:11244), we were
erasing corrupt ecksums, which is not _wrong_, but sort of defeats the
purpose of prerasing. Skipping the block and trying another:
1. Is better in terms of wear-leveling (try not to double erase!)
2. Minimizes latency if we have other preerased blocks we can use
- Made lfs3_fs_gc_ preerasing actually conditional on the
LFS3_GC_PREERASE flag.
Before, lfs3_fs_gc_ was unconditionally preerasing, which is wrong!
---
Currently passing:
LFS3_YES_GBMAP=1 \
LFS3_YES_REVPERTURB=1 \
LFS3_PREERASE=1 \
make test-runner -j \
& ./scripts/test.py -j -b
Other test fixes:
- Mostly just adding the necessary LFS3_I_PREERASE flags for all
lfs3_fs_stat calls.
- LFS3_I_PREERASE and LFS3_I_LOOKAHEAD can interact in funny ways. Just
needed testing.
- lfs3_trv_t doesn't actually do anything with LFS3_T_PREERASE, so we
shouldn't try to test it.
- Adopted lfs3_fs_ck instead of explicit traversals where possible.
- test_badblocks_*_btree_many was still running with LFS3_YES_GBMAP, but
it shouldn't be. The gbmap state is undefined during internal btree
tests.
Code changes:
code stack ctx
before: 35260 2136 660
after: 35260 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38492 2144 776
gbmap after: 38560 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39036 2168 796
preerase after: 39168 (+0.3%) 2168 (+0.0%) 796 (+0.0%)
This started with adding -d/--diff support to dbgflags.py, which is very
useful for comparing flags during test failure.
The flag asserts in our tests generally look like this:
tests/test_mount.toml:171:assert: assert failed with 33570064,
expected eq 33570576
assert(fsinfo.flags == (
Which can now be quickly compared with dbgflags.py:
$ ./scripts/dbgflags.py +i 33570064 -d 33570576
LFS3_I_GBMAP 0x02000000 Global on-disk block-map in use
LFS3_I_REVPERTURB 0x00000010 Mounted with LFS3_M_REVPERTURB
LFS3_I_MKCONSISTENT 0x00000100 Filesystem needs mkconsistent to write
-LFS3_I_LOOKAHEAD 0x00000200 Lookahead buffer is not full
LFS3_I_PREERASE 0x00000400 Blocks can be pre-erased
LFS3_I_COMPACT 0x00000800 Filesystem may have uncompacted metadata
LFS3_I_CKMETA 0x00001000 Metadata checksums not checked recently
LFS3_I_CKDATA 0x00002000 Data checksums not checked recently
The assert print is a bit more annoying than it needs to be, as it only
prints in decimal. But, since our prettyasserts.py only works at the
syntax layer, it's not possible to make it any smarter.
---
To make this diffing work required a couple more features in our
self-parsing Flag class:
- Keep track of lineno, mainly for ordering things
- Moved find logic into a staticmethod on all classes
- Added _sentinel based defaults to find functions
- Allowed self to be non-class in line functions to deduplicate "Unknown
flag" messages
I went ahead and extended these to the other self-parsing classes (Err
and Tag) in case they're useful in the future.
This was broken. The good news is this was easily detected by our
test_mtree tests.
The problem is that mroot split + drop (resulting in an mtree with one
mdir) is indistinguishable from mroot relocation via mdelta. Both cases
have an mdelta of 0.
This also breaks the later mid-mdir update if we wanted to stay on the
current chain mroot.
The tricky part is we have several entangled cases:
- mdir=active mroot, mid>=0 - follow mdir_
- mdir=active mroot, mid<=-1 - follow mroot_, not mdir_!
- mdir=chain mroot, mid<=-1 - follow mdir_, not mroot_!
---
It's tempting to rely on mid<=-2 vs mid==-1 for chain mroots vs active
mroot, but this doesn't always work! During traversals mid is always
<=-2, in part because we don't actually know if the current mroot is the
active mroot until we try to lookup its child.
Fortunately, what _does_ work is just comparing against the mroot's
blocks, which we know.
Though the continued reliance and reliability of mptr comparisons makes
me wonder if it's possible to simplify said function...
Code changes:
code stack ctx
before: 35224 2136 660
after: 35260 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38464 2144 776
gbmap after: 38492 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 39008 2168 796
preerase after: 39036 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
Maybe the subject line should say "Implemented", because these never
worked in the first place. Unfortunately our tests missed this due to a
couple reasons:
- Mroot chains are difficult to create due to the required exponential
growth.
- The only thing that actually commits to chain mroots is mdir
compaction. Though this functionality will be useful for future block
eviction/error correction.
- Previous revision count issues were making relocations in our
compaction tests unlikely.
Fortunately, now that revision count behavior is more correct, our tests
are correctly highlighting that this is broken.
---
Implementing chain mroot commits was a bit intimidating, but fortunately
it just required a bit of teasing to get lfs3_mdir_commit_ to trigger
the tail-recursive mroot chain update when the mdir is a non-active
mroot.
The gcksum is also doing a great job here with identifying bugs. Without
it this bug would have been difficult to notice, since compactions
otherwise have no observable effect on the system.
Code changes:
code stack ctx
before: 35164 2136 660
after: 35224 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38400 2144 776
gbmap after: 38464 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38940 2168 796
preerase after: 39008 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
This is a common pitfall with mdirs that has only ended up in the
codebase ~3(?) times. Naively using a power-of-two recycle counter for
relocations ends up aliasing mdir blocks such that only one block is
actually wear-leveled.
I'm not entirely sure it was intentional, but the previous
double-increment during needsrelocation checks made mdirs relocate one
recycle early, avoiding this aliasing issue.
However, the double-increment had other issues. The most glaring is that
it would always trigger two relocations back-to-back due to the mismatch
between counter cycles and overflow checks. Sort of defeating the
purpose of wear-leveling the two mdir blocks separately...
---
What we really want is a counter that's always coprime with 2. Such as
our old friend mod (2^n)-1.
Unfortunately mod (2^n)-1 counters don't really have any great
optimization trick. They show up all the time when code relies on the
multiplicative cycle of a 2^n finite-field, but despite this, all of
the implementations I've seen rely on a simple branch to handle the
one extra state.
I'm not sure this is the best implementation, but adding 2 and
subtracting 1 on non-overflow seems to minimize resulting code cost.
Presumably because the compiler is able to deduplicate this with the
needsrelocation check. Though we're only talking about a handful of
bytes.
Code changes:
code stack ctx
before: 35152 2136 660
after: 35164 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38388 2144 776
gbmap after: 38400 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38940 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
---
We don't have great tests for this, as it's difficult to create
rigorous checks for our best-effort dynamic wear-leveling. But,
surprisingly enough, this _was_ caught by
test_exhaustion_spam_uzd_fuzz's doubling-disk-doubles-lifetime check!
Though only with LFS3_YES_GBMAP=1:
LFS3_YES_GBMAP=1 \
TESTS=tests/test_exhaustion.toml \
make test-runner -j \
&& ./scripts/test.py test_exhaustion_spam_uzd_fuzz -O- -j \
| grep lifetime
rbyd.weight == btree.weight does not imply rbyd is a bshrub root!
This was introduced during a btrv rework, and, unfortunately, works
_most_ of the time. It's extra deceptive because we eagerly collapse
these degenerate roots in lfs3_btree_commit_, but we can't collapse
bshrub roots!
Well, not easily anyways (I guess we could convert to a btree...), but
what's important is that single-entry btree nodes are possible, and
relying on the weight for shrubbed roots is a weak condition.
Instead, we now just check the shrub bit for shrubbed roots. We have a
whole bit for this, so might as well actually use it.
This isn't even the first reliance of the shrub bit in this function!
---
Code changes minimal:
code stack ctx
before: 35148 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38388 2144 776
gbmap after: 38388 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
A simple but nasty typo! Quite confusing to figure out.
Valgrind was quick to highlight that mtortoise was uninitialized, but
without any sort of debugger support, it took many _many_ rereadings of
the code to figure out what was actually going wrong. I even started to
wonder if C's union aliasing rules were the culprit.
To make matters worse, I only noticed Valgrind's warning because I was
trying to find a heisenbug that turned out to be unrelated.
Code changes minimal:
code stack ctx
before: 35144 2136 660
after: 35148 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38384 2144 776
gbmap after: 38388 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38924 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
This was resulting in memory leak warnings from Valgrind, which were
getting in the way of debugging an unrelated uninitialized memory issue.
We normally wouldn't care about this sort of bounded memory leaks, but
in this case Valgrind can't tell if the memory leak is from the runner
or filesystem, errors, and prevents other tests from running. Just to be
more annoying, this only triggered when overriding defines, which is
something you do exactly when you are trying to debug something.
Fortunately, with a bit of typecasting we still have access to the
allocated value arrays (type-stripped due to opaque test_define_t), and
can clean up the relevant memory.
Whoops, looks like the lfs3_alloc refactoring resulted in us calling
lfs3_alloc_inc multiple times. In effect allocating multiple blocks when
triggered, wasting erased-state and lookahead scans.
The good(?) news is this was only triggered when we failed to erase a
block, which made it difficult for our tests to catch.
---
Saves a bit of code when we don't do unnecessary work:
code stack ctx
before: 35152 2136 660
after: 35144 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38392 2144 776
gbmap after: 38384 (-0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38928 2168 796
preerase after: 38924 (-0.0%) 2168 (+0.0%) 796 (+0.0%)
This was introduced with the simplified traversal clobbering logic.
Previously, traversal clobbering was a bit more aggressive, relying on
the explicit tstate state machine. This was replaced by implicit
mid-related state, which looks like it may have introduced some holes.
In this case, lfs3_mdir_commit was failing to clobber non-active mroot
chain mdirs. Non-active mroots are particularly tricky because we
(1) don't track these in-RAM, (2) only reach them during traversals,
and (3) require heavy wear-leveling writes for them to even appear in
in system.
---
The solution here is an extra check in lfs3_mdir_commit_'s post-commit
state updates to update any mid<=-1 mroots to the new active mroot.
This clobbers mroot chain traversals by skipping non-active mroots, but
this is unavoidable since lfs3_mdir_commit_ could always introduce
new/relocate mroot chain mroots. Note this should match the previous
state-machine dependent behavior.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38380 2144 776
gbmap after: 38392 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38920 2168 796
preerase after: 38928 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
Previously these only implied M/F flags, which risks quite a bit of
confusion. It's entirely reasonable to expect these to affect lfs3_fs_gc
(arguably the more correct behavior?) but they did not.
Maybe these should imply the GC behavior, or maybe we should rename them
to LFS3_YES_GC_*/LFS3_YES_M_*/etc, but at the very least the current
behavior of implying M/F is probably not a good idea.
So, removing for now. This is the safest option, and better thought-out
behavior can be added in the future.
The original motivation for making LFS3_PREERASE opt-out, is that it
makes sense for LFS3_GBMAP to bring in all gbmap-related features
(PREERASE, BADBLOCKS (future)). However, after a bit of use, I think
this just complicates our ifdef logic too much.
So instead, LFS3_PREERASE is now opt-in, with the intention of making
all ifdefs relative only to the default build. I think this will make it
easier to reason about ifdefs, at least internally.
Eventually, I want to look into alternative default builds (LFS3_BIGGER,
LFS3_BIGGERR, ..., LFS3_BIGGEST), which would provide an alternative way
to enable all gbmap-related features. Though these builds have a
high-risk of bikeshedding (LFS3_GC?), so we'll see.
---
That being said, the main ergonomic improvement was probably adding
a #error, so we don't have to check ifdef GBMAP everywhere.
Maybe this should be extended to LFS3_RDONLY? Or maybe not, LFS3_RDONLY
is a bit of a special case.
No code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38380 2144 776
gbmap after: 38380 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
preerase before: 38920 2168 796
preerase after: 38920 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
It's two words, and I've probably already spent too long fiddling around
with this.
This drops the messy NULL checks in lfs3_gbmap_set_, for a wrapper that
defaults to a global {.cksize=-1} ecksum when NULL.
---
Code changes, with both a compound literal (cl), and global
constant (gc). The current code uses a global constant:
code stack ctx
before: 35144 2136 660
after+cl: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
after+gc: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38272 2144 776
gbmap+np after+cl: 38412 (+0.4%) 2144 (+0.0%) 776 (+0.0%)
gbmap+np after+gc: 38380 (+0.3%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38940 2168 796
gbmap+yp after+cl: 38952 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
gbmap+yp after+gc: 38920 (-0.1%) 2168 (+0.0%) 796 (+0.0%)
It's interesting to note that while the global constant generally
reduces code cost, it prevents constant-expr optimizations from
eliminating the NULL checks when compiling without preerases (np).
Is that enough reason to revert this? Probably not. (1) The simpler
codebase, and reduced chance of forgetting a NULL check, is preferable,
and (2) we don't care about the code cost of niche gbmap configurations
as much as the non-gbmap modes.
This mostly reverts the previous commit, and makes non-NULL ecksums the
consistent API.
Non-NULL ecksums are what the original rbyd-level ecksum API expects,
and enforcing this avoids the ifdef mess required to minimize unused
code impact.
This unfortunately clutters up lfs3_gbmap_set_'s logic with NULL checks,
but at least keeps the mess constrained to lfs3_gbmap_set_.
lfs3_gbmap_set_ is really the only function that uses NULL ecksums, so
they should probably be lfs3_gbmap_set_'s problem to deal with.
Code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38296 2144 776
gbmap+np after: 38272 (-0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38908 2168 796
gbmap+yp after: 38940 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
This extends support of NULL => ecksum.cksize=-1 (i.e. no ecksum) down
into all ecksum APIs, but only when compiling with LFS3_GBMAP=1.
Previously this was limited to just lfs3_ecksum_cmp, where it is useful,
if inconsistent, for implementing lfs3_gbmap_set_.
The reason for the ifdef is to try to limit the code impact on non-gbmap
builds. We really only use NULL ecksums in the gbmap layer, and in
theory the gbmap should have no effect on code size when disabled.
Unfortunately the cross-layer usefulness of the ecksum API makes this
tricky.
Thanks to the ifdef, code changes limited to gbmap+nopreerase:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38272 2144 776
gbmap+np after: 38296 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38908 2168 796
gbmap+yp after: 38908 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
So:
- lfs3_alloc_markinuse -> lfs3_alloc_setinuse
- lfs3_alloc_markinusebptr -> lfs3_alloc_setinusebptr
- lfs3_gbmap_mark_ -> lfs3_gbmap_set_
- lfs3_gbmap_mark -> lfs3_gbmap_set
- lfs3_gbmap_markbptr -> lfs3_gbmap_setbptr
IMO a bit of a better name now that lfs3_gbmap_set* also stores optional
ecksums.
With REVPERTURB implemented, I think this is all we need to finish up
preerase allocations. Though not quite tested yet.
As noted when implementing lfs3_allocclaim, lfs3_allocclaim is only
half the solution for preerase allocations. If we tried to use
lfs3_allocclaim everywhere, our mdir commit path would quickly end up a
recursive mess.
This is where our ecksums kick in.
In theory, ecksums (erased-state checksums), let us detect attempted
progs. Unfortunately, in practice it's not so simple. If we tried to
detect a failed data block write, for example, it's entirely possible
the attempted write matches the erased-state exactly, making attempted
prog detection impossible. Imagine if users couldn't write all 0xffs to
a file, that'd be a weird constraint.
To work around this, we also require at least one bit flip during progs.
This ensures an ecksum failure requires a non-trivial checksum
collision.
This is where REVPERTURB comes in (and in rbyd logs, the valid bits).
---
Humorously, now that REVPERTURBs are implemented, I think the only
change required for preerased allocations is to error if REVPERTURB is
disabled.
Extra humorously, this is surprisingly tricky because REVPERTURB is a
mount flag and PREERASE a gc flag.
The solution is sort of best-effort. We error if trying to preerase
without REVPERTURB, but _don't_ error if trying to allocate when the
gbmap contains preerased blocks. This wastes the preerase cycles, but
allows disk compatibility between filesystems in different modes.
No code changes:
code stack ctx
before: 35144 2136 660
after: 35144 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38272 2144 776
gbmap+np after: 38272 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38908 2168 796
gbmap+yp after: 38908 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
The main change is adding LFS3_M_REVPERTURB, which will be necessary for
preerase allocations, but I got distracted and ended up giving the
revision count subsystem a bit of a refactor.
Main changes:
- Added LFS3_M_REVPERTURB, which ensures the leading bit in the
revision count changes after each allocation/relocation/compaction.
This is generally optional, but will be required for preerase
allocations. Our ecksum system is only reliable if we ensure at least
one bit changes, otherwise the chance of ecksum collision is very
high.
The downside of LFS3_M_REVPERTURB is that we need to read the contents
of the new block to figure out what the bit should change to. Probably
a minimal cost in the system, but still a good reason to make the
behavior optional.
Does LFS3_M_REVPERTURB have any use outside of preerased allocation?
I'm not sure. Maybe it has some niche use reducing the chance of bd
ECC collisions?
- Dropped LFS3_M_REVDBG, but adding low-effort debug bits that are
always enabled.
Making LFS3_M_REVDBG conditional was probably overkill. The flag
checks probably cost more than the actual debug bits when enabled.
Instead, replaced with a simpler, low-effort debug bit system, where
we only set the debug bits during mdir allocation/relocation. These
bits shouldn't change during normal compaction, but we _don't_
introduce debug bits if mounting a filesystem from a driver without
these debug bits.
- Restricted recycle counter to at most 20-bits to make space for
things. This ensures perturb/debug bits don't get overwritten (though
we really only care about perturb bits).
2^20 (~1M) recycles is probably enough for any device littlefs will
run on, especially considering the recycle_count should probably be
several orders of magnitude smaller than the device's expected erase
cycles.
Worst case this can always be increased in the future without
backwards incompatible changes. The only hard requirement for revision
counts is that the full 32-bits are comparable.
- Simplified lfs3_rev_inc and friends, and moved most of the
disk-dependent revision count stuff down into lfs3_rbyd appendrev.
This deduplicates the messy revision count handling in
lfs3_btree_commit_.
Though note the implicit lfs3_rbyd_appendrev now defaults to writing
the btree debug bits ('b'). A bit of a hack, but works for littlefs.
Here's the resulting encoding:
vvvv---- -------- -------- -ddddddd
vvvvrrrr rrrrrr-- -------- -ddddddd
vvvvrrrr rrrrrrnn nnnnnnnn pddddddd
'-.''----.----''----.----' ^'--.--'
'------|----------|------|---|---- 4-bit relocation revision
'----------|------|---|---- recycle-bits recycle counter
'------|---|---- pseudorandom noise (if revnoise)
'---|---- perturb bit (if revperturb)
'---- low-effort debug bits
11-1--- - h = mroot anchor
11-11-1 - m = mdir
11---1- - b = btree node
Note we store revision counts as le32s, so the perturb bit should end up
as the leading bit in the first byte.
Costs a bit more code (mostly because the debug bits are now
unconditional, even if low-effort), but simplifies the codebase:
code stack ctx
before: 35124 2136 660
after: 35144 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
after+yesrevperturb: 35192 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38252 2144 776
gbmap+np after: 38272 (+0.1%) 2144 (+0.0%) 776 (+0.0%)
gbmap+np after+yrp: 38328 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38832 2168 796
gbmap+yp after: 38852 (+0.1%) 2168 (+0.0%) 796 (+0.0%)
gbmap+yp after+yrp: 38908 (+0.2%) 2168 (+0.0%) 796 (+0.0%)
This just organizes the compat flags/masks a bit better, and avoids
needing to mess with the internals of lfs3_mountmroot anytime the wmask
flags change.
In case it isn't clear, the wmask/rmask/omask indicate which flags are
optional to mount the filesystem for the relevant mode. Currently the
only optional flag is LFS3_WCOMPAT_GBMAP.
Though, humorously, should lfs3_omask actually be all zeros?
---
Code changes minimal:
code stack ctx
before: 35124 2136 660
after: 35124 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38264 2144 776
gbmap+np after: 38252 (-0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38844 2168 796
gbmap+yp after: 38832 (-0.0%) 2168 (+0.0%) 796 (+0.0%)
This is hopefully a better alternative to LFS3_IFDEF_YES_* macros.
If we need special behavior for LFS3_IFDEF_YES_*, we almost always need
special behavior for LFS3_IFDEF_NO_* and LFS3_IFDEF_MAYBE_* as well.
So merging all three states into a single macro saves typing and
hopefully encourages correct handling of all cases.
No code changes.
The extends the removal of implicit ifdefs to the flag functions, where
previously implicit ifdefs were the norm. (Well, not really, implicit vs
explicit ifdef use was actually very inconsistent!)
The motivation for this is explicit ifdefs make it easier to see what
code is compiled in to what build. This in theory makes refactoring/
review easier. If you're doing something weird like calling
lfs3_o_isexcl in a rdonly context, the code should probably raise
eyebrows.
---
The only exception right now is the isrdonly/iswronly functions. These
are a bit more nuanced, and probably what started the implicit ifdef
pattern.
Some compiler noise due to lfs3_file_opencfg tweaks:
code stack ctx
before: 35112 2136 660
after: 35124 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38252 2144 776
gbmap+np after: 38264 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38832 2168 796
gbmap+yp after: 38844 (+0.0%) 2168 (+0.0%) 796 (+0.0%)
I'm not sure implicit ifdefs really help with readability.
They do redunce the number of lines, but the implicit ifdefs make it
harder to figure out what is actually compiled in.
The nice thing about explicit ifdefs is they're, well, explicit. This
makes it easier to rule out code paths early, and the earlier you can
rule things out, the easier it is to focus on what matters for the build
you care about.
---
TLDR IMO explicit ifdefs are preferable because that make it easier to
see what code is being compiled in to what build.
No code changes.
Note this is only half of preerased allocation.
And the easy half too.
The problem is littlefs's "restricted flash model" (as I'm now calling
it), which makes minimal assumptions about the behavior of the bd's
erase operation to support a wider range of devices. In particular,
littlefs doesn't assume the value of storage after an erase, which makes
detected failed progs (due to powerloss, etc) uniquely difficult.
For data blocks, we at least have the option of simply making sure the
relevant BMERASED range is deleted from the gbmap before use. This does
mean more progs during file writes, but in theory that is much cheaper
than erasing on-demand. And for storage where it's not, you should
probably consider not pre-erasing.
Fortunately, we don't actually need to commit to the gbmap to delete a
BMERASED range. If we consider BMERASED ranges outside the gbmap's known
window as invalid, we just need to decrement the known window to make
progress.
This is now implemented by lfs3_allocclaim, which forces an
lfs3_alloc_sync to ensure BMERASED blocks won't be reused even if power
is lost.
---
You may note this doesn't use the ecksums at all, except the check that
they're still valid. Unfortunately, we can't rely on ecksums for data
blocks because we don't control what gets progged. Worst-case, the data
being written matches the erased-state exactly, which is impossible for
littlefs to detect.
An alternative solution would be to add a header to every data block,
but this would come with several negatives:
- Headers would introduce some (albeit small) complexity into the write
path, have limited value outside of preerases (we would still need to
scan to allocate blocks), and raise questions around what headers
should contain.
- Files would no longer perform optimally around powers-of-two, which
may surprise users and risk unnecessarily poor performance.
- littlefs would lose its "universal migrator" status, as we would need
to inject headers into any existing data blocks.
The in-gbmap ecksums solve the different problem of allocating metadata
blocks, which we can't use lfs3_allocclaim for as it would introduce
recursion.
---
Adds a large (but necessary) chunk of code/stack to the gbmap mode, but
only when preerasing (and some non-gbmap noise?):
code stack ctx
before: 35116 2136 660
after: 35112 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38252 2144 776
gbmap+np after: 38252 (+0.0%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38664 2144 796
gbmap+yp after: 38832 (+0.4%) 2168 (+1.1%) 796 (+0.0%)
This kinda fell out of the preerase gc work.
Normally, we're lazy about committing the gbmap into the mtree. Most
on-demand gbmap rebuilds are followed by an mdir commit anyways, so
normally it would just add redundant work and muddy up the
lfs3_alloc_ckpoint path.
But this isn't the case for gc work, which will probably be followed by
long periods of idling. If we lose power while idling (which, let's be
honest, is the most likely time to lose power), we'll lose any
gbmap-related gc progres. Not ideal.
Fortunately, avoiding this is easy. We just need an additional step
after any traversal/preerasing gc work that eagerly commits the gbmap
into the mtree.
The only downside is a bit more code:
code stack ctx
before: 35116 2136 660
after: 35116 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38188 2144 776
gbmap+np after: 38252 (+0.2%) 2144 (+0.0%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38608 2144 796
gbmap+yp after: 38664 (+0.1%) 2144 (+0.0%) 796 (+0.0%)
Allocating pre-erased blocks gets quite complicated due to our
restricted flash model, but at least the actual pre-erasing is
relatively straightforward:
- We keep track of known preerased state in lfs3->gbmap.preeraser.
- If LFS3_GC_PREERASE is provided during gc work, we increment the
preeraser's known window by scanning the gbmap.
- Any BMFREE ranges we find, we erase a block at a time, and store the
resulting ecksum in a BMERASED range in the gbmap.
- We keep track of how many blocks we erased, and stop early if this
exceeds cfg.gc_preerase_count. This just lets users tune how many
blocks to preerase in case something (?) prevents preerased blocks
from being used.
Some notes:
- We don't really do anything with ranges in lfs3_alloc_preerase. In
theory we could bulk in erase to minimize the number of commits to the
gbmap, but we expect erase to dominate, so this probably isn't worth
it.
And if erase doesn't dominate, why would you bother pre-erasing
blocks?
- Preerasing isn't really a traversal operation, and is managed by a
sort of secondary state machine in lfs3_fs_gc_.
This also means lfs3_trv_read with LFS3_T_PREERASE does nothing, but I
guess that is ok? It's tempting to try to make lfs3_trv_read also
preerase, but it's unclear what block it should return -- it's
probably the wrong API.
- Introducing ecksums actually went quite a bit smoother than I
expected. Though it helps ecksums are the only optional payload, no
type punning or anything.
Ecksums do muddy the gbmap's design a bit, unfortunately. The main
issue being that we can only merge BMERASED ranges with equal ecksums.
This makes BMERASED ranges less compressable than the others, and may
be one reason to limit cfg.gc_preerase_count.
However:
1. This is where I think it's useful to emphasize that the gbmap's
responsibility is to track _free_ blocks, in-use blocks are
secondary.
When allocating, we're going to stop at the first BMFREE/BMERASED,
but may need to skip over an unbounded number of BMINUSE/BMBAD
blocks. So the compressability of BMFREE/BMERASED ranges should
have less of an impact on block allocation.
2. In practice, most flash uses consistent erase values, so the
resulting ecksums will probably be compressable. The exceptions are
noop-erases (SD/eMMC, RAM, NVRAM, etc), and encryption with block
address permutation?
Though noop-erases are a pretty big exception.
Code changes:
code stack ctx
before: 35116 2136 660
after: 35116 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap+np before: 38040 2136 776
gbmap+np after: 38188 (+0.4%) 2144 (+0.4%) 776 (+0.0%)
code stack ctx
gbmap+yp before: 38040 2136 776
gbmap+yp after: 38608 (+1.5%) 2144 (+0.4%) 796 (+2.6%)
This does two things:
- Deduplicates another fixgrm call, now all fixgrm cleanup (outside of
mkdir/remove) goes through lfs3_mtree_gc.
- Predicates fixgrm on if lookahead work is complete.
Code changes:
code stack ctx
before: 35128 2136 660
after: 35116 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38052 2136 776
gbmap after: 38040 (-0.0%) 2136 (+0.0%) 776 (+0.0%)
Despite programming in C for quite a while, I didn't expect this to
work. But it turns out you _can_ typedef nested structs, you just need
to put the typedef outside the struct.
IMO this makes it a bit easier to understand what state is exclusive to
the lfs3_t struct. We do use these typedefs internally, but only for
pointers into the core lfs3_t.
No code changes.
Also tweaked related flag comments a bit.
---
This adds another internal flag modifier ('a') to dbgflags.py to
indicate a flag is an alias for multiple other flags.
These still show up in -l/--list and name searches:
$ ./scripts/dbgflags.py -l +ck
LFS3_CK_MKCONSISTENT 0x00000100 Make the filesystem consistent
LFS3_CK_LOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_CK_COMPACT 0x00000800 Compact metadata logs
LFS3_CK_CKMETA 0x00001000 Check metadata checksums
LFS3_CK_CKDATA 0x00002000 Check metadata + data checksums
LFS3_CK_CK 0x00003000 Alias for all check work
LFS3_CK_GC 0x00003b00 Alias for all gc work
But are hidden from value searches, as they would be redundant and the
specific low-level flags are probably more useful:
$ ./scripts/dbgflags.py +ck 0x00003000
LFS3_CK_CKMETA 0x00001000 Check metadata checksums
LFS3_CK_CKDATA 0x00002000 Check metadata + data checksums
If lfs3_fs_fixgrm is an implicit requirement for LFS3_T_MKCONSISTENT, we
might as well move it into the core lfs3_mtree_gc logic and save on the
redundant lfs3_fs_fixgrm calls.
Saves a bit of code:
code stack ctx
before: 35164 2136 660
after: 35128 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38088 2136 776
gbmap after: 38052 (-0.1%) 2136 (+0.0%) 776 (+0.0%)
Will revert.
The idea here is that fixgrm isn't really a traversal operation. It's
convenient, but in an effort to simplify things, dropping fixgrm from
lfs3_trv_read makes sense.
But dropping fixgrm seems to cause more problems than it's worth.
---
Note test_trvs is currently failing because attempting to remove an
orphaned stickynote in the grm queue without calling fixgrm breaks
things.
It's probably fixable, but why? If we keep the implied fixgrm it's not
possible to trigger a remove without a clean grm queue. And we want to
keep our grm queue clean anyways to prevent a full fixorphan scan.
Code changes:
code stack ctx
before: 35164 2136 660
after: 35112 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38088 2136 776
gbmap after: 38040 (-0.1%) 2136 (+0.0%) 776 (+0.0%)
This extends the hopefully useful LFS3_GC_ALL flag alias to the other
functions, without trying to figure out what the heck LFS3_M_ALL should
mean semantically.
Current definitions:
// an alias for ck work
*_CK 0x00003000 ---- ---- ---- ---- --11 ---- ---- ----
// an alias for all possible gc work
*_GC 0x00003b00 ---- ---- ---- ---- ++11 11+1 ---- ----
*_MKCONSISTENT 0x00000100 ---- ---- ---- ---- ---- ---1 ---- ----
*_LOOKAHEAD 0x00000200 ---- ---- ---- ---- ---- --1- ---- ----
*_PREERASE+ 0x00000400 ---- ---- ---- ---- ---- -+-- ---- ----
*_COMPACT 0x00000800 ---- ---- ---- ---- ---- 1--- ---- ----
*_CKMETA 0x00001000 ---- ---- ---- ---- ---1 ---- ---- ----
*_CKDATA 0x00002000 ---- ---- ---- ---- --1- ---- ---- ----
*_REPAIRMETA+ 0x00004000 ---- ---- ---- ---- -+-- ---- ---- ----
*_REPAIRDATA+ 0x00008000 ---- ---- ---- ---- +--- ---- ---- ----
+ Planned
One weird artifact of this is that LFS3_GC_CK ~= LFS3_GC_CKDATA, but I'm
not sure that's a bad thing? Note this is very much not true for info
flags, LFS3_I_CK != LFS3_I_CKDATA.
No code changes.
LFS3_GC_ALL will hopefully be useful for users for convenience, but
internally we should probably preter explicit flag sets to make flag
changes explicit and easy to tweak.
---
One intention was to make the progress check less messy, but that didn't
really work. Attempting to deduplicate the LFS3_GC_LOOKAHEAD flag just
hurts literal sharing in the function.
Which is honestly a strong argument against this change...
No code changes.
Unsure if this falls into over-engineering. But note lfs3->flags can
change between gc steps, so the aborting useless traversals is probably
worth keeping if only for that reason.
In the future this is a good function to tweak without worrying about
compatibility issues.
Code changes:
code stack ctx
before: 35124 2136 660
after: 35164 (+0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38048 2136 776
gbmap after: 38088 (+0.1%) 2136 (+0.0%) 776 (+0.0%)
This is a number of tweaks intended to (1) minimize unexpected gc
latency due to last-minute lookahead scans, while (2) keeping the gc
logic simple and easy to reason about:
- Switched to using lfs3->flags directly for the is-work-done predicate.
This ensures lfs3_fs_gc_ never terminates until the requested work is
done, at the risk of, well, never terminating.
But the previous "pending" variables had the same risk (set
gc_compact_thresh=0 for example), it just removed the risk of
non-termination due to conflicting gc requests. In both cases
gc_compact_thresh has the biggest risk of non-termination.
- Prioritize lookahead scans before anything that can allocate
(MKCONSISTENT, COMPACT, etc).
- Dropped aborting useless traversals (ckpointed lookaheads mainly).
It's a good idea, but surprisingly complicated to decide when we
should abort for all traversals. COMPACT, CKMETA, for example, are
still useful to continue even if they can't prove anything about the
system.
Though now that I'm writing this, I'm wondering what the argument
against LOOKAHEAD aborting is. Maybe this should be reverted for
LOOKAHEAD as a special case...
Code changes minimal:
code stack ctx
before: 35152 2136 660
after: 35124 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38076 2136 776
gbmap after: 38048 (-0.1%) 2136 (+0.0%) 776 (+0.0%)
This effectively reverts 1f824a0:
- LFS3_T_COMPACTMETA -> LFS3_T_COMPACT
- gc_compactmeta_thresh -> gc_compact_thresh
And friends.
After using LFS3_T_COMPACTMETA for a bit, I think it just adds noise
without much value. Especially when next to LFS3_T_LOOKAHEAD,
LFS3_GC_PREERASE, LFS3_M_SYNC, etc.
It's interesting that we already have some very distinct verbs for this
sort of thing based on data type (compact => metadata, garbage-collect
=> disk, compress => data).
dbgflags.py now uses +flags to indicate flag namespaces, but our gdb
script only forwarded -f/--flags, which made dbgflags a bit of a pain
to use in the debugger!
Fortunately an easy fix.
Now this works:
(gdb) dbgflags +t trv.gc.t.h.flags
LFS3_T_RDWR 0x00000000 Open traversal as read and write
LFS3_T_MKCONSISTENT 0x00000100 Make the filesystem consistent
LFS3_T_LOOKAHEAD 0x00000200 Repopulate lookahead buffer
LFS3_t_TRAVERSAL 0x60000000 Type = traversal
LFS3_t_MDIR 0x00010000 Btype = mdir
Our flag space is already really packed, and I'm not sure having these
as separate flags is meaningful or useful for users. They both indicate
to repopulate allocators, and most users probably won't care that there
are two subtly different allocators operating under the hood.
There's an argument that LOOKAHEAD not touching disk is a useful
distinction, but in practice you really only need LOOKAHEAD work when
mounted RDWR.
So, merged the behaviors of LOOKAHEAD + LOOKGBMAP such that
LFS3_*_LOOKAHEAD requests repopulation of all allocators based on
gc_lookahead_thresh and gc_lookgbmap_thresh.
In priority order (some notes below):
1. If max(lookahead, gbmap) < gc_lookahead_thresh => repop lookahead
2. If gbmap < gc_lookgbmap_thresh => repop gbmap
As a plus, this makes it easier to avoid LFS3_IFDEF_GBMAP mess.
---
It's interesting to note LFS3_*_LOOKAHEAD will still repopulate the
lookahead buffer when the gbmap is present, but only if this would gain
more knowledge than was is currently in the gbmap.
I considered disabling lookahead scans completely when we have a gbmap,
but repopulating the lookahead buffer is still useful if the gbmap is at
risk of exhaustion. This is what gc_lookahead_thresh is for anyways, and
users can set gc_lookahead_thresh=0 if they want to disable this
behavior.
Relatedly, lookahead scans are actually prioritized over gbmap scans
(when they would gain knowledge). In theory this minimizes gc latency,
as gbmap scans risk triggering a full lookahead scan when building the
new gbmap.
---
Code changes minimal:
code stack ctx
before: 35152 2136 660
after: 35152 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38076 2136 776
gbmap after: 38080 (+0.0%) 2136 (+0.0%) 776 (+0.0%)
Before, the gbmap allocator worked by feeding the lookahead allocator,
so all allocation requests went through the lookahead buffer:
alloc ---> lookahead ---> gbmap
This worked, and was easy to strap on to the existing system, but
limited what we could cache to what fits in our lookahead buffer. This
doesn't have a big effect on runtime analysis, since fragmentation is
always a concern, but it does mean we're not taking advantage of the
gbmap range representation and accessing disk more than we need to.
It also limits in-use range skipping to a lookahead buffer at a time,
which is problematic as the whole reason for the gbmap is to make the
lookahead buffer mostly irrelevant.
On top of the range issues, this design makes it difficult to add
preerase info, which is coming up on the TODO list.
---
To fix this, I reorganized the two allocators to run in parallel, with
the gbmap being queried first before falling back to the lookahead
buffer:
alloc -+-> gbmap
'-> lookahead
Instead of relying on the lookahead buffer, the gbmap now stores one
range in the gbmap.free field, using the sign to indicate if it's free
vs in-use (not the best name, but oh well).
This adds a word of storage to the gbmap, but as a tradeoff we can track
a full region in RAM and avoid repeated gbmap lookups.
The lookahead buffer can still be populated by gc work, which may be
useful if the gbmap is exhausted, but will also be cleared during gbmap
allocations to avoid out-of-date state.
---
While reworking this, I also tweaked a number of other allocator things:
- Adjusted lookahead.window to point to next block candidate
(lookahead.window and gbmap.window should now always match)
- Renamed lfs3_alloc_zerogbmap -> lfs3_gbmap_zero
- Moved some alloc functions around
Code cost was mostly unaffected. Added an extra word to gbmap's ctx, but
as a tradeoff saved a chunk of stack by avoiding nested allocators:
code stack ctx
before: 35160 2136 660
after: 35152 (-0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38020 2152 772
gbmap after: 38076 (+0.1%) 2136 (-0.7%) 776 (+0.5%)
See previous commit for motivation.
I can't think of how you could easily find this information from the
gbmap during/after mount, short of a O(d log_b d) scan through the
gbmap. Maybe useful, but probably not a great tradeoff for what is only
debug/diagnostic information.
So reverting, but maybe interesting to explore in the future with other
debug APIs.
Code changes:
code stack ctx
before: 35220 2136 660
after: 35160 (-0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38116 2152 776
gbmap after: 38020 (-0.3%) 2152 (+0.0%) 772 (-0.5%)
The idea here was that we could provide best-effort known in-use/free
block info (and eventually pre-erased and bad block info) as a cheaper
alternative to lfs3_fs_usage. It's probably still not what users expect
from lfs3_fs_stat, but may be useful as debug/diagnostic info:
- fsinfo.known_free - Number of known free blocks
- fsinfo.known_inuse - Number of known in-use blocks
- fsinfo.known_preerased* - Number of pre-erased blocks
- fsinfo.known_bad* - Number of bad blocks
- fsinfo.block_count-(all of the above) - Number of unknown blocks
But while known_free/known_inuse is easy enough to find from the
lookahead buffer, it's surprisingly tricky from the gbmap. The best
option I can think of requires scanning the gbmap in O(d log_b d) either
(1) during mount, (2) during mkconsistent, or (3) during lookahead
scans. And that much extra work for debug/diagnostic info seems like a
poor tradeoff.
Note, though, that after scanning once, in theory the info would be
~free to maintain during gbmap rebuilds.
Will revert.
Code changes:
code stack ctx
before: 35160 2136 660
after: 35220 (+0.2%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38020 2152 772
gbmap after: 38116 (+0.3%) 2152 (+0.0%) 776 (+0.5%)
In theory, lfs3_size_t should be used for in-block sizes (though this is
also mixed up with in-device sizes?), lfs3_off_t for file sizes, and
lfs3_block_t for block counts (I don't think lfs3_off_t/lfs3_block_t
will ever differ, but the notation is helpful).
Though I've not done a great job at keeping these types organized...
Changed:
- cfg.block_count: lfs3_size_t -> lfs3_block_t
- cfg.file_limit: lfs3_size_t -> lfs3_off_t
- fsinfo.block_count: lfs3_size_t -> lfs3_block_t
- fsinfo.file_limit: lfs3_size_t -> lfs3_off_t
- lfs3_fs_usage: lfs3_ssize_t -> lfs3_sblock_t
- geometry.block_size: lfs3_size_t -> lfs3_off_t
- geometry.block_count: lfs3_size_t -> lfs3_block_t
- and some internals
No code changes.
- Adopted -2,-2 for unbounded lfs3_mdir_commit___ ranges that include
gstate.
Why not? We treat any negative upper bound as unbounded and it's a bit
easier to read.
- Prefer <=-2 when checking for rid bounds that include gstate.
- Prefer <=-2 when checking for attached rattr weights.
- Cleaned up a couple outdated comments.
No code changes:
code stack ctx
before: 35160 2136 660
after: 35160 (+0.0%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38020 2152 772
gbmap after: 38020 (+0.0%) 2152 (+0.0%) 772 (+0.0%)
May rerevert this in the future, but I'm on the fence.
It's true this only saves a small amount of code, but in theory it also
reduces stack consumption in name-related functions. Currently this
doesn't affect the stack hot-path, which is a bit surprising as this
includes lfs3_set, but it may in the future.
The arguments against this optimization are also a bit weak:
- Non-null-terminated strings - We probably shouldn't optimize for a
theoretical future feature. If anything, we want to optimize in the
opposite direction to best measure the theoretical code cost.
- Precomputing strlen early - While this is generally a good idea, our
rattrs benefit greatly from compact encodings, as rattrs sitting on
the stack are one of the bigger contributors to our stack hot-path.
So for now I'm unreverting to see how long this optimization makes
sense, but could see this being rereverted in the future.
At the very least we probably want to keep the test changes to make
future testing easier.
---
Saves a bit of code:
code stack ctx
before: 35188 2136 660
after: 35160 (-0.1%) 2136 (+0.0%) 660 (+0.0%)
code stack ctx
gbmap before: 38048 2152 772
gbmap after: 38020 (-0.1%) 2152 (+0.0%) 772 (+0.0%)
Christopher Haster