From 2383e7f9f91d376c458dc21916379ea6c2fd1e50 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Wed, 8 May 2024 00:49:56 -0500 Subject: [PATCH] Reverted most of the crammed 15-bit lfsr_attr_t representation I think this was becoming too over-engineered. That and if you're fighting the compiler too much, sometimes it's best to just let the compiler win. There is also the minor benefit of not needing to worry about uint16_t overflows in lfsr_cat_t. One concern is it's easy for missed lfsr_data_t indirections to go unnoticed. This reverts lfsr_attr_t from 3 words -> 4 words, but allows easy mapping to the lfsr_cat_t part of the lfsr_attr_t: lfsr_tag_t lfsr_attr_t .---+---. .---+---+---+---. . . .---+---+---+---. | tag |--------------->| tag | | | tag | | '---+---' +---+---+---+---+ +---+---+---+---+ .----->| delta | | delta | lfsr_srid_t | +---+---+---+---+ +---+---+---+---+ .---+---+---+---. | .--->|c| size | | cat | | delta |-' | +---+---+---+---+ | | '---+---+---+---' | .->| ptr | | | | | '---+---+---+---' . . '---+---+---+---' lfsr_cat_t | | .---+---+---+---. | | |c| size |---' | +---+---+---+---+ | | ptr |-----' '---+---+---+---' This doesn't impact stack as much as you would expect due to compiler overhead around inline functions operating on multi-word structs: before 15-bit: 33672 2776 best 15-bit: 33900 (+0.7%) 2736 (-1.4%) prev 15-bit: 34486 (+2.4%) 2912 (+4.9%) after: 33672 (+0.0%) 2744 (-1.2%) I think the only big thing not reverted was switching lfsr_cat_t from storing the sum of the lfsr_data_ts sizes to just storing the number of lfsr_data_ts. This is cheaper to initialize (because inline functions are weirdly costly!), cheaper to iterate over, and we don't actually need lfsr_cat_size that often, unlike lfsr_data_size. Eventually we'll probably need to revisit this to optimize 31/15-bit, etc, littlefs configurations. But that's a problem for another day... --- lfs.c | 124 ++++++++++++++++++++++++---------------------------------- 1 file changed, 52 insertions(+), 72 deletions(-) diff --git a/lfs.c b/lfs.c index a4170865..0eed8d46 100644 --- a/lfs.c +++ b/lfs.c @@ -1416,20 +1416,19 @@ typedef struct lfsr_cat { // sign(size)=0 => single in-RAM buffer // sign(size)=1 => multiple concatenated datas union { + lfs_size_t size; struct { - lfsr_tag_t tag; - uint16_t size; + lfs_size_t size; const uint8_t *buffer; } buf; struct { - lfsr_tag_t tag; - uint16_t size; + lfs_size_t size; const lfsr_data_t *datas; } cat; } u; } lfsr_cat_t; -#define LFSR_CAT_ISCAT 0x8000 +#define LFSR_CAT_ISCAT 0x80000000 #define LFSR_CAT_NULL() \ ((lfsr_cat_t){ \ @@ -1453,11 +1452,11 @@ typedef struct lfsr_cat { // cat helpers static inline bool lfsr_cat_isbuf(lfsr_cat_t cat) { - return !(cat.u.buf.size & LFSR_CAT_ISCAT); + return !(cat.u.size & LFSR_CAT_ISCAT); } static inline bool lfsr_cat_iscat(lfsr_cat_t cat) { - return cat.u.buf.size & LFSR_CAT_ISCAT; + return cat.u.size & LFSR_CAT_ISCAT; } static inline lfsr_data_t lfsr_cat_data(lfsr_cat_t cat) { @@ -1466,19 +1465,19 @@ static inline lfsr_data_t lfsr_cat_data(lfsr_cat_t cat) { return LFSR_DATA_BUF(cat.u.buf.buffer, cat.u.buf.size); } -static inline uint16_t lfsr_cat_count(lfsr_cat_t cat) { +static inline lfs_size_t lfsr_cat_count(lfsr_cat_t cat) { LFS_ASSERT(lfsr_cat_iscat(cat)); - return cat.u.buf.size & ~LFSR_CAT_ISCAT; + return cat.u.size & ~LFSR_CAT_ISCAT; } static inline lfs_size_t lfsr_cat_size(lfsr_cat_t cat) { // this gets a bit complicated for concatenated data if (lfsr_cat_isbuf(cat)) { - return cat.u.buf.size; + return cat.u.size; } else { - uint16_t count = lfsr_cat_count(cat); + lfs_size_t count = lfsr_cat_count(cat); lfs_size_t size = 0; - for (uint16_t i = 0; i < count; i++) { + for (lfs_size_t i = 0; i < count; i++) { size += lfsr_data_size(cat.u.cat.datas[i]); } return size; @@ -1546,8 +1545,8 @@ static int lfsr_bd_progcat(lfs_t *lfs, // indirect concatenated data? } else { - uint16_t count = lfsr_cat_count(cat); - for (uint16_t i = 0; i < count; i++) { + lfs_size_t count = lfsr_cat_count(cat); + for (lfs_size_t i = 0; i < count; i++) { int err = lfsr_bd_progdata(lfs, block, off, cat.u.cat.datas[i], cksum_); if (err) { @@ -1563,33 +1562,17 @@ static int lfsr_bd_progcat(lfs_t *lfs, // operations on attribute lists -// needed for lfsr_attr -typedef struct lfsr_shrubcommit lfsr_shrubcommit_t; - typedef struct lfsr_attr { + lfsr_tag_t tag; lfsr_srid_t delta; - union { - lfsr_tag_t tag; - lfsr_cat_t cat; - } u; + lfsr_cat_t cat; } lfsr_attr_t; #define LFSR_ATTR(_tag, _delta, _cat) \ - ((lfsr_attr_t){ \ - .delta=_delta, \ - .u.cat=lfsr_cat_tag(_cat, _tag)}) - -static inline lfsr_cat_t lfsr_cat_tag(lfsr_cat_t cat, lfsr_tag_t tag) { - cat.u.buf.tag = tag; - return cat; -} + ((lfsr_attr_t){_tag, _delta, _cat}) #define LFSR_ATTR_NOOP() \ - ((const lfsr_attr_t){ \ - .delta=_delta, \ - .u.cat.u.buf.tag=LFSR_TAG_NULL, \ - .u.cat.u.buf.size=0, \ - .u.cat.u.buf.buffer=NULL}) + LFSR_ATTR(LFSR_TAG_NULL, 0, LFSR_CAT_NULL()) // create an attribute list #define LFSR_ATTRS(...) \ @@ -1601,25 +1584,26 @@ static inline lfsr_cat_t lfsr_cat_tag(lfsr_cat_t cat, lfsr_tag_t tag) { // a move of all attrs from an mdir entry #define LFSR_ATTR_MOVE(_tag, _delta, _mdir) \ ((const lfsr_attr_t){ \ + .tag=_tag, \ .delta=_delta, \ - .u.cat.u.buf.tag=_tag, \ - .u.cat.u.buf.buffer=(const void*)(const lfsr_mdir_t*){_mdir}}) + .cat.u.buf.buffer=(const void*)(const lfsr_mdir_t*){_mdir}}) // a grm update, note this is mutable! we may update the grm during // mdir commits #define LFSR_ATTR_GRM(_tag, _delta, _grm) \ ((const lfsr_attr_t){ \ + .tag=_tag, \ .delta=_delta, \ - .u.cat.u.buf.tag=_tag, \ - .u.cat.u.buf.buffer=(const void*)(const lfsr_grm_t*){_grm}}) + .cat.u.buf.buffer=(const void*)(const lfsr_grm_t*){_grm}}) // writing to an unrelated trunk in the rbyd +typedef struct lfsr_shrubcommit lfsr_shrubcommit_t; #define LFSR_ATTR_SHRUBCOMMIT(_tag, _delta, \ _shrub, _rid, _attrs, _attr_count) \ ((const lfsr_attr_t){ \ + .tag=_tag, \ .delta=_delta, \ - .u.cat.u.buf.tag=_tag, \ - .u.cat.u.buf.buffer=(const void*)&(const lfsr_shrubcommit_t){ \ + .cat.u.buf.buffer=(const void*)&(const lfsr_shrubcommit_t){ \ .shrub=_shrub, \ .rid=_rid, \ .attrs=_attrs, \ @@ -1627,35 +1611,35 @@ static inline lfsr_cat_t lfsr_cat_tag(lfsr_cat_t cat, lfsr_tag_t tag) { #define LFSR_ATTR_SHRUBTRUNK(_tag, _delta, _shrub) \ ((const lfsr_attr_t){ \ + .tag=_tag, \ .delta=_delta, \ - .u.cat.u.buf.tag=_tag, \ - .u.cat.u.buf.buffer=(const void*)(const lfsr_shrub_t*){_shrub}}) + .cat.u.buf.buffer=(const void*)(const lfsr_shrub_t*){_shrub}}) // some helpers static inline bool lfsr_attr_isnoop(const lfsr_attr_t *attr) { - return !attr->u.tag && attr->delta == 0; + return !attr->tag && attr->delta == 0; } static inline bool lfsr_attr_isinsert(const lfsr_attr_t *attr) { - return !lfsr_tag_isgrow(attr->u.tag) && attr->delta > 0; + return !lfsr_tag_isgrow(attr->tag) && attr->delta > 0; } static inline lfsr_grm_t *lfsr_attr_grm(const lfsr_attr_t *attr) { - return (lfsr_grm_t*)attr->u.cat.u.buf.buffer; + return (lfsr_grm_t*)attr->cat.u.buf.buffer; } static inline const lfsr_mdir_t *lfsr_attr_mdir(const lfsr_attr_t *attr) { - return (lfsr_mdir_t*)attr->u.cat.u.buf.buffer; + return (lfsr_mdir_t*)attr->cat.u.buf.buffer; } static inline const lfsr_shrubcommit_t *lfsr_attr_shrubcommit( const lfsr_attr_t *attr) { - return (const lfsr_shrubcommit_t*)attr->u.cat.u.buf.buffer; + return (const lfsr_shrubcommit_t*)attr->cat.u.buf.buffer; } static inline const lfsr_shrub_t *lfsr_attr_shrubtrunk( const lfsr_attr_t *attr) { - return (const lfsr_shrub_t*)attr->u.cat.u.buf.buffer; + return (const lfsr_shrub_t*)attr->cat.u.buf.buffer; } @@ -3601,7 +3585,7 @@ static int lfsr_rbyd_appendattrs(lfs_t *lfs, lfsr_rbyd_t *rbyd, && (lfs_size_t)(rid + 1) <= (lfs_size_t)end_rid) { int err = lfsr_rbyd_appendattr(lfs, rbyd, rid - lfs_smax32(start_rid, 0), - attrs[i].u.tag, attrs[i].delta, attrs[i].u.cat); + attrs[i].tag, attrs[i].delta, attrs[i].cat); if (err) { return err; } @@ -5984,13 +5968,13 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir, // ignore any gstate tags here, these need to be handled // specially by upper-layers - if (attrs[i].u.tag == LFSR_TAG_GRM) { + if (attrs[i].tag == LFSR_TAG_GRM) { // do nothing // move tags copy over any tags associated with the source's rid // TODO can this be deduplicated with lfsr_mdir_compact__ more? // it _really_ wants to be deduplicated - } else if (attrs[i].u.tag == LFSR_TAG_MOVE) { + } else if (attrs[i].tag == LFSR_TAG_MOVE) { // weighted moves are not supported LFS_ASSERT(attrs[i].delta == 0); const lfsr_mdir_t *mdir__ = lfsr_attr_mdir(&attrs[i]); @@ -6112,15 +6096,15 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir, // shrub tags append a set of attributes to an unrelated trunk // in our rbyd - } else if (attrs[i].u.tag == LFSR_TAG_SHRUBALLOC - || attrs[i].u.tag == LFSR_TAG_SHRUBCOMMIT) { + } else if (attrs[i].tag == LFSR_TAG_SHRUBALLOC + || attrs[i].tag == LFSR_TAG_SHRUBCOMMIT) { const lfsr_shrubcommit_t *bshrubcommit = lfsr_attr_shrubcommit(&attrs[i]); // SHRUBALLOC is roughly the same as SHRUBCOMMIT but also // resets the shrub, we need to do this here so bshrub root // extensions are atomic - if (attrs[i].u.tag == LFSR_TAG_SHRUBALLOC) { + if (attrs[i].tag == LFSR_TAG_SHRUBALLOC) { bshrubcommit->shrub->blocks[0] = rbyd_.blocks[0]; bshrubcommit->shrub->trunk = LFSR_RBYD_ISSHRUB | 0; bshrubcommit->shrub->weight = 0; @@ -6140,12 +6124,12 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir, // // TODO should we preserve mode for all of these? // TODO should we do the same for sprouts? - } else if (lfsr_tag_key(attrs[i].u.tag) == LFSR_TAG_SHRUBTRUNK) { + } else if (lfsr_tag_key(attrs[i].tag) == LFSR_TAG_SHRUBTRUNK) { const lfsr_shrub_t *shrub = lfsr_attr_shrubtrunk(&attrs[i]); int err = lfsr_rbyd_appendattr(lfs, &rbyd_, rid - lfs_smax32(start_rid, 0), - lfsr_tag_mode(attrs[i].u.tag) | LFSR_TAG_BSHRUB, + lfsr_tag_mode(attrs[i].tag) | LFSR_TAG_BSHRUB, attrs[i].delta, // note we use the staged trunk here LFSR_CAT_SHRUB(shrub)); @@ -6155,11 +6139,11 @@ static int lfsr_mdir_commit__(lfs_t *lfs, lfsr_mdir_t *mdir, // write out normal tags normally } else { - LFS_ASSERT(!lfsr_tag_isinternal(attrs[i].u.tag)); + LFS_ASSERT(!lfsr_tag_isinternal(attrs[i].tag)); int err = lfsr_rbyd_appendattr(lfs, &rbyd_, rid - lfs_smax32(start_rid, 0), - attrs[i].u.tag, attrs[i].delta, attrs[i].u.cat); + attrs[i].tag, attrs[i].delta, attrs[i].cat); if (err) { return err; } @@ -6613,7 +6597,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, // parse out any pending gstate, these will get automatically // xored with on-disk gdeltas in lower-level functions for (lfs_size_t i = 0; i < attr_count; i++) { - if (attrs[i].u.tag == LFSR_TAG_GRM) { + if (attrs[i].tag == LFSR_TAG_GRM) { // encode to disk lfsr_grm_t *grm = lfsr_attr_grm(&attrs[i]); lfsr_gdelta_xorgrm(lfs, lfs->grm_d, LFSR_GRM_DSIZE, grm); @@ -6890,7 +6874,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, // patch any pending grms for (lfs_size_t i = 0; i < attr_count; i++) { - if (attrs[i].u.tag == LFSR_TAG_GRM) { + if (attrs[i].tag == LFSR_TAG_GRM) { // Assuming we already xored our gdelta with the grm, we first // need to xor the grm out of the gdelta. We can't just zero // the gdelta because we may have picked up extra gdelta from @@ -7032,7 +7016,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, lfsr_smid_t mid = mdir->mid; for (lfs_size_t i = 0; i < attr_count; i++) { // update any gstate changes - if (attrs[i].u.tag == LFSR_TAG_GRM) { + if (attrs[i].tag == LFSR_TAG_GRM) { lfs->grm = *lfsr_attr_grm(&attrs[i]); // keep track of the exact encoding on-disk @@ -7046,7 +7030,7 @@ static int lfsr_mdir_commit(lfs_t *lfs, lfsr_mdir_t *mdir, && o->mdir.mid >= mid) { // replaced? if (o->mdir.mid == mid - attrs[i].delta - && lfsr_tag_issup(attrs[i].u.tag)) { + && lfsr_tag_issup(attrs[i].tag)) { o->flags |= LFS_F_ZOMBIE | LFS_F_UNSYNC | LFS_O_DESYNC; @@ -10203,11 +10187,11 @@ static int lfsr_bshrub_commit(lfs_t *lfs, lfsr_file_t *file, lfs_size_t commit_estimate = 0; for (lfs_size_t i = 0; i < attr_count; i++) { // only include tag overhead if tag is not a grow/rm tag - if (!lfsr_tag_isgrow(attrs[i].u.tag) - && !lfsr_tag_isrm(attrs[i].u.tag)) { + if (!lfsr_tag_isgrow(attrs[i].tag) + && !lfsr_tag_isrm(attrs[i].tag)) { commit_estimate += lfs->attr_estimate; } - commit_estimate += lfsr_cat_size(attrs[i].u.cat); + commit_estimate += lfsr_cat_size(attrs[i].cat); } // does our estimate exceed our shrub_size? need to recalculate an @@ -11405,10 +11389,7 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { lfsr_attr_t attrs[2]; lfs_size_t attr_count = 0; lfsr_data_t name_data; - union { - lfsr_data_t data; - uint8_t buf[LFSR_BTREE_DSIZE]; - } data; + uint8_t buf[LFSR_BTREE_DSIZE]; // not created yet? need to convert orphan to normal file if (lfsr_f_isorphan(file->m.flags)) { @@ -11434,10 +11415,9 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { LFSR_CAT_NULL()); // small file inlined in mdir? } else if (lfsr_f_isunflush(file->m.flags)) { - data.data = LFSR_DATA_BUF(file->buffer, file->buffer_size); attrs[attr_count++] = LFSR_ATTR( LFSR_TAG_SUB | LFSR_TAG_DATA, 0, - LFSR_CAT_DATA(&data.data)); + LFSR_CAT_BUF(file->buffer, file->buffer_size)); // bshrub? } else if (lfsr_bshrub_isbshrub(&file->m.mdir, &file->bshrub)) { attrs[attr_count++] = LFSR_ATTR_SHRUBTRUNK( @@ -11447,7 +11427,7 @@ int lfsr_file_sync(lfs_t *lfs, lfsr_file_t *file) { } else if (lfsr_bshrub_isbtree(&file->m.mdir, &file->bshrub)) { attrs[attr_count++] = LFSR_ATTR( LFSR_TAG_SUB | LFSR_TAG_BTREE, 0, - lfsr_cat_frombtree(&file->bshrub.u.btree, data.buf)); + lfsr_cat_frombtree(&file->bshrub.u.btree, buf)); } else { LFS_UNREACHABLE(); }