From c08b7ccdd8fe82426698672a44265d52e6bf2b0b Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Sat, 30 Mar 2024 14:06:37 -0500 Subject: [PATCH] rbyd-rr: Fixed yellow-alt pruning being completely broken At some point during all this refactoring, `branch_ = branch` snuck its way into the common red-black pruning code: // collapse unreachable red alts if (lfsr_tag_isred(p_alts[0])) { alt = p_alts[0] & ~LFSR_TAG_R; weight = p_weights[0]; jump = p_jumps[0]; branch_ = branch; // <-- ??? lfsr_rbyd_p_pop(p_alts, p_weights, p_jumps); What this ends up doing is forcing the appendattr logic to branch to where it just was. Ignoring concerns about forward-progress, this somewhat humorously undoes the pruning of the alt. It's technically not an error, since the alt was prunable, but certainly counter-productive. First noticed because our post-split yellow alts were not getting cleaned up correctly, even though all the correct conditions were being hit. --- Unfortunately, attempting to simply remove that line breaks things. It turns out revisiting the pruned alt was hiding the fact that using an lfsr_tag_follow2(a_rid, a_tag) check to determine if we take the pruned alt is insufficient. At first glance this appears to be sufficient, after all if an alt is always taken, shouldn't lfsr_tag_follow2(a_rid, a_tag) always return true? The problem is when we look up a_rid/a_tag outside the tree. lfsr_tag_follow2(a_rid, a_tag) may return false, but _in the context of our current lower/upper bound_, the alt may always be taken and lfsr_tag_prune2() may return true. This mismatch in lfsr_tag_prune2 and lfsr_tag_follow2 breaks the underlying logic and causes the wrong branch to be taken. The fix here is to use the same reachability logic for both the pruning check and follow check. So a_rid/a_tag should not be involved in the pruning logic at all, which makes a bit of sense since a_rid/a_tag do not determine if an alt is reachable. I've also gone ahead and replaced lfsr_tag_prune{,2} with lfsr_tag_unreachable{,2} (never taken) and lfsr_tag_unavoidable{,2} (always taken) which I think capture/document the underlying conditions we need a bit better. Code changes: code stack before: 34220 2864 after: 34256 (+0.1%) 2864 (+0.0%) It's good that even though we changed a number of functions, the code changes match our expectation that the underlying logic didn't really change all that much. --- lfs.c | 98 +++++++++++++++++++++++++++++++++++++---------------------- 1 file changed, 61 insertions(+), 37 deletions(-) diff --git a/lfs.c b/lfs.c index 3a8a9cfd..6bcd0bda 100644 --- a/lfs.c +++ b/lfs.c @@ -967,36 +967,24 @@ static inline void lfsr_tag_trim2( lower_tag, upper_tag); } -static inline bool lfsr_tag_prune( +static inline bool lfsr_tag_unavoidable( lfsr_tag_t alt, lfsr_rid_t weight, lfsr_srid_t lower_rid, lfsr_srid_t upper_rid, lfsr_tag_t lower_tag, lfsr_tag_t upper_tag) { if (lfsr_tag_isgt(alt)) { - // unreachable? - return !lfsr_tag_follow( - alt, weight, - lower_rid, upper_rid, - upper_rid-1, upper_tag-1) - // only-reachable? - || lfsr_tag_follow( - alt, weight, - lower_rid, upper_rid, - lower_rid-1, lower_tag+1); + return lfsr_tag_follow( + alt, weight, + lower_rid, upper_rid, + lower_rid-1, lower_tag+1); } else { - // unreachable? - return !lfsr_tag_follow( - alt, weight, - lower_rid, upper_rid, - lower_rid-1, lower_tag+1) - // only-reachable? - || lfsr_tag_follow( - alt, weight, - lower_rid, upper_rid, - upper_rid-1, upper_tag-1); + return lfsr_tag_follow( + alt, weight, + lower_rid, upper_rid, + upper_rid-1, upper_tag-1); } } -static inline bool lfsr_tag_prune2( +static inline bool lfsr_tag_unavoidable2( lfsr_tag_t alt, lfsr_rid_t weight, lfsr_tag_t alt2, lfsr_rid_t weight2, lfsr_srid_t lower_rid, lfsr_srid_t upper_rid, @@ -1008,7 +996,42 @@ static inline bool lfsr_tag_prune2( &lower_tag, &upper_tag); } - return lfsr_tag_prune( + return lfsr_tag_unavoidable( + alt, weight, + lower_rid, upper_rid, + lower_tag, upper_tag); +} + +static inline bool lfsr_tag_unreachable( + lfsr_tag_t alt, lfsr_rid_t weight, + lfsr_srid_t lower_rid, lfsr_srid_t upper_rid, + lfsr_tag_t lower_tag, lfsr_tag_t upper_tag) { + if (lfsr_tag_isgt(alt)) { + return !lfsr_tag_follow( + alt, weight, + lower_rid, upper_rid, + upper_rid-1, upper_tag-1); + } else { + return !lfsr_tag_follow( + alt, weight, + lower_rid, upper_rid, + lower_rid-1, lower_tag+1); + } +} + +static inline bool lfsr_tag_unreachable2( + lfsr_tag_t alt, lfsr_rid_t weight, + lfsr_tag_t alt2, lfsr_rid_t weight2, + lfsr_srid_t lower_rid, lfsr_srid_t upper_rid, + lfsr_tag_t lower_tag, lfsr_tag_t upper_tag) { + if (lfsr_tag_isred(alt2)) { + lfsr_tag_trim( + alt2, weight2, + &lower_rid, &upper_rid, + &lower_tag, &upper_tag); + } + + return lfsr_tag_unreachable( alt, weight, lower_rid, upper_rid, lower_tag, upper_tag); @@ -2821,9 +2844,8 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, // note both normal and rm wide-tags have the same bounds, really it's // the normal non-wide-tags that are an outlier here if (lfsr_tag_issup(tag)) { - // TODO why can't this be rid+1? a_tag = 0x000; - b_tag = 0x800; + b_tag = 0xf00; } else if (lfsr_tag_issub(tag)) { a_tag = lfsr_tag_supkey(tag); b_tag = lfsr_tag_supkey(tag) + 0x100; @@ -2870,8 +2892,8 @@ again:; lfs_size_t branch = lfsr_rbyd_trunk(rbyd); lfsr_srid_t lower_rid = 0; lfsr_srid_t upper_rid = rbyd->weight; - lfsr_tag_t lower_tag = 0x0000; - lfsr_tag_t upper_tag = 0xffff; + lfsr_tag_t lower_tag = 0x000; + lfsr_tag_t upper_tag = 0xf00; // no trunk yet? if (!branch) { @@ -3006,20 +3028,23 @@ again:; // | |