Fixed recoloring tail-recursion violations during range removals
I spoke too soon and made a mistake when reenabling color preservation
during range removals.
I assumed, that thanks to replacing the diverging alt with a new black
alt for stitching together diverging trunks, we would avoid the issue
where a deleted diverging alt violates our rbyd's tail-recursive
recoloring invariant.
Unfortunately, this is not the case. All the stitching alt did was make
this violation more difficult to reach, but still reachable. Arguable a
worse situation.
Now, for this violation to happen, in addition to all of the other
requirements, we need the lower-diverging trunk to become empty.
This is the only case where we have no stitching alt, because we don't
need to stitch an empty trunk. Which means if the upper-diverging trunk
has yellow nodes both before and after the diverging alt, our
tail-recursive recoloring invariant can break.
Here's an example:
.-------------r-------------.
.-o-. .---+---y----. .-o-.
.o. .o. .o. .o. .o. .-y-+-. .o. .o.
a a a a a a a a c c c e e e e e e e
'--+--'
remove
Again, this doesn't capture the alt-layout, which _is_ important, so
here's the dbgrbyd.py view:
.-> aa .-> aa
.-b-> a .-b-> a
| .-> a | .-> a
.-----------b-b-> a .-------b-b-> a
| .-> a | .-> a
| .---------b-> a | .-b-> a
| | .-> a | | .-> a
| | .-------b-> a | .-b-b-> a
r-b-y-r-b-----b-> cc -. => y-y-r-b-----> ee <- two yellows!
| | '-> c + rm | '-----b-> e different dirs!
| | .-> c -' | '-> e should not happen!
| '-y-r-b-> ee | .-> e
| | '---> e | .-b-> e
| '-----> e | | .-> e
| .-> e '-----b-b-> e
| .-b-> e
| | .-> e
'---------b-b-> e
And the steps in our appendattr algorithm that led to this state, which
is insightful:
read <r => [<r]
read >b => [<r >b]
read <r => [<r >b <r]
read <r => [<r >b <r <r]
^--^------ red + red implies yellow
ysplit => [<r >r <b]
reorder => [<r <r >b]
^--^------------- yellow-same-dir invariant held
read >b => [<r <r >b >b]
diverge => [<r <r >b]
read >r => [<r <r >b >r]
read >r => <r [<r >b >r >r]
^-----------^-- our 4-alt fifo for flips/coloring
ysplit => <r [<r >r >b]
reorder => <r [>r >r <b]
^--^---------- yellow-same-dir invariant held
^---^------------- yellow-same-dir invariant NOT held
though 2 yellows is also a problem
The previous commit fixing this bug for the one-pass algorithm may also
be useful.
This tree is now tested in test_rbyd_delete_range_rydye and
test_rbyd_delete_range_rydye_backwards, though only
test_rbyd_delete_range_rydye_backwards reveals the bug, since the bug
requires _specifically_ the lower-diverging trunk to become empty (both
rydy and rydye now have in-order and backwards tests in case of other
chirality issues).
---
Taking a step back, and looking at this bug from a higher-level, the
core of the issue is that we are somewhat arbitrarily deleting nodes
after splitting nodes. This can break our tail-recursive recoloring
invariant.
What the heck is our tail-recursive recoloring invariant?
This is a property of 2-3-4 and greater B-trees, and transitively
red-black and red-black-yellow trees, that allows for tail-recursive,
self-balancing node insertion.
Basically, if you eagerly split any 4-nodes you encounter as you descend
down the tree, you will always be guaranteed to have an open slot in
your parent, so pushing up split nodes (or recoloring) only ever
propagates up a single level:
.-----. .-------. .-------.
|.a.h.| |.a.c.h.| |.a.c.h.|
'|-|-|' '|-|-|-|' '|-|-|-|'
| .-' '-. .-' '--.
v v v v v
.-------. .---. .---. .---. .-----.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.e.g.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|-|'
| | .-' '-.
v v v v
.-------. .-------. .---. .---.
|.d.e.f.| |.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|-|-|' '|-|' '|-|'
If you lazily split, you aren't guaranteed an open slot in your parent,
so you need recursion to solve splits. This is why 2-3 trees, though
self-balancing, are not tail-recursive:
.-----. .-----.
|.a.h.| |.a.h.|
'|-|-|' '|-|-|'
| |
v v
.-------. .'''''''''.
|.b.c.g.| => >.b.c.e.g.< 5!?
'|-|-|-|' '|.|.|.|.|'
| .-' '-.
v v v
.-------. .---. .---.
|.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|' '|-|'
But if you are eagerly splitting while also deleting nodes:
.-----. .-------. .-------. .'''''''''.
|.a.h.| |.a.c.h.| |.a.c.h.| 5!? >.a.c.e.h.<
'|-|-|' '|-|-|-|' '|-|-|-|' '|.|.|.|.|'
| .-' '-. .-' '---. .---' | '---.
v v v v v v v v
.-------. .---. .---. .---. .-------. .---. .---. .---.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.d.e.f.| => |.b.| |.d.| |.g.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|-|-|' '|-|' '|-|' '|-|'
| x | x
v v
.-------. .-------.
|.d.e.f.| |.d.e.f.|
'|-|-|-|' '|-|-|-|'
Suddenly, recursion. This is a problem.
The workaround implemented here is to check during pruning if our parent
may risk recursion, and if so, recolor the last alt so nothing will
break.
This ends up equivalent to the following transformation:
.-----. .-------. .-----. .-----.
|.a.h.| |.a.c.h.| |.a.c.| |.a.c.|
'|-|-|' '|-|-|-|' '|-|-|' '|-|-|'
| .-' '-. .-' '-. .-' '--.
v v v v v v v
.-------. .---. .---. .---. .---. .---. .-----.
|.b.c.g.| => |.b.| |.g.| => |.b.| |.h.| => |.b.| |.e.h.|
'|-|-|-|' '|-|' '|-|' '|-|' '|-|' '|-|' '|-|-|'
| x | x | .-' '-.
v v v v v
.-------. .-------. .-------. .---. .---.
|.d.e.f.| |.d.e.f.| |.d.e.f.| |.d.| |.f.|
'|-|-|-|' '|-|-|-|' '|-|-|-|' '|-|' '|-|'
You may notice this isn't exactly optimal. The >h branch ends up one
level lower, making the balance of the tree off by one. But it at least
ends up with a functional tree.
I may try to find a better solution...
---
The test_rbyd_delete_range_rydy/rydye tests should cover the cases where
a diverging alt is deleted.
I also tried to write tests for the cases where an alt is pruned, the
closest I got is in test_rbyd_delete_range_dryy_backwards, but I
couldn't actually come up with a sequence that would break our rbyds.
In theory it's possible, but it would need this substructure:
.-------> c y-r-b-------> c
y-r-b-y-r-b-> c or | | '-y-r-b-> c
| | | | | | | |
Which, as far as I can tell, can't actually be created with our current
algorithm...
Note the inverse structure:
.---------> c
| .-y-r-b-> c
y-r- | |
Will be pruned before it has a chance to split. So there is no invariant
concerns there. We only have issues when it's the tail alts that get
pruned, because we decide to split before we know if we are pruning or
not. I don't think this can be avoided without additional read-ahead.
Also, even if we could create the above substructure, because we are on
a diverged trunk, and by definition all alts point the same direction,
we would never end up violating our same-dir yellow invariant/assert...
Code changes:
code stack
before: 33880 2880
after: 33912 (+0.1%) 2880 (+0.0%)
This commit is contained in:
Christopher Haster
@@ -2904,11 +2904,12 @@ again:;
|
||||
// | | <b | <b |
|
||||
// | | .----'| | .----'| |
|
||||
// 1 2 3 4 4 1 2 3 4 4 2
|
||||
if (lfsr_tag_prune2(
|
||||
alt, weight,
|
||||
p_alts[0], p_weights[0],
|
||||
lower_rid, upper_rid,
|
||||
lower_tag, upper_tag)
|
||||
bool y_unreachable = lfsr_tag_prune2(
|
||||
alt, weight,
|
||||
p_alts[0], p_weights[0],
|
||||
lower_rid, upper_rid,
|
||||
lower_tag, upper_tag);
|
||||
if (y_unreachable
|
||||
// prune because of diverged paths?
|
||||
|| d_state == LFSR_D_DIVERGINGLOWER
|
||||
|| (lfsr_d_isdiverged(d_state)
|
||||
@@ -2943,7 +2944,14 @@ again:;
|
||||
jump = p_jumps[0];
|
||||
lfsr_rbyd_p_pop(p_alts, p_weights, p_jumps);
|
||||
} else {
|
||||
y_branch= branch;
|
||||
// if we're diverged pruning, we risk making recoloring
|
||||
// no longer tail-recursive, consuming potential yellow
|
||||
// alts avoids this
|
||||
if (!y_unreachable && lfsr_tag_isred(p_alts[1])) {
|
||||
LFS_ASSERT(lfsr_d_isdiverged(d_state));
|
||||
p_alts[1] &= ~LFSR_TAG_R;
|
||||
}
|
||||
y_branch = branch;
|
||||
branch = branch_;
|
||||
continue;
|
||||
}
|
||||
|
||||
+1898
-3
File diff suppressed because it is too large
Load Diff
Reference in New Issue
Block a user