From 96b62ff8048f9ae4791238152d901603b0fad440 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 20 Jan 2024 15:01:46 -0600 Subject: [PATCH] Dropped conditional/noop attrs, prefer incremental attr allocation So instead of using C's ternary operator everywhere: (condition) ? LFSR_ATTR(rid, tag, delta, data) : LFSR_ATTR_NOOP Use incremental attr allocation instead: lfsr_attr_t attrs[1]; lfs_size_t attr_count = 0; if (condition) { attrs[attr_count++] = LFSR_ATTR(rid, tag, delta, data); } LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t)); Incremental attr allocation is more flexible, allowing nested conditions and conditions that span multiple attrs without sacrificing readability, though at a verbosity cost. We already need this for lfsr_btree_commit and lfsr_file_carve, adopting it everywhere we need conditional attrs allows us to drop the noop attr and avoid messy and hard-to-read C expressions. This also changes the lfsr_btree_commit to explicitly omit noop grows. We were relying on lfsr_rbyd_appendattr implicitly skipping these to avoid unnecessary attr commits, but I think it's probably better to make these noops explicit. This does add some code cost though, I'm guessing sequential conditional attrs landing at different offsets complicates code generation a bit: code stack before: 33940 2928 after: 34052 (+0.3%) 2928 (+0.0%) --- lfs.c | 117 +++++++++++++++++++++++++----------------- tests/test_btree.toml | 30 ++++++----- 2 files changed, 87 insertions(+), 60 deletions(-) diff --git a/lfs.c b/lfs.c index a1663ced..7a9f8367 100644 --- a/lfs.c +++ b/lfs.c @@ -1445,8 +1445,6 @@ typedef struct lfsr_attr { _delta, \ LFSR_DATA_##_data}) -#define LFSR_ATTR_NOOP() LFSR_ATTR(-1, GROW, 0, NULL()) - // TODO make this const again eventually #define LFSR_ATTRS(...) \ (const lfsr_attr_t[]){__VA_ARGS__}, \ @@ -2491,12 +2489,6 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, return LFS_ERR_RANGE; } - // ignore noops - // TODO is there a better way to represent noops? - if (!lfsr_tag_iswide(tag) && !lfsr_tag_key(tag) && delta == 0) { - return 0; - } - // make sure every rbyd starts with a revision count if (rbyd->eoff == 0) { int err = lfsr_rbyd_appendrev(lfs, rbyd, 0); @@ -4072,8 +4064,10 @@ static int lfsr_btree_commit_(lfs_t *lfs, attrs__[attr_count++] = LFSR_ATTR(bid+rid, BRANCH, 0, FROMBRANCH(&rbyd_, &buf__[buf_size])); buf_size += LFSR_BRANCH_DSIZE; - attrs__[attr_count++] = LFSR_ATTR(bid+rid, - GROW, -rbyd.weight + rbyd_.weight, NULL()); + if (rbyd_.weight != rbyd.weight) { + attrs__[attr_count++] = LFSR_ATTR(bid+rid, + GROW, -rbyd.weight + rbyd_.weight, NULL()); + } } attrs = attrs__; @@ -4355,8 +4349,10 @@ static int lfsr_btree_commit_(lfs_t *lfs, BRANCH, 0, FROMBRANCH(&rbyd_, &buf__[buf_size])); buf_size += LFSR_BRANCH_DSIZE; - attrs__[attr_count++] = LFSR_ATTR(bid+rid, - GROW, -rbyd.weight + rbyd_.weight, NULL()); + if (rbyd_.weight != rbyd.weight) { + attrs__[attr_count++] = LFSR_ATTR(bid+rid, + GROW, -rbyd.weight + rbyd_.weight, NULL()); + } attrs__[attr_count++] = LFSR_ATTR( bid+rid - rbyd.weight + rbyd_.weight + 1, BRANCH, +sibling.weight, @@ -4436,8 +4432,10 @@ static int lfsr_btree_commit_(lfs_t *lfs, attrs__[attr_count++] = LFSR_ATTR(bid+rid, BRANCH, 0, FROMBRANCH(&rbyd_, &buf__[buf_size])); buf_size += LFSR_BRANCH_DSIZE; - attrs__[attr_count++] = LFSR_ATTR(bid+rid, - GROW, -rbyd.weight + rbyd_.weight, NULL()); + if (rbyd_.weight != rbyd.weight) { + attrs__[attr_count++] = LFSR_ATTR(bid+rid, + GROW, -rbyd.weight + rbyd_.weight, NULL()); + } attrs = attrs__; rbyd = parent; @@ -8466,17 +8464,25 @@ int lfsr_mkdir(lfs_t *lfs, const char *path) { // commit our new directory into our parent, zeroing the grm in the // process - err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS( - LFSR_ATTR(mdir.mid + ((exists) ? 1 : 0), - DIR, +1, CAT( - LFSR_DATA_LEB128(did), - LFSR_DATA_BUF(name, name_size))), - LFSR_ATTR(mdir.mid + ((exists) ? 1 : 0), - DID, 0, LEB128(did_)), - (exists) - ? LFSR_ATTR(mdir.mid, RM, -1, NULL()) - : LFSR_ATTR_NOOP(), - LFSR_ATTR(-1, GRM, 0, GRM(&((lfsr_grm_t){{-1, -1}}))))); + lfsr_attr_t attrs[4]; + lfs_size_t attr_count = 0; + + lfsr_data_t datas[2] = { + LFSR_DATA_LEB128(did), + LFSR_DATA_BUF(name, name_size) + }; + attrs[attr_count++] = LFSR_ATTR(mdir.mid + ((exists) ? 1 : 0), + DIR, +1, DATA(lfsr_data_fromcat(datas, 2))); + attrs[attr_count++] = LFSR_ATTR(mdir.mid + ((exists) ? 1 : 0), + DID, 0, LEB128(did_)); + if (exists) { + attrs[attr_count++] = LFSR_ATTR(mdir.mid, RM, -1, NULL()); + } + lfsr_grm_t grm = {{-1, -1}}; + attrs[attr_count++] = LFSR_ATTR(-1, GRM, 0, GRM(&grm)); + + LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t)); + err = lfsr_mdir_commit(lfs, &mdir, attrs, attr_count); if (err) { return err; } @@ -8569,18 +8575,26 @@ int lfsr_remove(lfs_t *lfs, const char *path) { bool zombie = lfsr_mid_isopened(lfs, mdir.mid); // remove the metadata entry - err = lfsr_mdir_commit(lfs, &mdir, LFSR_ATTRS( - // create an orphan if zombied - // - // we use a create+delete here to also clear any attrs - // and trim the entry size - (zombie) - ? LFSR_ATTR(mdir.mid+1, ORPHAN, +1, CAT( - LFSR_DATA_LEB128(did), - LFSR_DATA_BUF(name, name_size))) - : LFSR_ATTR_NOOP(), - LFSR_ATTR(mdir.mid, RM, -1, NULL()), - LFSR_ATTR(-1, GRM, 0, GRM(&grm)))); + lfsr_attr_t attrs[3]; + lfs_size_t attr_count = 0; + + // create an orphan if zombied + // + // we use a create+delete here to also clear any attrs + // and trim the entry size + lfsr_data_t datas[2] = { + LFSR_DATA_LEB128(did), + LFSR_DATA_BUF(name, name_size) + }; + if (zombie) { + attrs[attr_count++] = LFSR_ATTR(mdir.mid+1, + ORPHAN, +1, DATA(lfsr_data_fromcat(datas, 2))); + } + attrs[attr_count++] = LFSR_ATTR(mdir.mid, RM, -1, NULL()); + attrs[attr_count++] = LFSR_ATTR(-1, GRM, 0, GRM(&grm)); + + LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t)); + err = lfsr_mdir_commit(lfs, &mdir, attrs, attr_count); if (err) { return err; } @@ -8726,17 +8740,24 @@ int lfsr_rename(lfs_t *lfs, const char *old_path, const char *new_path) { // rename our entry, copying all tags associated with the old rid to the // new rid, while also marking the old rid for removal - err = lfsr_mdir_commit(lfs, &new_mdir, LFSR_ATTRS( - LFSR_ATTR(new_mdir.mid + ((exists) ? 1 : 0), - TAG(old_tag), +1, CAT( - LFSR_DATA_LEB128(new_did), - LFSR_DATA_BUF(new_name, new_name_size))), - LFSR_ATTR(new_mdir.mid + ((exists) ? 1 : 0), - MOVE, 0, MOVE(&old_mdir)), - (exists) - ? LFSR_ATTR(new_mdir.mid, RM, -1, NULL()) - : LFSR_ATTR_NOOP(), - LFSR_ATTR(-1, GRM, 0, GRM(&grm)))); + lfsr_attr_t attrs[4]; + lfs_size_t attr_count = 0; + + lfsr_data_t datas[2] = { + LFSR_DATA_LEB128(new_did), + LFSR_DATA_BUF(new_name, new_name_size) + }; + attrs[attr_count++] = LFSR_ATTR(new_mdir.mid + ((exists) ? 1 : 0), + TAG(old_tag), +1, DATA(lfsr_data_fromcat(datas, 2))); + attrs[attr_count++] = LFSR_ATTR(new_mdir.mid + ((exists) ? 1 : 0), + MOVE, 0, MOVE(&old_mdir)); + if (exists) { + attrs[attr_count++] = LFSR_ATTR(new_mdir.mid, RM, -1, NULL()); + } + attrs[attr_count++] = LFSR_ATTR(-1, GRM, 0, GRM(&grm)); + + LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t)); + err = lfsr_mdir_commit(lfs, &new_mdir, attrs, attr_count); if (err) { return err; } diff --git a/tests/test_btree.toml b/tests/test_btree.toml index 0d02edb1..d8ec869a 100644 --- a/tests/test_btree.toml +++ b/tests/test_btree.toml @@ -85,18 +85,24 @@ code = ''' return err; } - return lfsr_btree_commit(lfs, btree, LFSR_ATTRS( - LFSR_ATTR(bid, GROW, +weight1-weight_, NULL()), - LFSR_ATTR(bid-(weight_-1)+weight1-1, TAG(tag1), 0, DATA(data1)), - (lfsr_data_size(&name) > 0 - ? LFSR_ATTR(bid-(weight_-1)+weight1, - NAME, +weight2, DATA(name)) - : LFSR_ATTR_NOOP()), - (lfsr_data_size(&name) > 0 - ? LFSR_ATTR(bid-(weight_-1)+weight1+weight2-1, - TAG(tag2), 0, DATA(data2)) - : LFSR_ATTR(bid-(weight_-1)+weight1, - TAG(tag2), +weight2, DATA(data2))))); + lfsr_attr_t attrs[4]; + lfs_size_t attr_count = 0; + + attrs[attr_count++] = LFSR_ATTR(bid, GROW, +weight1-weight_, NULL()); + attrs[attr_count++] = LFSR_ATTR(bid-(weight_-1)+weight1-1, + TAG(tag1), 0, DATA(data1)); + if (lfsr_data_size(&name) > 0) { + attrs[attr_count++] = LFSR_ATTR(bid-(weight_-1)+weight1, + NAME, +weight2, DATA(name)); + attrs[attr_count++] = LFSR_ATTR(bid-(weight_-1)+weight1+weight2-1, + TAG(tag2), 0, DATA(data2)); + } else { + attrs[attr_count++] = LFSR_ATTR(bid-(weight_-1)+weight1, + TAG(tag2), +weight2, DATA(data2)); + } + + LFS_ASSERT(attr_count <= sizeof(attrs)/sizeof(lfsr_attr_t)); + return lfsr_btree_commit(lfs, btree, attrs, attr_count); } '''