From f90906458ae38273ec7398068aff539c4ee73ffd Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Tue, 21 Jan 2025 14:30:43 -0600 Subject: [PATCH] rbyd: Terminate rbyd compaction layers with altgts This fixes a minor balance issue with rbyd commits after compaction, reducing the worst case rbyd height by ~1/2. I've been noticing some strange balance issues in our rbyds for a while, it turns out our algorithms for compaction and commit weren't playing nicely with each other. The problem is always terminating with altles, and subtleties around incomplete trees (non-powers-of-two). When we terminate with altles, we end up with a dangling null tag: rbyd log: rbyd tree: data a <. .---> a data b <--. .---r-b-> b data c <----. | .---> c data d <------. .---r-b-r-b-> d data e <--------. | .---b---b-> e altrle a <. | | | | r-b---------> null altble b -|-' | | | null | | | | altrle c <--.-' | | altble d -|-|---' | null | | | altble e <----.---' null | | | altrle b <. | | altble d -|-' | null | | altble e <--.-' null | | altrle d -' | altble e ---' null Which is fine, we're allowed to terminate rbyds with a null tag. The problem is that from the rbyd's view of the world, that null tag has the same balance as the rest of the entire tree. If we continue to append tags (which is a very common thing to do), we end up with a lopsided tree: rbyd log: rbyd tree: data a <. .---> a data b <--. .---r-b-> b data c <----. | .---> c data d <------. .---r-b-r-b-> d data e <--------. | .---b---b-> e altrle a <. | | | | r-b---------> null altble b -|-' | | | v null | | | | .---> a altrle c <--.-' | | .---r-b-> b altble d -|-|---' | | .---> c null | | | .---r-b-r-b-> d altble e <----.---' | .---b---b-> e null | | | r-b---------> f altrle b <. | | v altble d -|-' | .---> a null | | .---r-b-> b altble e <--.-' | .---> c null | | .-----r-b-r-b-> d altrle d | | | .-----b---b-> e altble e | | | | .---------> f null | | y-r-b---------> g altrle d | | v altble e | | .---> a data f <--. .---r-b-> b altrle d <. | | .---> c altrle e -|-| .---r-b-r-b-> d altble f | | .-y-r---b---b-> e data g <----. | .-----------> f altble e | | | | | .---------> g altrle f | | | b-r-b---------> h altble g | | | v data h <------. .---> a altble e -' | | | .---r-b-> b altrle f ---' | | | .---> c altrle g -----' | .---r-b-r-b-> d altble h -------' .---y-r---b---b-> e data i | .-------------> f | | .-----------> g | | | .---------> h b-y-r-b---------> i '---- h=4 --------' Fortunately the solution is relatively simple. If we terminate each layer of the compaction with an altgt, instead of an altle, the null tag becomes unreachable: rbyd log: rbyd tree: data a <. .---> a data b <--. .---r-b-> b data c <----. | .---> c data d <------. .---r-b-r-b-> d data e <--------. r-b---b---b-> e altrle a <. | | | | altble b -|-' | | | null | | | | altrle c <--.-' | | altble d -|-|---' | null | | | altbgt d <----.---' null | | | altrle b <. | | altble d -|-' | null | | altbgt d <--.-' null | | altrle d -' | altbgt d ---' null This preserves the balance of the tree: rbyd log: rbyd tree: data a <. .---> a data b <--. .---r-b-> b data c <----. | .---> c data d <------. .---r-b-r-b-> d data e <. | | | r-b---b---b-> e altrle a -| | | | v altble b -|-' | | .---> a null | | | .---r-b-> b altrle c <--.-' | | .---> c altble d -|-|---' .-r-b-r-b-> d null | | | .-> e altbgt d | | b---b---b-> f null | | v altrle b <. | .---> a altble d -|-' .---r-b-> b null | | .---> c altbgt d | .-r-b-r-b-> d null | | .---> e altrle d | | | .-> f altbgt d | b---b-r-b-> g null | v altble d | .---> a altbn d | .-----r-b-> b altble e | | .---> c data f <|-. .-r-b---r-b-> d altble d | | | .-----> e altbn | | | | .---> f altrle e | | | | | .-> g altble f | | b---b-y-r-b-> h data g <----. v altble d | | | .---> a altbn | | | .---r-b-> b altrle e <--. | | .---> c altrle f -|-| | .-r-b-r-b-> d altble g | | | | .---> e data h <------. | .-y-r-> f altble d -' | | | | | .---> g altble f ---' | | | | | .-> h altrle g -----' | b---b-r-b-> i altble h -------' data i '---- h=3 --' As a plus, this also makes it a bit easier to see compaction layers without decoding jumps, which is nice for debugging. --- It's worth noting the subtlety around when use use altgts vs altles here. We need to use altles in all but the last tag, in order to know what the largest tag was in each subtree when building the next layer. Fortunately, the last tag is the only tag where we need an altgt in order to make the null tag unreachable. This adds a bit of code, but preserving rbyd balance after compaction is well worth it: code stack ctx before: 38500 2624 640 after: 38512 (+0.0%) 2624 (+0.0%) 640 (+0.0%) --- lfs.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lfs.c b/lfs.c index 8e23038e..29c42a60 100644 --- a/lfs.c +++ b/lfs.c @@ -1332,7 +1332,7 @@ static inline void lfsr_tag_trim( LFS_ASSERT((lfsr_srid_t)weight >= 0); if (lfsr_tag_isgt(alt)) { *upper_rid -= weight; - if (upper_tag && !lfsr_tag_isn(alt)) { + if (upper_tag && !lfsr_tag_isa(alt)) { *upper_tag = alt + 1; } } else { @@ -4345,6 +4345,7 @@ static int lfsr_rbyd_appendcompaction(lfs_t *lfs, lfsr_rbyd_t *rbyd, // balanced binary tree upwards until we have a single trunk lfs_size_t layer = off; lfsr_rid_t weight = 0; + lfsr_tag_t tag_ = 0; while (true) { lfs_size_t layer_ = lfsr_rbyd_eoff(rbyd); off = layer; @@ -4404,23 +4405,30 @@ static int lfsr_rbyd_appendcompaction(lfs_t *lfs, lfsr_rbyd_t *rbyd, goto done; } - // connect with an altle + // connect with an altle/altgt // - // note we can't use an altas here, we need to encode the - // exact tag so we know the largest tag when building the - // next layer + // note we need to use altles for all but the last tag + // so we know the largest tag when building the next + // layer, but for that last tag we need an altgt so + // future appends maintain the balance of the tree err = lfsr_rbyd_appendtag(lfs, rbyd, - LFSR_TAG_ALT( - (i == 0 && off < layer_) - ? LFSR_TAG_R - : LFSR_TAG_B, - LFSR_TAG_LE, - tag), + (off < layer_) + ? LFSR_TAG_ALT( + (i == 0) ? LFSR_TAG_R : LFSR_TAG_B, + LFSR_TAG_LE, + tag) + : LFSR_TAG_ALT( + LFSR_TAG_B, + LFSR_TAG_GT, + tag_), weight, lfsr_rbyd_eoff(rbyd) - trunk); if (err) { return err; } + + // keep track of the previous tag for altgts + tag_ = tag; } // terminate with a null tag