From f06ef46e8bdc80d01113cf111599b7827f8a9a7c Mon Sep 17 00:00:00 2001 From: Christopher Haster Date: Mon, 8 Apr 2024 20:35:51 -0500 Subject: [PATCH] rbyd-rr: Simplified diverging state machine, rely on relative a/b ordering So instead of explicitly keeping track of which bound we are on, either via separate DIVERGEDLOWER/DIVERGEDUPPER states or a d_upper bool, we can infer the bound based on the relative ordering a_rid/tag and b_rid/tag: - a_rid < b_rid || a_tag < b_tag => lower bound - a_rid > b_rid || a_tag > b_tag => upper bound - a_rid == b_rid && a_tag == b_tag => not diverging This is more appealing now that we don't rely on the specific bound for diverged triming. The only remaining state is if we have diverged yet, a simple boolean. Measuring code size was a bit confusing. During a partial edit, it looked like this was going to save a bit of code, but the result was actually worse. It seems that explicitly masking/oring a single bit in the original uint8_t d_state is somehow cheaper than storing if we have diverged as a bool? code stack before: 34516 2864 bitmask: 34504 (-0.0%) 2864 (+0.0%) boolean: 34528 (+0.0%) 2864 (+0.0%) code frame stack appendattr before: 2366 216 568 appendattr bitmask: 2354 (-0.5%) 216 (+0.0%) 568 (+0.0%) appendattr boolean: 2378 (+0.5%) 216 (+0.0%) 568 (+0.0%) No idea why this would happen. If feels like some sort of compiler/optimizer bug... But this is pretty close to the compiler noise floor and compilers aren't perfect. I'm probably reading too much into an extra 24 bytes... This is still a worthwhile change as it's usually good to prefer implicit state over explicit. Less things can fall out of sync this way. --- lfs.c | 99 +++++++++++++++++++++++------------------------------------ 1 file changed, 39 insertions(+), 60 deletions(-) diff --git a/lfs.c b/lfs.c index d74e573d..f2bc4b66 100644 --- a/lfs.c +++ b/lfs.c @@ -2785,30 +2785,6 @@ static void lfsr_p_recolor( } } -// diverged state machine for range appends -enum { - LFSR_D_NOTDIVERGEDLOWER = 0x0, - LFSR_D_NOTDIVERGEDUPPER = 0x1, - LFSR_D_DIVERGEDLOWER = 0x2, - LFSR_D_DIVERGEDUPPER = 0x3, -}; - -static inline bool lfsr_d_isdiverged(uint8_t d_state) { - return d_state & LFSR_D_DIVERGEDLOWER; -} - -static inline bool lfsr_d_isupper(uint8_t d_state) { - return d_state & LFSR_D_NOTDIVERGEDUPPER; -} - -static inline bool lfsr_d_islower(uint8_t d_state) { - return !lfsr_d_isupper(d_state); -} - -static inline uint8_t lfsr_d_diverge(uint8_t d_state) { - return d_state |= LFSR_D_DIVERGEDLOWER; -} - // core rbyd algorithm static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, lfsr_srid_t rid, lfsr_tag_t tag, lfsr_srid_t delta, lfsr_data_t data) { @@ -2895,7 +2871,7 @@ static int lfsr_rbyd_appendattr(lfs_t *lfs, lfsr_rbyd_t *rbyd, // 2. to write the common trunk + diverged-upper trunk, stitching the // two diverged trunks together where they diverged // - uint8_t d_state = LFSR_D_NOTDIVERGEDLOWER; + bool diverged = false; lfsr_srid_t d_rid = 0; lfsr_tag_t d_tag = 0; @@ -2953,7 +2929,7 @@ trunk:; lfs_size_t branch_ = branch + d; // do bounds want to take different paths? begin diverging - if (!lfsr_d_isdiverged(d_state) + if (!diverged // diverging black? && (((lfsr_tag_isblack(alt) // give up if we find a yellow alt @@ -2971,7 +2947,7 @@ trunk:; lower_rid, upper_rid, a_rid, a_tag, b_rid, b_tag)))) { - d_state = lfsr_d_diverge(d_state); + diverged = true; // diverging red? flip if (lfsr_tag_isred(p[0].alt) @@ -3012,7 +2988,7 @@ trunk:; } // diverging upper? stitch together both trunks - if (lfsr_d_isupper(d_state)) { + if (a_rid > b_rid || a_tag > b_tag) { if (lfsr_tag_isgt(alt)) { lfsr_tag_flip2( &alt, &weight, @@ -3041,7 +3017,7 @@ trunk:; } // force diverged alts to be pruned - } else if (lfsr_d_isdiverged(d_state) + } else if (diverged && lfsr_tag_diverging2( alt, weight, p[0].alt, p[0].weight, @@ -3245,39 +3221,42 @@ trunk:; // the last alt should always end up black LFS_ASSERT(lfsr_tag_isblack(p[0].alt)); - // diverged lower trunk? move on to upper trunk - if (d_state == LFSR_D_DIVERGEDLOWER) { - d_state = LFSR_D_NOTDIVERGEDUPPER; - // keep track of the lower diverged bound - d_rid = lower_rid; - d_tag = lower_tag; + if (diverged) { + // diverged lower trunk? move on to upper trunk + if (a_rid < b_rid || a_tag < b_tag) { + // keep track of the lower diverged bound + d_rid = lower_rid; + d_tag = lower_tag; - // flush any pending alts - err = lfsr_p_flush(lfs, rbyd, p, 3); - if (err) { - return err; + // flush any pending alts + err = lfsr_p_flush(lfs, rbyd, p, 3); + if (err) { + return err; + } + + // terminate diverged trunk with an unreachable tag + err = lfsr_rbyd_appendattr_(lfs, rbyd, + (lfsr_rbyd_isshrub(rbyd) ? LFSR_TAG_SHRUB : 0) + | LFSR_TAG_NULL, + 0, + LFSR_DATA_NULL()); + if (err) { + return err; + } + + // swap tag/rid and move on to upper trunk + diverged = false; + branch = trunk_; + lfs_swap16(&a_tag, &b_tag); + lfs_sswap32(&a_rid, &b_rid); + goto trunk; + + } else { + // use the lower diverged bound for leaf weight + // calculation + lower_rid = d_rid; + lower_tag = d_tag; } - - // terminate diverged trunk with an unreachable tag - err = lfsr_rbyd_appendattr_(lfs, rbyd, - (lfsr_rbyd_isshrub(rbyd) ? LFSR_TAG_SHRUB : 0) - | LFSR_TAG_NULL, - 0, - LFSR_DATA_NULL()); - if (err) { - return err; - } - - // swap tag/rid and move on to upper trunk - branch = trunk_; - lfs_swap16(&a_tag, &b_tag); - lfs_sswap32(&a_rid, &b_rid); - goto trunk; - - } else if (d_state == LFSR_D_DIVERGEDUPPER) { - // use the lower diverged bound for leaf weight calculation - lower_rid = d_rid; - lower_tag = d_tag; } goto stem;