From 794bd3df617fa10b11f58f16342d60098c44deb8 Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 15 Jul 2025 18:48:42 -0500 Subject: [PATCH] btree: Slightly tweaked lfs3_btree_commit_'s internal gotos This moves the default recurse logic (previously the commit label) back up before the compact/relocate/split/merge branches. I know the general rule is to try to limit gotos to foward jumps, but in this case, placing the default recurse logic at the end of lfs3_btree_commit_ disrupts the default "happy" path and makes refactoring more difficult than it needs to be. Contextually, the default recurse logic is a part of the default commit logic, and split, merge, etc, are exceptional branches that just happen to sometimes converge. --- I think the real problem is that all of the gotos in lfs3_btree_commit_ are modeling mutually recursive functions, but in a context where we can't actually recurse. _Technically_, it is possible to transform any tail-recursive function into loops and if statements (structured program theorem), but doing so risks significant code duplication. We could duplicate this recurse logic everywhere it's needed for example. But this is also something we want to avoid in littlefs. So goto soup it is. --- Some code changes, but probably just compiler noise: code stack ctx before: 37052 2416 652 after: 37048 (-0.0%) 2416 (+0.0%) 652 (+0.0%) I also added some more informative-only labels now that we've adopted -Wno-unused-label. These are useful for documenting independent chunks of logic in a large function like this, and as debugging targets. --- lfs3.c | 98 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 50 insertions(+), 48 deletions(-) diff --git a/lfs3.c b/lfs3.c index 31914a9a..df3918f1 100644 --- a/lfs3.c +++ b/lfs3.c @@ -5574,7 +5574,52 @@ static int lfs3_btree_commit_(lfs3_t *lfs3, return err; } - goto commit; + recurse:; + // propagate successful commits + + // done? + if (!lfs3_rbyd_trunk(&parent)) { + // update the root + // (note btree_ == child_) + // no new root needed + *rattr_count = 0; + return 0; + } + + // is our parent the root and is the root degenerate? + if (child.weight == btree->weight) { + // collapse the root, decreasing the height of the tree + // (note btree_ == child_) + // no new root needed + *rattr_count = 0; + return 0; + } + + // prepare commit to parent, tail recursing upwards + // + // note that since we defer merges to compaction time, we can + // end up removing an rbyd here + rattr_count_ = 0; + bid_ -= pid - (child.weight-1); + if (child_->weight == 0) { + bctx->rattrs[rattr_count_++] = LFS3_RATTR( + LFS3_TAG_RM, -child.weight); + } else { + lfs3_data_t branch = lfs3_data_frombranch( + child_, &bctx->buf[0*LFS3_BRANCH_DSIZE]); + bctx->rattrs[rattr_count_++] = LFS3_RATTR_BUF( + LFS3_TAG_BRANCH, 0, + branch.u.buffer, lfs3_data_size(branch)); + if (child_->weight != child.weight) { + bctx->rattrs[rattr_count_++] = LFS3_RATTR( + LFS3_TAG_GROW, -child.weight + child_->weight); + } + } + rattrs_ = bctx->rattrs; + + child = parent; + rid_ = pid; + continue; compact:; // estimate our compacted size @@ -5746,7 +5791,7 @@ static int lfs3_btree_commit_(lfs3_t *lfs3, return err; } - goto commit; + goto recurse; split:; // we should have something to split here @@ -5871,9 +5916,10 @@ static int lfs3_btree_commit_(lfs3_t *lfs3, if (child_->weight == 0) { *child_ = sibling; } - goto commit; + goto recurse; } + split_recurse:; // lookup first name in sibling to use as the split name // // note we need to do this after playing out pending rattrs in case @@ -6000,6 +6046,7 @@ static int lfs3_btree_commit_(lfs3_t *lfs3, return err; } + merge_recurse:; // we must have a parent at this point, but is our parent the root // and is the root degenerate? LFS3_ASSERT(lfs3_rbyd_trunk(&parent)); @@ -6032,51 +6079,6 @@ static int lfs3_btree_commit_(lfs3_t *lfs3, child = parent; rid_ = pid + sibling.weight; continue; - - commit:; - // done? - if (!lfs3_rbyd_trunk(&parent)) { - // update the root - // (note btree_ == child_) - // no new root needed - *rattr_count = 0; - return 0; - } - - // is our parent the root and is the root degenerate? - if (child.weight == btree->weight) { - // collapse the root, decreasing the height of the tree - // (note btree_ == child_) - // no new root needed - *rattr_count = 0; - return 0; - } - - // prepare commit to parent, tail recursing upwards - // - // note that since we defer merges to compaction time, we can - // end up removing an rbyd here - rattr_count_ = 0; - bid_ -= pid - (child.weight-1); - if (child_->weight == 0) { - bctx->rattrs[rattr_count_++] = LFS3_RATTR( - LFS3_TAG_RM, -child.weight); - } else { - lfs3_data_t branch = lfs3_data_frombranch( - child_, &bctx->buf[0*LFS3_BRANCH_DSIZE]); - bctx->rattrs[rattr_count_++] = LFS3_RATTR_BUF( - LFS3_TAG_BRANCH, 0, - branch.u.buffer, lfs3_data_size(branch)); - if (child_->weight != child.weight) { - bctx->rattrs[rattr_count_++] = LFS3_RATTR( - LFS3_TAG_GROW, -child.weight + child_->weight); - } - } - rattrs_ = bctx->rattrs; - - child = parent; - rid_ = pid; - continue; } } #endif