010c82475f48ab6ee243a3a12ab05d1acda5dfc7
1050 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Christopher Haster
|
010c82475f |
Crammed LFSR_CAT_NAME to reuse the wasted word in the name lfsr_data_t
One annoying thing about lfsr_data_t is when representing in-RAM
buffers, the last word of the struct goes completely unused. This
commit attempts to save this word of RAM in file names by forcibly
(hackily?) truncating the name's lfsr_data_t:
.---+---+---+---. . . . . .---+---+---+---. . . . . .---+---+---+---.
|0| did_size | |0| did_size | | did_data |
+---+---+---+---+ +---+---+---+---+ | |
| did_ptr ------. | did_ptr ------. | |
+---+---+---+---+ | +---+---+---+---+ | | |
| (unused) | | | (unused) | | | |
+---+---+---+---+ . | . . +---+---+---+---+ . | . . +---+---+---+---+
|0| name_size | | |0| name_size | | | name_size |
+---+---+---+---+ | +---+---+---+---+ | | |
| name_ptr | | | name_ptr | | | |
+---+---+---+---+ | +---+---+---+---+ | | |
| (unused) | | | did | <-' | |
+---+---+---+---+ . | . . | | . . . . '---+---+---+---'
| did | <-' '---+---+---+---'
| |
'---+---+---+---'
Curiously, this didn't seem to save RAM but saved a bit of code cost?
I guess this is because 1. file names, despite being very common, don't
occur on the stack hot-path that starts at lfsr_file_sync, and 2. while
we don't save stack cost, sometimes the reduced stack pressure can
reduce stack manipulation instructions:
code stack
before: 33728 2776
after: 33672 (-0.2%) 2776 (+0.0%)
If we look at the per-function stack cost, we can see the expected minor
stack savings in most functions, albeit outside of the stack hot-path:
function oframe olimit nframe nlimit dframe dlimit
lfsr_file_open 16 2040 16 2032 +0 -8 (+0.0%, -0.4%)
lfsr_rename 240 2128 232 2120 -8 -8 (-3.3%, -0.4%)
lfsr_remove 176 2064 168 2056 -8 -8 (-4.5%, -0.4%)
lfsr_file_opencfg 136 2024 128 2016 -8 -8 (-5.9%, -0.4%)
|
||
|
Christopher Haster
|
ca2d0b980c |
Dropped LFSR_CAT_CAT
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%)
|
||
|
Christopher Haster
|
77bfcb69ad |
Ripped out attr-list data/buf allocators, hand allocated necessary state
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%)
|
||
|
Christopher Haster
|
78b92cc954 |
Replaced attr-list datas/buf arrays with union
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
|
0509fba9b9 |
Replaced attr-list arenas with three independent arrays
lfsr_attr_t attrs[a*d*b]; => lfsr_attr_t attrs[a];
lfs_size_t attr_count; lfs_size_t attr_count;
lfs_size_t attr_scratch; lfsr_data_t datas[d];
lfs_size_t data_count;
uint8_t buf[b];
lfs_size_t buf_size;
This mostly reverts the allocator scaffolding needed for the attr-list
arenas (LFS_ALIGNOF, etc). This is the main draw of this change, as it
would be nice to avoid a low-level arena implementation headaches unless
they prove to be worthwhile. Which they haven't really so far...
Unfortunately this comes with another code cost, I think due to the
number of counters needed to keep track of separate attr/data/buf
allocations. At least stack showed a slight improvement:
code stack
before: 33844 2824
after: 33872 (+0.1%) 2816 (-0.3%)
|
||
|
Christopher Haster
|
8f3036f1e5 |
Unified attr-list context into little attr arenas
The idea is for cases where we need to incrementally allocate attrs +
context, to allocate from both sides of a statically allocated attr
array. This keeps all of the attr-list state in one place, simplifying
state allocation:
.---+---+---+---.
| attr |
+---+---+---+---+
| attr ----------.
+---+---+---+---+ |
| attr --------. |
+---+---+---+---+ | |
| | | | |
| v | | |
| | | |
| ^ | | |
| | | | |
+---+---+---+---+ | |
| data | <-' |
+---+---+---+---+ |
| encoded bptr | <---'
'---+---+---+---'
This is especially useful for the non-terminating tail-recursive
lfsr_btree_commit_, which needs to pass this state through a function
call.
Unfortunately, to make this work we needed to implement more-or-less a
full arena allocator, complete with annoying alignment handling. alignof
isn't even available in C99, so we needed a few more intrinsics:
- LFS_ALIGNOF(t) - Alignment of type t
- LFS_ALIGNEDSIZEOF(t) - Necessary size to force alignment for t
- LFS_MIN(a, b) - Compile-time min
- LFS_MAX(a, b) - Compile-time max
Technically only LFS_ALIGNOF was required, but the others are nice to
have. LFS_MIN/LFS_MAX is also useful anywhere you need to calculate
complicated compile-time sizes.
At least in C11 we get alignof, so we won't need compiler extensions/
hacks for this in the future...
---
Unfortunately this ended up a net-negative. Pushing up the code/stack
cost to near pre-cat levels:
code stack
before cat: 33856 2824
before scratch: 33812 (-0.1%) 2800 (-0.8%)
after: 33844 (-0.0%) 2824 (+0.0%)
I think the two main culprits are 1. the extra logic needed to calculate
alignment, and 2. wasted stack due to aligning scratch space up to the
nearest lfsr_attr_t.
|
||
|
Christopher Haster
|
88a098c616 |
Added lfsr_cat_t to represent concatenated data
So now, instead of one data type trying to do everything, we have two:
1. lfsr_data_t - Readable data, either in-RAM or on-disk
2. lfsr_cat_t - Concatenated data for progging, may be either a simple
in-RAM buffer or an indirect list of lfsr_data_ts
This comes from an observation that most lfsr_attr_t datas were either
simple buffers, NULL, or required the indirect concatenated datas
anyways (concatendated file fragments). By separating lfsr_cat_t and
lfsr_data_t, maybe we can save RAM in lfsr_attr_t by not needing the
three words necessary for the less-common disk references.
Note the interesting tradeoff:
Simple in-RAM buffers/NULL decrease by 1 word (4 bytes):
lfsr_data_t lfsr_cat_t
.---+---+---+---. .---+---+---+---.
|0| size | => |0| size |
+---+---+---+---+ +---+---+---+---+
| ptr | | ptr |
+---+---+---+---+ '---+---+---+---'
| (unused) |
'---+---+---+---'
'-------.-------' '-------.-------'
12 bytes 8 bytes
While on-disk references increase by 2 words (8 bytes):
lfsr_data_t lfsr_cat_t lfsr_data_t
.---+---+---+---. .---+---+---+---. .---+---+---+---.
|1| size | => |1| size | .>|1| size |
+---+---+---+---+ +---+---+---+---+ | +---+---+---+---+
| block | | ptr -------' | block |
+---+---+---+---+ '---+---+---+---' +---+---+---+---+
| off | | off |
'---+---+---+---' '---+---+---+---'
'-------.-------' '-----------------.-----------------'
12 bytes 20 bytes
Unless the on-disk references also need concatenation, in which case
this still saves 1 word (4 bytes).
Note I'm not sure this type split is generalizable to other systems. In
littlefs we can't use recursion, so progging concatenated datas already
required two nested functions, and we happen to never need to read
concatenated data, allowing us to completely omit that functionality. In
other systems, where maybe disk-reference attrs are more common, this
tradeoff may not make sense.
Some other things to note:
- We're also losing the inlined-data representation in this change.
Unfortunately earlier lfsr_data_t measurements showed that this didn't
really contribute much. It saved RAM in name attrs but added quite a
bit of complexity to lfsr_data_t operations.
- By separating simple/cat and RAM/disk, we reduce the abused size bits
from 2-bits down to 1-bit. This doesn't really matter for our current
31/28-bit littlefs impl, but is nice in that it reenables the
theoretical 31/31-bit littlefs impl without in-RAM data-structure
changes.
There are a few temporary hacks that need to be figured out, but this is
already showing code/stack savings. Which is fascinating considering the
new lfsr_cat_* functions and increased temporary allocations:
code stack
before: 33856 2824
after: 33812 (-0.1%) 2800 (-0.8%)
|
||
|
Christopher Haster
|
3f11e6c4a1 |
Tweaked *_compact() arg order
This feels more correct.
*_compact() is notably inconsistent with *_commit/appendattrs(), but I
think this is more a case of attrs/attr_count being a special case.
Code changes, this seems to just be compiler noise. Arg order can affect
quite a bit:
code stack
before: 33876 2824
after: 33856 (-0.1%) 2824 (+0.0%)
|
||
|
Christopher Haster
|
580ff26024 |
Prefer unsigned ints when using sign-bit as a flag
Note this is slightly different than cases where we use the sign-bit for
muxing two different types, such as `int err` and `lfsr_srid_t rid`. In
those cases we'd never extract the lower bits of the int
unconditionally.
This leads to fewer casts and I think signals the intention of these
sign-bit-is-flag ints a bit better. We aren't really interpreting these
as signed, and mask out other bits in some cases (lfsr_data_t).
This leads to more code in places, I'm guessing because of C treating
signed overflow as undefined behavior... Maybe this is a good thing:
code stack
before: 33856 2824
after: 33876 (+0.1%) 2824 (+0.0%)
|
||
|
Christopher Haster
|
6ad4cd5168 |
Dropped *_hastrunk() functions
We can just rely on the truthiness of *_trunk() here. |
||
|
Christopher Haster
|
0ed38211bf |
Made lfsr_shrub_t its own struct
This now properly encodes the different eoff/estimate field usage
between the two types.
In theory this could save some RAM, but we don't actually allocate
lfsr_shrub_t anywhere it's not unioned with lfsr_btree_t, so:
code stack
before: 33976 2824
after: 33976 (+0.0%) 2824 (+0.0%)
|
||
|
Christopher Haster
|
45a4e9ffb4 |
Moved lfsr_ecksum_t back into lfs.c
Now that becksums were proven to not work, we don't need this in lfs.h anymore. |
||
|
Christopher Haster
|
ab2a1cb571 |
Enabled erase=noop in test_rbyd, changed read* to error on leb128 overflow
Now that reproducibility issues with erase_value=-1 (erase=noop) are
fixed, this much more useful to test than erase_value=0x1b. Especially
since erase=noop is filled with so many sharp corners.
These tests already found that we were being too confident with our
leb128/lleb128/tag parsing. Since we need to partially parse unfinished/
old commits, lfsr_dir_read* can easily encounter invalid leb128s during
normal operation. If this happens we should not assert.
Doing things correctly has a bit of a cost:
code stack
before: 33928 2824
after: 33976 (+0.1%) 2824 (+0.0%)
At least we haven't seen any issues with our valid bit invalidating
logic yet.
|
||
|
Christopher Haster
|
b122a50b6c |
Trying to handle ecksums correctly when erased=>LFS_ERR_CORRUPT
It should be legal for block devices to return LFS_ERR_CORRUPT when
erased, this is common on devices with ECC, where the erased-state is
not valid ECC and results in LFS_ERR_CORRUPT.
If anything this is a better indicator than fixed-value erased-state,
but we need to make sure we track this with our ecksums consistently.
This gets a bit arbitrary.
Normally:
valid = m[0] & 0x80
cksum = crc32c(m)
If bd_read returns LFS_ERR_CORRUPT:
valid = 0 & 0x80
cksum = crc32c([])
Yeah, implementing this gets a bit funky, but the code cost is trivial:
code stack
before: 33924 2824
after: 33928 (+0.0%) 2824 (+0.0%)
Note this is only best effort right now, we really need tests over
erased=>LFS_ERR_CORRUPT...
|
||
|
Christopher Haster
|
dbe503776d |
Added lfs_parity intrinsic
We're using parity a lot more than popc now (actually, now that we don't use CTZ skip-lists, do we use popc at all?), so it makes sense to the compiler's __builtin_parity intrinsic when possible. On some processors parity can be much cheaper than popc. Notably, the 8080 family just includes a parity flag in the set of carry flags that are implicitly updated on most ALU operations. Though I think this approach didn't scale, you don't really see parity flags on most >8-bit architectures... Unfortunately, ARM thumb, our test arch, does not have a popc or parity instruction. I guess because thanks to implicit shifts in most instructions, the tree-reduction solution is surprisingly cheap: ea80 4010 eor.w r0, r0, r0, lsr #16 ea80 2010 eor.w r0, r0, r0, lsr #8 ea80 1010 eor.w r0, r0, r0, lsr #4 ea80 00c0 eor.w r0, r0, r0, lsr #2 ea80 0050 eor.w r0, r0, r0, lsr #1 f000 0001 and.w r0, r0, #1 Both popc and parity benefit from this (GCC 11): code __popcountsi2: 40 __paritysi2: 32 (-20.0%) So, thumb is not an arch where we see much benefit: code stack before: 33908 2824 after: 33924 (+0.0%) 2824 (+0.0%) Not really sure where the +16 bytes come from, we removed several masks, so I guess it's just bool vs in compiler noise? Still, this may be useful for other archs with parity instructions/ hardware. |
||
|
Christopher Haster
|
1c9cc63994 |
Adopted crc32c xor trick to avoid masking valid bits
Turns out these are equivalent:
cksum' = crc32c([d & ~0x80], cksum)
cksum' = crc32c([d], cksum ^ (d & 0x80))
Which is quite nice. The second form is a bit cheaper and works better
in situations where you may have an immutable buffer.
I took the long way to find this and may or may not have brute forced
an xor mask for the valid bit:
crc32c(62 95 e3 fd 00) => c7844d4d
crc32c(00 00 00 00 80) => c7844d4d
But this is equivalent to 00000080 after xoring in the init junk.
If you look at the naive lfs_crc32c impl, the first step is to xor the
first byte, so really xoring any byte will cancel it out of our crc32c.
Code changes, thought this would save more because we can reuse bd
checksumming a bit better... Oh well, at least the theory works:
code stack
before: 33916 2824
after: 33908 (-0.0%) 2824 (+0.0%)
|
||
|
Christopher Haster
|
8a75a68d8b |
Made rbyd cksums erased-state agnostic
Long story short, rbyd checksums are now fully reproducible. If you
write the same set of tags to any block, you will end up with the same
checksum.
This is actually a bit tricky with littlefs's constraints.
---
The main problem boils down to erased-state. littlefs has a fairly
flexible model for erased-state, and this brings some challenges. In
littlefs, storage goes through 2 states:
1. Erase - Prepare storage for progging. Reads after an erase may return
arbitrary, but consistent, values.
2. Prog - Program storage with data. Storage must be erased and no progs
attempted. Reads after a prog must return the new data.
Note in this model erased-state may not be all 0xffs, though it likely
will be for flash. This allows littlefs to support a wide range of
other storage devices: SD, RAM, NVRAM, encryption, ECC, etc.
But this model also means erased-state may be different from block to
block, and even different on later erases of the same block.
And if that wasn't enough of a challenge, _erased-state can contain
perfectly valid commits_. Usually you can expect arbitrary valid cksums
to be rare, but thanks to SD, RAM, etc, modeling erase as a noop, valid
cksums in erased-state is actually very common.
So how do we manage erased-state in our rbyds?
First we need some way to detect it, since we can't prog if we're not
erased. This is accomplished by the forward-looking erased-state cksum
(ecksum):
.---+---+---+---. \
| commit | |
| | |
| | |
+---+---+---+---+ +-.
| ecksum -------. | | <-- ecksum - cksum of erased state
+---+---+---+---+ | / |
| cksum --------|---' <-- cksum - cksum of commit,
+---+---+---+---+ | including ecksum
| padding | |
| | |
+---+---+---+---+ \ |
| erased | +-'
| | /
. .
. .
You may have already noticed the start of our problems. The ecksum
contains the erased-state, which is different per-block, and our rbyd
cksum contains the ecksum. We need to include the ecksum so we know if
it's valid, but this means our rbyd cksum changes block to block.
Solving this is simple enough: Stop the rbyd's canonical cksum before
the ecksum, but include the ecksum in the actual cksum we write to disk.
Future commits will need to start from the canonical cksum, so the old
ecksum won't be included in new commits, but this shouldn't be a
problem:
.---+---+---+---. . . \ . \ . . . . .---+---+---+---. \ \
| commit | | | | commit | | |
| | | +- rbyd | | | |
| | | | cksum | | | |
+---+---+---+---+ +-. / +---+---+---+---+ | |
| ecksum -------. | | | ecksum | . .
+---+---+---+---+ | / | +---+---+---+---+ . .
| cksum --------|---' | cksum | . .
+---+---+---+---+ | +---+---+---+---+ . .
| padding | | | padding | . .
| | | | | . .
+---+---+---+---+ \ | . . . . . . . +---+---+---+---+ | |
| erased | +-' | commit | | |
| | / | | | +- rbyd
. . | | | | cksum
. . +---+---+---+---+ +-. /
| ecksum -------. | |
+---+---+---+---+ | / |
| cksum ------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ \ |
| erased | +-'
| | /
. .
. .
The second challenge is the pesky possibility of existing valid commits.
We need some way to ensure that erased-state following a commit does not
accidentally contain a valid old commit.
This is where are tag's valid bits come into play: The valid bit of each
tag must match the parity of all preceding tags (equivalent to the
parity of the crc32c), and we can use some perturb bits in the cksum tag
to make sure any tags in our erased-state do _not_ match:
.---+---+---+---. \ . . . . . .---+---+---+---. \ \ \
|v| tag | | |v| tag | | | |
+---+---+---+---+ | +---+---+---+---+ | | |
| commit | | | commit | | | |
| | | | | | | |
+---+---+---+---+ +-----. +---+---+---+---+ +-. | |
|v|p| tag | | | |v|p| tag | | | | |
+---+---+---+---+ / | +---+---+---+---+ / | | |
| cksum | | | cksum | | . .
+---+---+---+---+ | +---+---+---+---+ | . .
| padding | | | padding | | . .
| | | | | | . .
+---+---+---+---+ . . . | . . +---+---+---+---+ | | |
|v---------------- != --' |v------------------' | |
| erased | +---+---+---+---+ | |
. . | commit | | |
. . | | | |
+---+---+---+---+ +-. +-.
|v|p| tag | | | | |
+---+---+---+---+ / | / |
| cksum ----------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ |
|v---------------- != --'
| erased |
. .
. .
New problem! The rbyd cksum contains the valid bits, which contain the
perturb bits, which depends on the erased-state!
And you can't just derive the valid bits from the rbyd's canonical
cksum. This avoids erased-state poisoning, sure, but then nothing in the
new commit depends on the perturb bits! The catch-22 here is that we
need the valid bits to both depend on, and ignore, the erased-state
poisoned perturb bits.
As far as I can tell, the only way around this is to make the rybd's
canonical cksum not include the parity bits. Which is annoying, masking
out bits is not great for bulk cksum calculation...
But this does solve our problem:
.---+---+---+---. \ . . . . . .---+---+---+---. \ \ \ \
|v| tag | | |v| tag | | | o o
+---+---+---+---+ | +---+---+---+---+ | | | |
| commit | | | commit | | | | |
| | | | | | | | |
+---+---+---+---+ +-----. +---+---+---+---+ +-. | | |
|v|p| tag | | | |v|p| tag | | | | . .
+---+---+---+---+ / | +---+---+---+---+ / | | . .
| cksum | | | cksum | | . . .
+---+---+---+---+ | +---+---+---+---+ | . . .
| padding | | | padding | | . . .
| | | | | | . . .
+---+---+---+---+ . . . | . . +---+---+---+---+ | | | |
|v---------------- != --' |v------------------' | o o
| erased | +---+---+---+---+ | | |
. . | commit | | | +- rbyd
. . | | | | | cksum
+---+---+---+---+ +-. +-. /
|v|p| tag | | | o |
+---+---+---+---+ / | / |
| cksum ----------------'
+---+---+---+---+ |
| padding | |
| | |
+---+---+---+---+ |
|v---------------- != --'
| erased |
. .
. .
Note that because each commit's cksum derives from the canonical cksum,
the valid bits and commit cksums no longer contain the same data, so our
parity(m) = parity(crc32c(m)) trick no longer works.
However our crc32c still does tell us a bit about each tag's parity, so
with a couple well-placed xors we can at least avoid needing two
parallel calculations:
cksum' = crc32c(cksum, m)
valid' = parity(cksum' xor cksum) xor valid
This also means our commit cksums don't include any information about
the valid bits, since we mask these out before cksum calculation. Which
is a bit concerning, but as far as I can tell not a real problem.
---
An alternative design would be to just keep track of two cksums: A
commit cksum and a canonical cksum.
This would be much simpler, but would also require storing two cksums in
RAM in our lfsr_rbyd_t struct. A bit annoying for our 4-byte crc32cs,
and a bit more than a bit annoying for hypothetical 32-byte sha256s.
It's also not entirely clear how you would update both crc32cs
efficiently. There is a way to xor out the initial state before each
tag, but I think it would still require O(n) cycles of crc32c
calculation...
As it is, the extra bit needed to keep track of commit parity is easy
enough to sneak into some unused sign bits in our lfsr_rbyd_t struct.
---
I've also gone ahead and mixed in the current commit parity into our
cksum's perturb bits, so the commit cksum at least contains _some_
information about the previous parity.
But it's not entirely clear this actually adds anything. Our perturb
bits aren't _required_ to reflect the commit parity, so a very unlucky
power-loss could in theory still make a cksum valid for the wrong
parity.
At least this situation will be caught by later valid bits...
I've also carved out a tag encoding, LFSR_TAG_PERTURB, solely for adding
more perturb bits to commit cksums:
LFSR_TAG_CKSUM 0x3cpp v-11 cccc -ppp pppp
LFSR_TAG_CKSUM 0x30pp v-11 ---- -ppp pppp
LFSR_TAG_PERTURB 0x3100 v-11 ---1 ---- ----
LFSR_TAG_ECKSUM 0x3200 v-11 --1- ---- ----
LFSR_TAG_GCKSUMDELTA+ 0x3300 v-11 --11 ---- ----
+ Planned
This allows for more than 7 perturb bits, and could even mix in the
entire previous commit cksum, if we ever think that is worth the RAM
tradeoff.
LFSR_TAG_PERTURB also has the advantage that it is validated by the
cksum tag's valid bit before being included in the commit cksum, which
indirectly includes the current commit parity. We may eventually want to
use this instead of the cksum tag's perturb bits for this reason, but
right now I'm not sure this tiny bit of extra safety is worth the
minimum 5-byte per commit overhead...
Note if you want perturb bits that are also included in the rbyd's
canonical cksum, you can just use an LFSR_TAG_SHRUBDATA tag. Or any
unreferenced shrub tag really.
---
All of these changes required a decent amount of code, I think mostly
just to keep track of the parity bit. But the isolation of rbyd cksums
from erased-state is necessary for several future-planned features:
code stack
before: 33564 2816
after: 33916 (+1.0%) 2824 (+0.3%)
|
||
|
Christopher Haster
|
c4fcc78814 |
Tweaked file types/name tag encoding to be a bit less quirky
The intention behind the quirky encoding was to leverage bit 1 to
indicate if the underlying file type would be backed by the common file
B-tree data structure. Looking forward, there may be several of these
types, compressed files, contiguous files, etc, that for all intents and
purposes are just normal files interpreted differently.
But trying to leverage too many bits like this is probably going to give
us a sparse, awkward, and confusing tag encoding, so I've reverted to a
hopefully more normal encoding:
LFSR_TAG_NAME 0x02tt v--- --1- -ttt tttt
LFSR_TAG_NAME 0x0200 v--- --1- ---- ----
LFSR_TAG_REG 0x0201 v--- --1- ---- ---1
LFSR_TAG_DIR 0x0202 v--- --1- ---- --1-
LFSR_TAG_SYMLINK* 0x0203 v--- --1- ---- --11
LFSR_TAG_BOOKMARK 0x0204 v--- --1- ---- -1--
LFSR_TAG_ORPHAN 0x0205 v--- --1- ---- -1-1
LFSR_TAG_COMPR* 0x0206 v--- --1- ---- -11-
LFSR_TAG_CONTIG* 0x0207 v--- --1- ---- -111
* Hypothetical
Note the carve-out for the hypothetical symlink tag. Symlinks are
actually incredibly low in the priority list, but they are also
the only current hypothetical file type that would need to be exposed to
users. Grouping these up makes sense.
This will get a bit messy if we ever end up with a 4th user-facing type,
but there isn't any in POSIX at least (ignoring non-fs types, socket,
fifo, character, block, etc).
The gap also helps line things up so reg/orphan are a single bit flip,
and the non-user facing types all share a bit.
This had no impact on code size:
code stack
before: 33564 2816
after: 33564 (+0.0%) 2816 (+0.0%)
|
||
|
Christopher Haster
|
6e5d314c20 |
Tweaked struct tag encoding so b*/m* tags are earlier
These b*/m* struct tags have a common pattern that would be good to
emphasize in the encoding. The later struct tags get a bit more messy as
they leave space for future possible extensions.
New encoding:
LFSR_TAG_STRUCT 0x03tt v--- --11 -ttt ttrr
LFSR_TAG_DATA 0x0300 v--- --11 ---- ----
LFSR_TAG_BLOCK 0x0304 v--- --11 ---- -1rr
LFSR_TAG_BSHRUB 0x0308 v--- --11 ---- 1---
LFSR_TAG_BTREE 0x030c v--- --11 ---- 11rr
LFSR_TAG_MROOT 0x0310 v--- --11 ---1 --rr
LFSR_TAG_MDIR 0x0314 v--- --11 ---1 -1rr
LFSR_TAG_MSHRUB* 0x0318 v--- --11 ---1 1---
LFSR_TAG_MTREE 0x031c v--- --11 ---1 11rr
LFSR_TAG_DID 0x0320 v--- --11 --1- ----
LFSR_TAG_BRANCH 0x032c v--- --11 --1- 11rr
* Hypothetical
Note that all shrubs currently end with 1---, and all btrees, including
the awkward branch tag, end with 11rr.
This had no impact on code size:
code stack
before: 33564 2816
after: 33564 (+0.0%) 2816 (+0.0%)
|
||
|
Christopher Haster
|
5fa85583cd |
Dropped block-level erased-state checksums for RAM-tracked erased-state
Unfortunately block-level erased-state checksums (becksums) don't really
work as intended.
An invalid becksum _does_ signal that a prog has been attempted, but a
valid becksum does _not_ prove that a prog has _not_ been attempted.
Rbyd ecksums work, but only thanks to a combination of prioritizing
valid commits and the use of perturb bits to force erased-state changes.
It _is_ possible to end up with an ecksum collision, but only if you
1. lose power before completing a commit, and 2. end up with a
non-trivial crc32c collision. If this does happen, at the very least the
resulting commit will likely end up corrupted and thrown away later.
Block-level becksums, at least as originally designed, don't have either
of these protections. To make matters worse, the blocks these becksums
reference contain only raw user data. Write 0xffs into a file and you
will likely end up with a becksum collision!
This is a problem for a couple of reasons:
1. Progging multiple times to erased-state is likely to result in
corrupted data, though this is also likely to get caught with
validating writes.
Worst case, the resulting data looks valid, but with weakened data
retention.
2. Because becksums are stored in the copy-on-write metadata of the
file, attempting to open a file twice for writing (or more advanced
copy-on-write operations in the future) can lead to a situation where
a prog is attempted on _already committed_ data.
This is very bad and breaks copy-on-write guarantees.
---
So clearly becksums are not fit for purpose and should be dropped. What
can we replace them with?
The first option, implemented here, is RAM-tracked erased state. Give
each lfsr_file_t its own eblock/eoff fields to track the last known good
erased-state. And before each prog, clear eblock/eoff so we never
accidentally prog to the same erased-state twice.
It's interesting to note we don't currently clear eblock/eoff in all
file handles, this is ok only because we don't currently share
eblock/eoff across file handles. Each eblock/eoff is exclusive to the
lfsr_file_t and does not appear anywhere else in the system.
The main downside of this approach is that, well, the RAM-tracked
erase-state is only tracked in RAM. Block-level erased-state effectively
does not persist across reboots. I've considered adding some sort of
per-file erased-state tracking to the mdir that would need to be cleared
before use, but such a mechanism ends up quite complicated.
At the moment, I think the best second option is to put erased-state
tracking in the future-planned bmap. This would let you opt-in to
on-disk tracking of all erased-state in the system.
One nice thing about RAM-tracked erased-state is that it's not on disk,
so it's not really a compatibility concern and won't get in the way of
additional future erased-state tracking.
---
Benchmarking becksums vs RAM-tracking has been quite interesting. While
in theory becksums can track much more erased-state, it's quite unlikely
anything but the most recent erased-state actually ends up used. The end
result is no real measurable performance loss, and actually a minor
speedup because we don't need to calculate becksums on every block
write.
There are some pathological cases, such as multiple write heads, but
these are out-of-scope right now (note! multiple explicit file handles
currently handle this case beautifully because we don't share
eblock/eoff!)
Becksums were also relatively complicated, and needed extra scaffolding
to pass around/propagate as secondary tags alongside the primary bptr.
So trading these for RAM-tracking also gives us a nice bit of code/stack
savings, albeit at a 2-word RAM cost in lfsr_file_t:
code stack structs
before: 33888 2864 1096
after: 33564 (-1.0%) 2816 (-1.7%) 1104 (+0.7%)
lfsr_file_t before: 104
lfsr_file_t after: 112 (+7.7%)
|
||
|
Christopher Haster
|
799ef63eb8 |
Brought back lfsr_p_* -> lfsr_rbyd_p_*
These are very specific functions for only lfsr_rbyd_appendattr. Associating them with the lfsr_rbyd_* seems like the correct thing to do. |
||
|
Christopher Haster
|
86a8582445 |
Tweaked canonical altn to point to itself
By definition, altns should never be followed, so it doesn't really
matter where they point. But it's not like they can point literally
nowhere, so where should they point?
A couple options:
1. jump=jump - Wherever the old alt pointed
- Easy, literally a noop
- Unsafe, bugs could reveal outdated parts of the tree
- Encoding size eh
2. jump=0 - Point to offset=0
- Easier, +0 code
- Safer, branching to 0 should assert
- Worst possible encoding size
3. jump=itself - Point to itself
- A bit tricky, +4 code
- Safe, should assert, even without asserts worst case infinite loop
- Optimal encoding size
An infinite loop isn't the best failure state, but we can catch this
with an assert, which we would need for jump=0 anyways. And this is only
a concern if there are other fs bugs. jump=0 is actually slightly worse
if asserts are disabled, since we'd end up reading the revision count as
garbage.
Adopting jump=itself gives us the optimal 4-byte encoding:
altbn w0 = 40 00 00 00
'-+-' ^ ^
'----|--|-- tag = altbn
'--|-- weight = 0
'-- jump = itself (branch - 0)
This requires tweaking the alt encoder a bit, to avoid relative encoding
jump=0s, but this is pretty cheap:
code stack
jump=jump: 34068 2864
jump=0: 34068 (+0.0%) 2864 (+0.0%)
jump=itself: 34072 (+0.0%) 2864 (+0.0%)
I thought we may need to also tweak the decoder, so later trunk copies
don't accidentally point to the old location, but humorously our pruning
kicks in redundantly to reset altbn's jump=itself on every trunk.
Note lfsr_rbyd_lookupnext was also rearranged a bit to make it easier to
assert on infinite loops and this also added some code. Probably just
due to compiler noise:
code stack
before: 34068 2864
after: 34076 (+0.0%) 2864 (+0.0%)
Also note that we still accept all of the above altbn encoding options.
This only affects encoding and dbg scripts.
|
||
|
Christopher Haster
|
e8f6b0006c |
Added a comment after mistakenly trying to use altas during rbyd compaction
Spent an embarrassingly long time debugging rbyd over this. It's tempting to terminate inner binary nodes with altas during compaction, since the last alt should always be taken. But it's easy to miss that our compaction algorithm actually relies on copying the tag forward each layer to avoid recursively finding the largest tag. Adding a comment will hopefully prevent the headache for someone else in the future. |
||
|
Christopher Haster
|
faf8c4b641 |
Tweaked alt-tag encoding to match color/dir naming order
This is mainly to avoid mistakes caused by names/encodings disagreeing:
LFSR_TAG_ALT 0x4kkk v1cd kkkk -kkk kkkk
^ ^^ '------+-----'
'-||--------|------- valid bit
'|--------|------- color
'--------|------- dir
'------- key
Notably, the LFSR_TAG_ALT() macro has already caused issues by being
both 1. ambiguous, and 2. not really type-checkable. It's easy to get
the order wrong and things not really break, just behave poorly, it's
really not great!
To be honest the exact order is a bit arbitrary, the color->dir naming
appeared by accident because I guess it felt more natural. Maybe because
of English's weird implicit adjective ordering? Maybe because of how
often conditions show up as the last part of the name in other
instruction sets?
At least one plus is that this moves the dir-bit next to the key. This
makes it so all of the condition information is encoding is the lowest
13-bits of the tag, which may lead to minor optimization tricks for
implementing flips and such.
Code changes:
code stack
before: 34080 2864
after: 34068 (-0.0%) 2864 (+0.0%)
|
||
|
Christopher Haster
|
884982987e |
Tried to adopt consistent flip/flop/follow indention in rbyd functions
The intention here is to try to help readability by keeping arg locations somewhat consistent. Readability is already difficult enough given that these functions are so context dependent... |
||
|
Christopher Haster
|
8bfb1be926 |
Added some more tree transformation comments to lfsr_rbyd_appendattr
Hopefully having something to help visualize these tree operations will help make lfsr_rbyd_appendattr easier to understand. This one function is probably the most complicated function in littlefs, but for good reason. |
||
|
Christopher Haster
|
77c45827e5 |
rbyd-rr: Explicitly deduplicated diverging conditions
I'm not really sure why the compiler isn't taking care of this for us.
Usually I prefer duplicated logic over more variables since it means
less state to keep track of when reading/debugging, and the compiler
will optimize it away anyways. But I guess these conditions are just too
complicated in this case?
Maybe the compiler is trying to take advantage of &&/|| short-circuiting
even with -Os?
Even marking the lfsr_tag_diverging* functions with
__attribute__((noinline, pure, const)) doesn't help...
Oh well, this is a case where we can just make the deduplication
explicit for a bit of code savings:
code stack
before: 34176 2864
after: 34080 (-0.3%) 2864 (+0.0%)
code frame stack
appendattr before: 2162 208 560
appendattr after: 2104 (-2.7%) 216 (+3.8%) 568 (+1.4%)
|
||
|
Christopher Haster
|
94eb672315 |
rbyd-rr: Rearranged diverged pruning/trimming after flipping
This was a bit more tricky than the other eager-flip related
transformations, mainly because we have to be careful to not prune the
diverging alt that connects the two diverged trunks. The diverging alt,
i.e. the first alt that diverges, passes all the criteria for pruning,
but is a bit special in that we need to keep it around until we stitch
the trunks together.
I ended up more-or-less just reverting the handling of both-diverging
nodes to being collapsed as a special case of our first encounter with
the diverging alt. Because we eagerly prune, both-diverging nodes can
only happen if they include the diverging alt. We can leveraging this to
simplify our diverging logic a bit, which is already crazy complicated.
Not only does this finish moving all of the alt-related logic into
"flipped space", it also moves all of the diverging logic together,
which is more readable and hopefully leads to better code deduplication
by the compiler.
Long story short, more code savings!
code stack
before: 34244 2864
after: 34176 (-0.2%) 2864 (+0.0%)
code frame stack
appendattr before: 2232 216 568
appendattr after: 2162 (-3.1%) 208 (-3.7%) 560 (-1.4%)
---
All of these code savings are making our 2-trunk range removal algorithm
more appealing:
code stack
rr-div-naive: 33968 2864
rr-div-altn: 34304 (+1.0%) 2864 (+0.0%)
rr-2trunk-altn: 34176 (+0.6%) 2864 (+0.0%)
code frame stack
appendattr rr-stitching: 1940 184 536
appendattr rr-div-naive: 2028 (+4.5%) 200 (+8.7%) 552 (+3.0%)
appendattr rr-div-altn: 2198 (+13.3%) 216 (+17.4%) 568 (+6.0%)
appendattr rr-2trunk-altn: 2162 (+11.4%) 208 (+13.0%) 560 (+4.5%)
That being said, it is getting increasingly hard to compare these
functions. You could argue the eager-flip transformations would also
result in code savings for the earlier iterations of our algorithm,
but it is worth noting the 2-trunk approach _did_ require more flips to
get working, so...
|
||
|
Christopher Haster
|
00a2332417 |
rbyd-rr: Tweaked both-diverged trimming to not pop
This adds some code:
code stack
before: 34224 2864
after: 34244 (+0.1%) 2864 (+0.0%)
code frame stack
appendattr before: 2190 216 568
appendattr after: 2232 (+1.9%) 216 (+0.0%) 568 (+0.0%)
But makes it so both diverged-trimming cases end up with a zero weight
unreachable alt, which may lead to more simplification...
|
||
|
Christopher Haster
|
82ddb33510 |
rbyd-rr: Rearranged pruning to only need lfsr_tag_unreachable*
An excellent example of the sort of simplification that eagerly flipping
gives us.
By flipping _before_ pruning, all unavoidable alts are transformed into
unreachable. This lets us check for one condition instead of two:
code stack
before: 34320 2864
after: 34224 (-0.3%) 2864 (+0.0%)
code frame stack
appendattr before: 2280 216 568
appendattr after: 2190 (-3.9%) 216 (+0.0%) 568 (+0.0%)
|
||
|
Christopher Haster
|
8f8dd9f981 |
rbyd-rr: Eagerly flip, adopt branch before/after to disambiguate ysplits
It's been annoying for a while how many flip operations we need in
lfsr_rbyd_appendattr to implement diverging range removals correctly.
Unfortuantely, we need all of these flips since we need to know the
original alt ordering in order to know how to split yellow nodes.
Keep in mind yellow splits depend on what alts exist in our history:
<y >b
.-------'| .-'|
| <r take red/yellow | >b
| .----'| => .-----|-'|
| | <b | <b |
| | .-'| | .-'| |
1 2 3 4 1 2 3 4 1
<b
.-'|
<y |
take black .-------'| |
=> | <r |
| .----' |
| | <b
| | .----'|
1 2 3 4 4
Or so I thought! Turns out there is a sort of hack we can use to
figure out the yellow split even after flipping.
Take a look at this example yellow node, and the various possible
jump/branch destinations:
.-- branch = 0xb20
00000b10: altrle 0x401 w0 0xa10 -|-> p[0].jump = 0xa10
00000b20: altrle 0x402 w0 0xa20 <'-> jump = 0xa20
00000b30: altble 0x403 w0 0xa30 <--- branch_ = 0xb30
Anything jump out? That's right! only branch_ is > branch.
This holds even after flips:
branch = 0xb20 branch = 0xb20 flip2 branch = 0xb20
p[0].jump = 0xa10 flip p[0].jump = 0xa10 --.---> p[0].jump = 0xb30
jump = 0xa20 --.--> jump = 0xb30 --'-.-> jump = 0xa20
branch_ = 0xb30 --'--> branch_ = 0xa20 ----'-> branch_ = 0xa10
This is provable by noting that our alts can't even encode forward
jumps. So... proof by lack of encoding?
We can use this to determine which yellow split is needed even after
flipping:
- branch_ < branch && jump < branch => take yellow alt
- branch_ < branch && jump > branch => take red alt
- branch_ > branch => take black alt
This lets us move/deduplicate the flipping logic before the diverging
logic and operate in a sort of "flipped space", where branch_ is always
the next branch we will take.
Unfortunately we do need to flip red alts that don't get split back
before descending down red nodes, which sort of matches our weird access
pattern, but this extra flip is well worth the code savings elsewhere.
---
This greatly simplifies the state space of lfsr_rbyd_appendattr, and it
already shows in code size measurements:
code stack
before: 34528 2864
after: 34320 (-0.6%) 2864 (+0.0%)
code frame stack
appendattr before: 2378 216 568
appendattr after: 2280 (-4.1%) 216 (+0.0%) 568 (+0.0%)
But this is really only after simplifying the diverging logic and yellow
splits. I think there may be even more savings if we can figure out how
to move all of the alt logic into the "flipped space"...
|
||
|
Christopher Haster
|
f06ef46e8b |
rbyd-rr: Simplified diverging state machine, rely on relative a/b ordering
So instead of explicitly keeping track of which bound we are on, either via
separate DIVERGEDLOWER/DIVERGEDUPPER states or a d_upper bool, we can
infer the bound based on the relative ordering a_rid/tag and b_rid/tag:
- a_rid < b_rid || a_tag < b_tag => lower bound
- a_rid > b_rid || a_tag > b_tag => upper bound
- a_rid == b_rid && a_tag == b_tag => not diverging
This is more appealing now that we don't rely on the specific bound for
diverged triming. The only remaining state is if we have diverged yet, a
simple boolean.
Measuring code size was a bit confusing. During a partial edit, it
looked like this was going to save a bit of code, but the result was
actually worse. It seems that explicitly masking/oring a single bit in
the original uint8_t d_state is somehow cheaper than storing if we have
diverged as a bool?
code stack
before: 34516 2864
bitmask: 34504 (-0.0%) 2864 (+0.0%)
boolean: 34528 (+0.0%) 2864 (+0.0%)
code frame stack
appendattr before: 2366 216 568
appendattr bitmask: 2354 (-0.5%) 216 (+0.0%) 568 (+0.0%)
appendattr boolean: 2378 (+0.5%) 216 (+0.0%) 568 (+0.0%)
No idea why this would happen. If feels like some sort of
compiler/optimizer bug... But this is pretty close to the compiler noise
floor and compilers aren't perfect. I'm probably reading too much into
an extra 24 bytes...
This is still a worthwhile change as it's usually good to prefer
implicit state over explicit. Less things can fall out of sync this way.
|
||
|
Christopher Haster
|
ffc36b0f36 |
rbyd-rr: Added lfsr_tag_diverging and lfsr_tag_diverging2
If nothing else these at least makes the code a bit more readable.
Curiously this improved lfsr_rbyd_appendattr, but made the total code
size worse. I guess these really should be inlined, but don't pass some
compiler heuristic. Oh well, optimization is a hard problem:
code stack
before: 34492 2864
after: 34516 (+0.1%) 2864 (+0.0%)
code frame stack
appendattr before: 2414 216 568
appendattr after: 2366 (-2.0%) 216 (+0.0%) 568 (+0.0%)
|
||
|
Christopher Haster
|
660d323564 |
rbyd-rr: Renamed lfsr_rbyd_p_* -> lfsr_p_*
I didn't notice the inconsistency at first, but with the addition of the diverging state machine, we have to subcomponents in lfsr_rbyd_appendattr with different naming conventions: - lfsr_rbyd_p_* - the p-alt fifo - lfsr_d_* - the diverging state machine One of these needs to change, and lfsr_rbyd_d_isdiverged is such a keyful... |
||
|
Christopher Haster
|
d3e09b082f |
rbyd-rr: Minor tweaks, adopted diverging check for diverged triming
Previously we used the direction of post-diverged alts to decide if they
need to be trimmed or not:
lfsr_d_isdiverged(d_state)
&& lfsr_d_isupper(d_state)
^ lfsr_tag_isgt(alt)
^ lfsr_tag_follow2(
alt, weight,
p[0].alt, p[0].weight,
lower_rid, upper_rid,
a_rid, a_tag)
But this working is a bit accidental. The real condition that needs to
be met for trimming is if our bounds continue to diverge on the alt:
lfsr_d_isdiverged(d_state)
&& lfsr_tag_follow2(
alt, weight,
p[0].alt, p[0].weight,
lower_rid, upper_rid,
a_rid, a_tag)
^ lfsr_tag_follow2(
alt, weight,
p[0].alt, p[0].weight,
lower_rid, upper_rid,
b_rid, b_tag)
This may seem more complicated, and does add code, but I'm hopeful it
can eventually lead to better code deduplication with the preceding
not-diverged -> diverged checks:
code stack
before: 34468 2864
after: 34492 (+0.1%) 2864 (+0.0%)
code frame stack
appendattr before: 2390 216 568
appendattr after: 2414 (+1.0%) 216 (+0.0%) 568 (+0.0%)
I've also been trying to simplify/deduplicate the diverging logic more,
but it's proven difficult. There's an annoying catch-22 where 1. we need
to trim diverging alts before applying color transformations, but 2. we
need to resolve yellow splits before triming diverging alts.
|
||
|
Christopher Haster
|
01b28b3224 |
rbyd-rr: Rearranged some things so appendattr gotos make a bit more sense
- Renamed again: -> trunk:
- Added stem:, moved the awkward pre-stem logic into the not-alt check
- Kept leaf: unchanged
This organizes lfsr_rbyd_appendattr into logical trunk -> stem -> leaf
stages, which I think makes quite a bit of sense.
GCC is happy if we change the loop termination into goto stem, but I
think it's quite unfortunate that GCC's -Wunused-label warning
discourages labels for purely code organization. They're quite useful
for organizing complicated functions at a level higher than comments,
and GDB's break func:label syntax shows potential for external tooling.
Maybe we should disable -Wunused-label?
---
Not sure why this impacted code size, the transformation should have
been a noop. Then again, it's not too surprising, gotos are supposedly
pretty annoying to optimize around:
code stack
before: 34480 2864
after: 34468 (-0.5%) 2864 (+0.0%)
code frame stack
appendattr before: 2404 216 568
appendattr after: 2390 (-0.6%) 216 (+0.0%) 568 (+0.0%)
|
||
|
Christopher Haster
|
54c8beee70 |
rbyd-rr: Adopted a struct-based p-alt fifo representation
So instead of:
lfsr_tag_t p_alts[3];
lfsr_rid_t p_weights[3];
lfs_size_t p_jumps[3];
We now have:
lfsr_alt_t p[3];
Note this is the only place where we use the new lfsr_alt_t type,
hopefully using such a general name doesn't create confusion down the
road...
I was mostly just curious which representation the compiler
(GCC 11.4 -mthumb) would handle better. In theory a struct
representation will result in more efficient memmoves, since we usually
operate on entire alts at a time when manipulting our fifo.
The original motivation for the separate arrays was to avoid alignment
issues with the 16-bit lfsr_tag_t, but this was apparently premature:
code stack
before: 34644 2864
after: 34480 (-0.5%) 2864 (+0.0%)
code frame stack
appendattr before: 2452 216 568
appendattr after: 2404 (-2.0%) 216 (+0.0%) 568 (+0.0%)
Actually, it's a bit strange that lfsr_rbyd_appendattr showed _no_ stack
changes... I wonder why that is?
|
||
|
Christopher Haster
|
eb2c7a9a05 |
rbyd-rr: Switched diverging state from bools to a small state machine
The state machine is pretty simple:
NOTDIVERGEDLOWER
|
diverging?-no--.
yes |
v |
DIVERGEDLOWER |
| |
v |
NOTDIVERGEDUPPER |
| |
v |
DIVERGEDUPPER |
'--------. |
v v
done
The nice thing about the 2-trunk algorithm is we don't need any extra
states for cleanup and we don't need to predict if we will diverge or
not. The always start by writing out the common trunk, and switch to the
diverging state machine retroactively if necessary.
With only 4 states, the difference between bools and a small state
machine is negligible. I was mostly just curious which approach the
compiler (GCC 11.4 -mthumb) could optimize better.
Which is apparently the state machine:
code stack
before: 34656 2864
after: 34644 (-0.0%) 2864 (+0.0%)
code frame stack
appendattr before: 2464 224 576
appendattr after: 2452 (-0.5%) 216 (-3.6%) 568 (-1.4%)
Though word of warning, this is basically the compiler's noise floor.
|
||
|
Christopher Haster
|
64046d495e |
rbyd-rr: Cleaned up new 2-trunk range-removal algorithm
Removed a bunch of outdated code, printfs, old diverging state machine,
updated comments, etc.
Also tried to simplify the diverging alt logic as much as possible, but
the logic is quite stubborn. We can at least make some interesting
assumptions about alt ordering on the upper-diverged path, since we know
the lower-diverged path will flip and collapse 2-3 nodes.
---
Now that the dust has settled (again), we can compare our new 2-trunk
algorithm to our previous attempts:
code stack
rr-div-naive: 33968 2864
rr-div-altn: 34304 (+1.0%) 2864 (+0.0%)
rr-2trunk-altn: 34656 (+2.0%) 2864 (+0.0%)
Focusing on lfsr_rbyd_appendattr, which lets us compare further back in
history:
code frame stack
appendattr rr-stitching: 1940 184 536
appendattr rr-div-naive: 2028 (+4.5%) 200 (+8.7%) 552 (+3.0%)
appendattr rr-div-altn: 2198 (+13.3%) 216 (+17.4%) 568 (+6.0%)
appendattr rr-2trunk-altn: 2464 (+27.0%) 224 (+21.7%) 576 (+7.5%)
And comparing the resulting tree color-balance:
2-tree 2-3-4-tree
rr-stitching: +~2x +~2x
rr-div-naive: +0 +~2x
rr-div-altn: +0 +~1 on red
rr-2trunk-altn: +0 +~1 on yellow
It's again an annoyingly expensive algorithm change, but necessary to
maintain the correct balance of our rbyds as much as possible. Keep in
mind range operations are used _everywhere_ in the high-level operations
in our filesystem. It's just too useful a tool.
The "+~1 on yellow" vs "+~1 on red" may not seem like that much of an
improvement, but keep in mind yellow alts are much less common, and
temporary. Decaying into black alts on the next append. At rest, most
alts are either black or red.
It's also worth mentioning that, in theory, the rr-2trunk-altn approach
_could_ be extended to be perfectly balancing, but this would likely
require duplicating the entire yellow-split logic, which is probably not
worth it in this implemention...
|
||
|
Christopher Haster
|
9c8a44a461 |
rbyd-rr: Enabled color preservation on diverging-lower alt
It's a great sign that this just worked.
Now, the only case where coloring is not preserved is the
diverging-upper alt, and only when encountering a yellow node. A rather
complicated corner case:
.-> .-> .-> h=4 -.
.-----b-> .-b-> .-b-> |
| .-> | .-> | .-> |
| .---b-> .-y-b-> .-y-b-> |
| | .-> | .-> | .-> |
| | .-b-> | .-b-> | .-b-> |
| | | .-> y-r-b-b-b-> .-b-b-b-> |
.-y-r-b-b-> rm me => | | => | +- unbal :(
| .-> rm me | | .-> | |
| .-b-> | | .-b-> r-b---b-b-> |
| | .-> | | | .-> | | .-> |
| .---b-b-> | '---b-b-> | '-b-> |
| | .-> | .-> | .-> |
| | .-b-> | .-b-> | .-b-> |
| | | .-> | | .-> | | .-> |
r-b---b-b-> '-----b-b-> '-----b-b-> h=3 -'
^ ^
diverging diverging/stitching
In theory it _is_ possible to preserve coloring on yellow nodes, but
right now this only seems possible by duplicating most of the
yellow-split logic, which doesn't seem worth it...
|
||
|
Christopher Haster
|
f957dad821 |
rbyd-rr: Implemented very ugly, but working! diverging 2-3 nodes
It's a mess, but all tests are passing.
We're still recoloring the diverging alt, so hopefully I won't need to
eat my words, but at least on paper this should be able to preserve
colors for all 2-3 permutations of the diverging alt.
The key observation here is that diverging 2-3 nodes have three possible
permutations:
1. Diverging on the black alt:
.-> .-> .->
.---b-> .---b-> .---b->
| .-> => r-b-b-> => | .-b->
| .-b-> rm me | | |
| | .-> rm me | .-> | |
r-b-b-> '-b-> r-b-b->
^ ^
diverging diverging
2. Diverging on the red alt:
.-> r-b-b-> .--->
.---b-> rm me | | |
| .-> rm me => | | .-> => |
| .-b-> | '-b-> r-b-b->
| | .-> | .-> | .->
r-b-b-> '---b-> '---b->
^ ^
diverging diverging
3. Diverging on both alts:
.-> b---b-> .---b->
.---b-> rm me | |
| .-> rm me => | => |
| .-b-> rm me | |
| | .-> rm me | .-> |
r-b-b-> '---b-> b---b->
^^^ ^
diverging diverging
With 3., both diverging, being the tricky one, where we need to both
switch to the diverged state while also collapsing the 3-node into a
2-node.
1. and 2. can both be deduplicated with a well-timed flip, but so far
it seems like 3. needs its own special case. At least these can all be
contained as extra conditions in the diverging alt logic, reducing the
possible states.
lfs_rbyd_appendattr is a complete mess now, and a lot of the diverging
logic is duplicated everywhere, but at least things seem to be working.
|
||
|
Christopher Haster
|
c370fbec1a |
rbyd-rr: Limping along, fixed test_files_many, all tests are passing now
The issue, found in test_files_many:h1g4j10l18, occurs when a SUBWIDE
tag follows a compaction.
When this happens, it's possible for our stitched diverging alt to be
followed/flipped when it shouldn't be. This is because the new
lower_rid/upper_rid window can make the stitched alt ambiguous.
I don't think this is strictly an issue with compaction, as much as
compaction is giving us a tree structure that's not reachable through
only appendattrs.
Here are the three culprit trunks:
altrle 0x300 w8 0x2c8
altble 0x203 w6 0x2d4 <- diverge
null
altrle 0x300 w8 0x2c8
altbgt 0x203 w0 0x2e8 <- diverge
altble 0x300 w4 0x2b4
altbn w0 0x0
altble 0x300 w1 0x228
altbn w0 0x0
null
altrle 0x300 w8 0x2c8
altbgt 0x300 w0 0x2e8 <- stitch
altbn w0 0x0
altbn w0 0x0
altbn w0 0x0
altbgt 0x201 w0 0x164
reg w1
And here is a simplified view, after compaction, before we do a subwide
append/replace:
.-> reg w1 a
.-------b-> data
| .-> reg w1 b
| .---b-> data
r-b-r-b-b-> orphan w1 <- removed as a part of our subwide op
^
diverging
'-+-'
weight=3
altrle data w1
altble orphan w1
First, as a part of our subwide append, we're going to write out the
lower trunk. We diverge on the first altble since the entire orphan is
inside our subwide range.
It may seem a bit strange to diverge on a null tag, but this isn't
actually an issue, we're allowed a single null tag to terminate our
tree:
.-> reg w1 a
.-------b-> data
| .-> reg w1 b
r-b-----b-> data
^
diverging
'-+-'
weight=3
altrle data w1
altbgt data w0
Nothing wrong so far. The weight of our leaves (2) don't match our
tree's weight (3), but this is normal for the lower trunk. We fix this
when we stitch the diverging alt on the upper trunk.
Speaking of the upper trunk, let's start writing it out, but pause at
the stitching alt:
.-> reg w1 a
.-------b-> data
| .-> reg w1 b
| .-----b-> data
r-b-?
^
stitching
'-+-'
weight=3
altrle data w1
altble data w1
Note we've flipped the altbgt data into an altble data, since we're
going down the other diverged path now.
But before we continue, as a part of stitching, we need to adjust our
tree weight to account for the weight of the orphan we deleted as a part
of our range operation:
.-> reg w1 a
.-------b-> data
| .-> reg w1 b
| .-----b-> data
r-b-?
^
stitching
'-+-'
weight=2
altrle data w1
altble data w1
Uh oh. Weight is 2 and both our alts add up to 2? All of a sudden it
looks like we should follow the stitched alt.
Our follow/flip logic kicks in, and disaster!
.-> reg w1 a
.-------b-> data
r-b-----b-> reg w1 c <- added as a part of our subwide op
'-> data <- somehow data survives
'-+-' but where did b go?
weight=2
altrle data w1
altbgt data w0
We go down the wrong path, and because our state machine thinks we've
diverged, we prune all le alts, destroying our tree.
---
So what's is going wrong?
The problem is that when we update our window, the stitched diverging
alt can become ambiguous.
Which sort of makes sense. The reason we update our window is so we can
continue down the tree veiwing it as it was _before_ the range
operation. But the stitched alt belongs to the tree _after_ the range
operation.
The solution here is to just make sure we never follow the stitched alt.
This is a bit annoying, as it makes the stitched alt a rather special
case, but as far as I can tell it's necessary to avoid ambiguity.
|
||
|
Christopher Haster
|
c4681fff0e |
rbyd-rr: Preserving diverging alt coloring with careful pruning rules
This seems to mostly be working, now passing rbyd tests at least. This pruning/triming logic desperately needs to be simplified/cleaned up, but preserving diverging alt color balance without breaking things is still proving to be difficult... |
||
|
Christopher Haster
|
c73749039e |
rbyd-rr: Trying another approach, 2-trunk diverging
This is a good checkpoint and is mostly working, though we're back to
recoloring the diverging alt black again. So no balance improvements.
But this already feels much better complexity-wise.
The fact that things could get back to a working state so quickly is a
good sign, or maybe just a sign I've been steeped in this algorithm for
too long...
---
The idea here is instead of a relatively complex 4-step state machine:
diverged? diverged
skip common -+-> write lower -> write common -> write upper -> done
'-> write common -> done
We just write two trunks: one for the lower bound, one for the upper
bound.
We _do_ need to keep track of where we diverge so we can prune
correctly, so this is _technically_ still 4-steps, but it is at least
conceptually, and in code, much simpler:
diverged? diverged
write common -+-> write lower -> write common -> write upper -> done
'-> done
Note that if we discover no tags in our range, we can terminate after
writing the lower/common trunk, which is nice. Previously we needed a
second pass.
The obvious downside is that we write the common trunk twice now. Which
is a bit of a downside, those alts will never really be used, but as a
tradeoff it really isn't that much of a waste. It's already possible for
range operations to need to write the full trunk twice, even for small
ranges:
.-------o-------.
.---o---. .---o---.
.-o-. .-o-. .-o-. .-o-.
.o. .o. .o. .o. .o. .o. .o. .o.
a b c d e f g h i j k l m n o p
'-+-'
remove
The original motivation for trying yet-another-range-removal-algorithm
comes from attempting to solve issues with the color-balance of the
diverging alt.
The core conundrum being the case of two pending yellow splits. In our
previous algorithm, we only have one common trunk, so trying to
propagate two red edges violates tail recursion:
.-> .-> .-> h=4 -.
.-----b-> .-b-> .-b-> |
| .-> | .-> | .-> |
| .---b-> .-y-b-> .-y-b-> |
| | .-> | .-> | .-> |
| | .-b-> | .-b-> | .-b-> |
| | | .-> b-b-b-> .-b-b-b-> |
.-y-r-b-b-> rm me => => | +- unbalanced :(
| .-> r-b-b-b-> |
| .-----b-> | | '-> |
| | .-> | | .-> |
| | .---b-> | '-b-> |
| | | .-> | .-> |
| | | .-b-> | .-b-> |
| | | | .-> | | .-> |
b-y-r-b-b-> '-b-b-> h=3 -'
^ ^
diverging lost color propagation
But if we have two trunks? Even only temporarily? This allows both
yellow splits/red edge propagation to settle tail recursively:
.-> .-> .-> h=3 -.
.-----b-> .-b-> .-b-> |
| .-> | .-> | .-> |
| .---b-> .-y-b-> .-----y-b-> |
| | .-> | .-> | .-> |
| | .-b-> | .-b-> | .-b-> |
| | | .-> r---b-b-b-> | .---b-b-> |
.-y-r-b-b-> rm me => | => | | +- balanced :)
| .-> | .-> y-r-b-b-b-> |
| .-----b-> | .-----b-> | | '-> |
| | .-> | | .-> | | .-> |
| | .---b-> | | .---b-> | '-b-> |
| | | .-> | | | .-> | .-> |
| | | .-b-> | | | .-b-> | .-b-> |
| | | | .-> | | | | .-> | | .-> |
b-y-r-b-b-> '-y-r-b-b-> '-b-b-> h=3 -'
^
diverging
It's interesting to note while we _are_ currently recoloring the
diverging alt black (intentionally simplifying to algorithm to get
things moving), we are already allowing yellow splits/red edge
propagation by just writing out both trunks normally.
And that's what makes this yet-another-range-removal-algorithm appealing
and worth yet another iteration, the diverging trunks are no longer such
special cases. Not only will this make a better diverging color-balance
possible, it will hopefully make the whole diverging algorithm simpler,
easier, and cheaper. And it is already showing good signs so far.
|
||
|
Christopher Haster
|
233fc2c212 |
rbyd-rr: Attempting correct balance of the diverging node itself
So far, our color-balance preserving range removal algorithm is working
great:
- Common trunk? color-balance preserving ✓
- Lower-diverged trunk? color-balance preserving ✓
- Upper-diverged trunk? color-balance preserving ✓
The only hole in our algorithm is the color-balance of the diverging
node itself.
Up until now we've simply recolored the diverging alt black, as this
avoids a large number of complicated corner cases. Unfortunately this
has the consequence of potentially offsetting the balance of our tree
by +-1:
.-> b-> .---b-> h=2 -.
.---b-> rm me | |
| .-> => => b-b-b-> +- unbalanced :(
| .-b-> | '-> |
| | .-> | .-> |
r-b-b-> '-b-> h=3 -'
^
diverging
This attempts to preserve the coloring of the diverging alt, and
preserve the color-balance, but we quickly run into the, uh, previously
mentioned complicated corner cases...
- First to note, we _can_ preserve red coloring on the gt path:
.-> b-> .---b-> h=2 -.
.---b-> rm me | |
| .-> => => r-b-b-> +- balanced :)
| .-b-> | '-> |
| | .-> | .-> |
r-b-b-> '-b-> h=2 -'
^
diverging
But only if it isn't a part of a pending yellow split. If it _is_ a
pending yellow split, the yellow split may try to reference the
yellow node in the history, but this won't work because our history
has been modified:
.-> .-> h=2 -.
.-----b-> .-----b-> |
| .-> b-> | .---b-> |
| .---b-> rm me => => | | +- unbalanced :(
| | .-> r-b-b-b-> |
| | .-b-> | '-> |
| | | .-> '-b-> |
y-r-b-b-> '-> h=3 -'
^ '-+-'
diverging wants to have split
- As for the le path, we can't even preserve the red coloring! For this
to work we would need to somehow color a flipped alt red (so the
"follow" edge is red, not the "not-follow"), but this isn't possible
with our encoding scheme (and definitely not worth reserving a whole
additional bit in every alt for):
r-b-b-> .-b-> .-b-> h=3 -.
| | '-> | '-> | '-> |
| '-b-> => | .-> => | .-> +- unbalanced :(
| '-> b-b-> .-b-b-> |
'---b-> rm me | |
'-> b---b-> h=2 -'
^ ^
diverging this wants to be red
The reason we can preserve reds on the gt path but not the le path is
because we write the le path first and stitch on the gt path. If
instead you wrote the gt path first, this would be flipped:
r-b-b-> .-b-> h=2 -.
| | '-> | '-> |
| '-b-> => => | .-> +- balanced :)
| '-> r-b-b-> |
'---b-> rm me | |
'-> b-> '---b-> h=2 -'
^
diverging
In theory, you could do _another_ pass over the tree to figure out
which order is needed to preserve coloring. But this would be an even
more complicated mess...
Not to mention this wouldn't even completely solve the color-balance
of the diverging alt because of yellow split issues...
And we haven't even touched issues related to yellow split color
propagation! Fortunately this JustWorksTM on the gt path, since it
mostly looks like a normal trunk after stitching. But we completely
ignore yellow split color propagation on the le path since this runs
into many of the same issues as red flipping.
But if you manage to make it though all of this mess while preserving
color-balance (code size be damned), we arive on what seems to be an
impossible case: How do you preserve color balance of a diverging alt
when both paths contain a pending yellow split?
.-> .-> .-> h=4 -.
.-----b-> .-b-> .-b-> |
| .-> | .-> | .-> |
| .---b-> .-y-b-> .-y-b-> |
| | .-> | .-> | .-> |
| | .-b-> | .-b-> | .-b-> |
| | | .-> b-b-b-> .-b-b-b-> |
.-y-r-b-b-> rm me => => | +- unbalanced :(
| .-> r-b-b-b-> |
| .-----b-> | | '-> |
| | .-> | | .-> |
| | .---b-> | '-b-> |
| | | .-> | .-> |
| | | .-b-> | .-b-> |
| | | | .-> | | .-> |
b-y-r-b-b-> '-b-b-> h=3 -'
^ ^
diverging lost color propagation
This seems to violate tail recursion!
Anyways, this turned into a bit of a rant and a bit of a mess.
If anyone reads this and is interested in exploring the balancing issues
further, the diverging alt logic currently contains some commented-out
coloring conditions:
(true) / (false) / (lfsr_tag_isred(p_alts[0]))
These are currently commented-out to what is currently known to be
optimal (see above), but can be tweaked to try to preserve different
colorings.
|
||
|
Christopher Haster
|
4f14f3cef4 |
rbyd-rr: Fixed issue where red alts were just not being pruned
Not sure how I missed this earlier, but we aren't pruning unreachable/
unavoidable red alts.
There are two cases where we can use red alts to prune. Both cases
effectively collapse a 3-node into a 2-node, while converting isolated
black alts into altns effectively collase a 2-node into a 1-node:
.---> a rm me
| .-> b red prune .-> b <-- we weren't handling
-r-b-> c => ---b-> c this case correctly
.---> a .---> a
| .-> b rm me red prune |
-r-b-> c => -b---> c
.-> a rm me v------ altn
.-b-> b black flatten .-b-> b
| .-> c => | .-> c
-b-b-> d -b-b-> d
Humorously, we were handling the arguably more difficult case of pruning
a black alt following a red alt correctly. But we weren't handling the
case when a red alt itself needs to be pruned.
Fortunately this code is identical to pruning root alts (also arguably a
more tricky case!), so we can just extend the relevant if statement to
cover the case of an unreachable/unavoidable red alt.
And small code change means small code change:
code stack
before: 34288 2864
after: 34304 (+0.0%) 2864 (+0.0%)
|
||
|
Christopher Haster
|
1ce47bfc47 |
rbyd-rr: Implemented coloring during rbyd compaction
This tweaks our rbyd compaction algorithm to color the alts correctly to
represent a balanced 2-3-4 tree.
Previously, we didn't really care about coloring the compacted tree,
because we didn't really care about color when pruning unreachable
alts.
But now that we refuse to prune isolated black alts, or risk unbalancing
the underlying 2-3-4 tree, it's important we color the compacted tree
correctly. Otherwise the unreachable alts that terminate our binary nodes
will just never be pruned, unbalancing each layer of the tree by ~1.
Compaction without coloring:
tags: effective rby tree:
data a <. .---> a
data b <--. .---b-b-> b
data c <----. | .---> c
data d <------. b-b-b-b-> d
altble a <. | | |
altble b -|-' | |
null | | | effective 2-3-4 tree:
altble c <--.-' | .---o -.
altble d -|-|---' | o |
null | | .-o .-o +- h=4
altble b -' | | o | o |
altble d ---' a b c d -'
null
Compaction with coloring:
tags: effective rby tree:
data a <. .---> a
data b <--. .---r-b-> b
data c <----. | .---> c
data d <------. r-b-r-b-> d
altrle a <. | | |
altble b -|-' | |
null | | | effective 2-3-4 tree:
altrle c <--.-' | .---o -.
altble d -|-|---' .-o .-o +- h=2
null | | a b c d -'
altrle b -' |
altble d ---'
null
Note that if the compacted tree is not full, i.e. not a power-of-two, we
need to make sure the resulting unary nodes are still colored black.
Isolated red alts are not allowed and would create even more hilarious
problems.
Fortunately there is just enough context in lfsr_rbyd_appendcompaction,
since we know exactly where each layer ends, to determine if each node
is binary or unary without needing to attempt to read unnecessary tags.
It's also worth noting the resulting unary nodes may seem like an
unnecessary side effect, but they are actually quite useful here for
preserving the underlying 2-3-4 balance! In the same way unary nodes
preserve the 2-3-4 balance during range operations, unary nodes in the
compacted tree can be consumed later to introduce new attrs without
unbalancing the tree.
Now I'm wondering, how would a rebalancing algorithm even work on a
red-black tree without unary nodes...? Did I dodge a bullet here?
Unaligned compaction with coloring:
tags: effective rby tree:
data a <. .---> a
data b <--. .---r-b-> b
data c <----. | .---> c
data d <------. .---r-b-r-b-> d
data e <--------. r-b---b---b-> e
altrle a <. | | | |
altble b -|-' | | |
null | | | | effective 2-3-4 tree:
altrle c <--.-' | | .-o -.
altble d -|-|---' | .---o o +- h=3
null | | | .-o .-o o |
altble e <----.---' a b c d e -'
null | | |
altrle b <. | |
altble d -|-' |
null | |
altble e <--.-'
null | |
altrle d -' |
altble e ---'
null
Code changes minimal, just needed some twiddly logic in
lfsr_rbyd_appendcompaction to make this work:
code stack
before: 34256 2864
after: 34288 (+0.1%) 2864 (+0.0%)
|
||
|
Christopher Haster
|
c08b7ccdd8 |
rbyd-rr: Fixed yellow-alt pruning being completely broken
At some point during all this refactoring, `branch_ = branch` snuck its
way into the common red-black pruning code:
// collapse unreachable red alts
if (lfsr_tag_isred(p_alts[0])) {
alt = p_alts[0] & ~LFSR_TAG_R;
weight = p_weights[0];
jump = p_jumps[0];
branch_ = branch; // <-- ???
lfsr_rbyd_p_pop(p_alts, p_weights, p_jumps);
What this ends up doing is forcing the appendattr logic to branch to
where it just was.
Ignoring concerns about forward-progress, this somewhat humorously
undoes the pruning of the alt. It's technically not an error, since the
alt was prunable, but certainly counter-productive.
First noticed because our post-split yellow alts were not getting
cleaned up correctly, even though all the correct conditions were being
hit.
---
Unfortunately, attempting to simply remove that line breaks things.
It turns out revisiting the pruned alt was hiding the fact that using an
lfsr_tag_follow2(a_rid, a_tag) check to determine if we take the pruned
alt is insufficient.
At first glance this appears to be sufficient, after all if an alt is
always taken, shouldn't lfsr_tag_follow2(a_rid, a_tag) always return
true?
The problem is when we look up a_rid/a_tag outside the tree.
lfsr_tag_follow2(a_rid, a_tag) may return false, but _in the context of
our current lower/upper bound_, the alt may always be taken and
lfsr_tag_prune2() may return true. This mismatch in lfsr_tag_prune2 and
lfsr_tag_follow2 breaks the underlying logic and causes the wrong branch
to be taken.
The fix here is to use the same reachability logic for both the pruning
check and follow check. So a_rid/a_tag should not be involved in the
pruning logic at all, which makes a bit of sense since a_rid/a_tag do
not determine if an alt is reachable.
I've also gone ahead and replaced lfsr_tag_prune{,2} with
lfsr_tag_unreachable{,2} (never taken) and lfsr_tag_unavoidable{,2}
(always taken) which I think capture/document the underlying conditions
we need a bit better.
Code changes:
code stack
before: 34220 2864
after: 34256 (+0.1%) 2864 (+0.0%)
It's good that even though we changed a number of functions, the code
changes match our expectation that the underlying logic didn't really
change all that much.
|
||
|
Christopher Haster
|
dcc67d22a8 |
rbyd-rr: Tweaked lfsr_tag_follow to make altn/alta implicit again
In theory, checking altn/alta tags for followability should be implicit.
These are encoding as altle/altgt tag 0, which should never be requested
in normal operation:
altn => altle 0
alta => altgt 0
But while that's good in theory, null tags, tag 0, has a tendency to
creep into these functions and has already caused a number of headaches.
Conditionally checking for altn/alta is safer, but asserting on tag 0 is
just as safe and adds no code cost.
Both lfsr_rbyd_appendattr and lfsr_rbyd_lookupnext have
`tag = lfs_max16(tag, 0x1)` guards now to comply with this rule. But
it's still a nice safety net to assert on tag 0 in lfsr_tag_follow*.
In case you were curious if the max16 guards were more expensive than
the explicit altn/alta checks, code size says no:
code stack
before: 34256 2864
after: 34220 (-0.1%) 2864 (+0.0%)
|
||
|
Christopher Haster
|
0475af0415 |
Renamed lower/upper -> lower_rid/upper_rid for consistency/clarity
The lower/upper names were introduced fairly early. I think before appendattr bounds included lower_tag/upper_tag? Since then the explicit lower_rid/upper_rid names have become more common. Changing for consistency, and because, you know, it's probably a bit better to indicate what these variables actually are the lower/upper bounds of... |