Github/littlefs
1
Fork 0
Code Actions Activity

Dropped conditional/noop attrs, prefer incremental attr allocation

Browse Source 
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%)
This commit is contained in:
Christopher Haster
2024-01-20 15:01:46 -06:00
parent ff6d8a588e
commit 96b62ff804
2 changed files with 87 additions and 60 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lfs.c
+69 -48
View File
@@ -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;
}
tests/test_btree.toml
+18 -12
View File
@@ -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);
}
'''