From 0b9f46e7cb331112e245b2f74c0b0d355bca39d2 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sun, 5 Jan 2025 14:58:49 -0600 Subject: [PATCH] Attempted to re-add inlined lfsr_data_t representation The idea, which has floated up a few times, is to add a third representation of lfsr_data_t where the data is inlined in the struct directly. In theory saving RAM for small pieces of data such as dids, leb128s, flags, etc: inlined: in-RAM buffer: on-disk: .---+---+---+---. .---+---+---+---. .---+---+---+---. |01| size | |00| size | |1| size | +---+---+---+---+ +---+---+---+---+ +---+---+---+---+ | inlined data | | ptr -------. | block | + + +---+---+---+---+ | +---+---+---+---+ | | | (unused) | | | off | '---+---+---+---' '---+---+---+---' | '---+---+---+---' .---+---+---+---. | | data |<' : : : Unfortunately in practice this just doesn't work out. It turns out we benefit a lot from the _simplicity_ of lfsr_data_t. When lfsr_data_t is built out of simple words, the compiler can make some pretty strong assumptions and basically break it down into simple register operations. When you stick a byte array in the middle of the struct, this sort of breaks down. --- We can see this in our code measurements. After adding inlined data, but before implementing slicing (in lfsr_data_fromslice), we can see decent stack savings. But as soon as we add the memmove to lfsr_data_fromslice, any benefit is lost: code stack ctx before: 38060 2608 752 without slicing: 38056 (-0.0%) 2568 (-1.5%) 752 (+0.0%) after: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%) One reason for this is the extra logic does cause lfsr_data_fromslice to be no longer inlined, but adding __attribute__((always_inline)) only claws back some of the code/stack savings (though it's interesting to note the compiler heuristic failure here): code stack ctx before: 38060 2608 752 after+inline: 38128 (+0.2%) 2672 (+2.5%) 752 (+0.0%) after+always_inline: 38684 (+1.6%) 2656 (+1.8%) 752 (+0.0%) --- Oh, and inlined lfsr_data_t is no longer compatible with LFSR_RAT's simple data conversion, since lfsr_rat_t's can only point to existing buffers. This causes tests to fail rather quickly. This should be reverted, but I think the hidden cost of inlined lfsr_data_t is surprising and interesting to note. --- lfs.c | 158 ++++++++++++++++++++++++++++++++++++++-------------------- lfs.h | 7 ++- 2 files changed, 109 insertions(+), 56 deletions(-) diff --git a/lfs.c b/lfs.c index a17e458c..e7cbef3d 100644 --- a/lfs.c +++ b/lfs.c @@ -1650,6 +1650,7 @@ static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs, /// lfsr_data_t stuff /// #define LFSR_DATA_ONDISK 0x80000000 +#define LFSR_DATA_ISIMM 0x40000000 #define LFSR_DATA_NULL() \ ((lfsr_data_t){ \ @@ -1661,6 +1662,15 @@ static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs, .size=_size, \ .u.buffer=(const void*)(_buffer)}) +#define LFSR_DATA_IMM(_buffer, _size) \ + ((struct {lfsr_data_t d;}){lfsr_data_fromimm(_buffer, _size)}.d) + +#define LFSR_DATA_LEB128(_word) \ + ((struct {lfsr_data_t d;}){lfsr_data_fromleb128(_word)}.d) + +#define LFSR_DATA_LLEB128(_word) \ + ((struct {lfsr_data_t d;}){lfsr_data_fromlleb128(_word)}.d) + #define LFSR_DATA_DISK(_block, _off, _size) \ ((lfsr_data_t){ \ .size=LFSR_DATA_ONDISK | (_size), \ @@ -1683,20 +1693,69 @@ static lfs_ssize_t lfsr_bd_progtag(lfs_t *lfs, .u.disk.off=_off}) #endif +// these can't really be macros, so these get a bit hacky +static inline lfsr_data_t lfsr_data_fromimm( + const void *buffer, lfs_size_t size) { + // inlined-data limited to 8 bytes + LFS_ASSERT(size <= 8); + + lfsr_data_t data; + memcpy(data.u.imm, buffer, size); + data.size = LFSR_DATA_ISIMM | size; + return data; +} + +#define LFSR_LEB128_DSIZE 5 + +static inline lfsr_data_t lfsr_data_fromleb128(uint32_t word) { + // leb128s should not exceed 31-bits + LFS_ASSERT(word <= 0x7fffffff); + + lfsr_data_t data; + lfs_ssize_t d = lfs_toleb128(word, data.u.imm, LFSR_LEB128_DSIZE); + if (d < 0) { + LFS_UNREACHABLE(); + } + data.size = LFSR_DATA_ISIMM | d; + return data; +} + +#define LFSR_LLEB128_DSIZE 4 + +static inline lfsr_data_t lfsr_data_fromlleb128(uint32_t word) { + // little-leb128s should not exceed 28-bits + LFS_ASSERT(word <= 0x0fffffff); + + lfsr_data_t data; + lfs_ssize_t d = lfs_toleb128(word, data.u.imm, LFSR_LLEB128_DSIZE); + if (d < 0) { + LFS_UNREACHABLE(); + } + data.size = LFSR_DATA_ISIMM | d; + return data; +} + // data helpers static inline bool lfsr_data_ondisk(lfsr_data_t data) { return data.size & LFSR_DATA_ONDISK; } static inline bool lfsr_data_isbuf(lfsr_data_t data) { - return !(data.size & LFSR_DATA_ONDISK); + return (data.size & (LFSR_DATA_ONDISK | LFSR_DATA_ISIMM)) + == 0; +} + +static inline bool lfsr_data_isimm(lfsr_data_t data) { + return (data.size & (LFSR_DATA_ONDISK | LFSR_DATA_ISIMM)) + == LFSR_DATA_ISIMM; } static inline lfs_size_t lfsr_data_size(lfsr_data_t data) { - return data.size & ~LFSR_DATA_ONDISK; + return data.size & ~(LFSR_DATA_ONDISK | LFSR_DATA_ISIMM); } -static lfsr_data_t lfsr_data_fromslice(lfsr_data_t data, +// fancier data functions +static inline lfsr_data_t lfsr_data_fromslice(lfsr_data_t data, lfs_ssize_t off, lfs_ssize_t size) { // limit our off/size to data range, note the use of unsigned casts // here to treat -1 as unbounded @@ -1712,10 +1771,18 @@ static lfsr_data_t lfsr_data_fromslice(lfsr_data_t data, data.u.disk.off += off_; data.size = LFSR_DATA_ONDISK | size_; + // inlined? + } else if (lfsr_data_isimm(data)) { + memmove(data.u.imm, data.u.imm + off_, size_); + data.size = LFSR_DATA_ISIMM | size_; + // buffer? - } else { + } else if (lfsr_data_isbuf(data)) { data.u.buffer += off_; data.size = size_; + + } else { + LFS_UNREACHABLE(); } return data; @@ -1790,6 +1857,10 @@ static lfs_ssize_t lfsr_data_read(lfs_t *lfs, lfsr_data_t *data, } } + // inlined? + } else if (lfsr_data_isimm(*data)) { + lfs_memcpy(buffer, data->u.imm, d); + // buffer? } else { lfs_memcpy(buffer, data->u.buffer, d); @@ -1897,14 +1968,26 @@ static lfs_scmp_t lfsr_data_cmp(lfs_t *lfs, lfsr_data_t data, } } + // inlined? + } else if (lfsr_data_isimm(data)) { + int cmp = lfs_memcmp(data.u.imm, buffer, d); + if (cmp < 0) { + return LFS_CMP_LT; + } else if (cmp > 0) { + return LFS_CMP_GT; + } + // buffer? - } else { + } else if (lfsr_data_isbuf(data)) { int cmp = lfs_memcmp(data.u.buffer, buffer, d); if (cmp < 0) { return LFS_CMP_LT; } else if (cmp > 0) { return LFS_CMP_GT; } + + } else { + LFS_UNREACHABLE(); } // if data is equal, check for size mismatch @@ -1967,64 +2050,31 @@ static int lfsr_bd_progdata(lfs_t *lfs, } } - // buffer? - } else { + // inlined? + } else if (lfsr_data_isimm(data)) { int err = lfsr_bd_prog(lfs, block, off, - data.u.buffer, data.size, + data.u.imm, lfsr_data_size(data), cksum, align); if (err) { return err; } + + // buffer? + } else if (lfsr_data_isbuf(data)) { + int err = lfsr_bd_prog(lfs, block, off, + data.u.buffer, lfsr_data_size(data), + cksum, align); + if (err) { + return err; + } + + } else { + LFS_UNREACHABLE(); } return 0; } -// we can also treat leb128/lleb128 encoding has a high-level operation, -// which is useful for building rats - -#define LFSR_LEB128_DSIZE 5 - -#define LFSR_DATA_LEB128_(_word, _buffer) \ - ((struct {lfsr_data_t d;}){lfsr_data_fromleb128(_word, _buffer)}.d) - -#define LFSR_DATA_LEB128(_word) \ - LFSR_DATA_LEB128_(_word, (uint8_t[LFSR_LEB128_DSIZE]){0}) - -static inline lfsr_data_t lfsr_data_fromleb128(uint32_t word, - uint8_t buffer[static LFSR_LEB128_DSIZE]) { - // leb128s should not exceed 31-bits - LFS_ASSERT(word <= 0x7fffffff); - - lfs_ssize_t d = lfs_toleb128(word, buffer, LFSR_LEB128_DSIZE); - if (d < 0) { - LFS_UNREACHABLE(); - } - - return LFSR_DATA_BUF(buffer, d); -} - -#define LFSR_LLEB128_DSIZE 4 - -#define LFSR_DATA_LLEB128_(_word, _buffer) \ - ((struct {lfsr_data_t d;}){lfsr_data_fromlleb128(_word, _buffer)}.d) - -#define LFSR_DATA_LLEB128(_word) \ - LFSR_DATA_LLEB128_(_word, (uint8_t[LFSR_LLEB128_DSIZE]){0}) - -static inline lfsr_data_t lfsr_data_fromlleb128(uint32_t word, - uint8_t buffer[static LFSR_LLEB128_DSIZE]) { - // little-leb128s should not exceed 28-bits - LFS_ASSERT(word <= 0x0fffffff); - - lfs_ssize_t d = lfs_toleb128(word, buffer, LFSR_LLEB128_DSIZE); - if (d < 0) { - LFS_UNREACHABLE(); - } - - return LFSR_DATA_BUF(buffer, d); -} - // operations on attribute lists @@ -13431,7 +13481,7 @@ static inline bool lfsr_ocompat_isincompat(lfsr_ocompat_t ocompat) { // little-endian, truncated bits must be assumed zero #define LFSR_DATA_COMPAT(_compat) \ - LFSR_DATA_BUF(((uint8_t[]){ \ + LFSR_DATA_IMM(((uint8_t[]){ \ (((_compat) >> 0) & 0xff), \ (((_compat) >> 8) & 0xff)}), 2) diff --git a/lfs.h b/lfs.h index 92448f6f..57b6b196 100644 --- a/lfs.h +++ b/lfs.h @@ -614,11 +614,14 @@ typedef struct lfsr_omdir { // either an on-disk or in-device data pointer typedef struct lfsr_data { - // sign(size)=0 => in-RAM buffer - // sign(size)=1 => on-disk reference + // the top bits of size indicate the exact encoding: + // top2(size)=0b00 => in-RAM buffer + // top2(size)=0b01 => inlined data + // top2(size)=0b1x => on-disk reference lfs_size_t size; union { const uint8_t *buffer; + uint8_t imm[8]; struct { lfs_block_t block; lfs_size_t off;