Replace recursive tree traversal #58

wngr · 2021-03-16T15:24:34Z

Closes https://github.com/Actyx/Cosmos/issues/5824
Closes https://github.com/Actyx/Cosmos/issues/6153
~~next step is to filter_chunked_reverse ..~~

wngr · 2021-03-16T15:25:00Z

banyan/tests/common/mod.rs

+        let digest: [u8; 32] = value.hash().digest().try_into()?;
+        Ok(Self(digest))
+    }
+}


.. just some code shuffling in the tests

banyan/src/forest/read.rs

rklaehn

First of all: AWESOME!!!

Now, some remarks:

I think it is not clear whether it is worth it to have the iterator do double duty as forward and backward iterator. There are so many mode switches that it might be nicer to just have a bit more duplicate code and have a separate forward and backward iterator. Maybe the code can be DRYed in another way.
what would be awesome would be if you could combine the two stacks into one, and use a struct with named fields for that. It seems that one stack is "lagging" the other one, but other than that they seem to be used in the same way. The depth is going to be the same at all times +-1. So a single stack might make the code more readable...

banyan/src/forest/read.rs

wngr · 2021-03-17T13:33:23Z

@rklaehn I combined both stacks into one, and created a named struct to use it with: TraverseState. I tried to DRY up the code a bit with that, but did not split up the forward and backwards case -- you think this is sufficiently clear now or should I split it up (or better: you have an idea to make it clearer but not split it up :-))?

code a bit

banyan/src/forest/read.rs

rklaehn · 2021-03-17T14:53:33Z

banyan/src/forest/read.rs

-                        Mode::Backward => *pos == usize::MAX,
-                    };
-                    if new_branch {
+                    if head.filter.is_none() {


Much nicer this way, I think.

rklaehn · 2021-03-17T14:58:34Z

banyan/src/forest/read.rs

@@ -162,44 +183,39 @@ where
                        self.query
                            .intersecting(start_offset, index, &mut q_matching);
                        debug_assert_eq!(branch.children.len(), q_matching.len());
-                        let _ = std::mem::replace(matching, q_matching);
+                        head.filter.replace(q_matching);

                        if matches!(self.mode, Mode::Backward) {


I think it would be clearer here to set the position in both cases, instead of using the default if Mode is Forward.

banyan/src/forest/read.rs

rklaehn

Looks very good. Will probably approve. Just checking this out to look at it some more.

rklaehn · 2021-03-17T15:04:32Z

I think this would benefit from a test that builds a super-degenerate tree aka linked list with 10000 iitems and then tries to traverse it. I think we should be able to make this reliably fail with the previous impl. I will write it.

when reading this, I thought - oh shit, there is a nested loop. But actually there is just one.

banyan/src/forest/read.rs

rklaehn · 2021-03-17T18:17:13Z

I think there is a bug with the range for the placeholder. Also, I have experimented with how to simplify the stack frame. See #59

rklaehn · 2021-03-17T18:27:41Z

banyan/src/forest/read.rs

+    index: Arc<Index<T>>,
+    // If `index` points to a branch node, `position` points to the currently
+    // traversed child
+    position: usize,


It is probably worth noting that this is not the child offset, but the child offset in case of forward traversal, and the child offset + 1 in case of backward traversal.

I guess you have already tried to make this an isize and have it be exactly the position?

I tried this out here: 10bfa3d

Cool! Let's get it in!
I haven't tried that. At some point I kind of lost sight ..

rklaehn · 2021-03-17T18:47:49Z

Here is an experiment, making position an isize so it can refer directly to the child:

https://github.com/Actyx/banyan/tree/rkl/experiments

Possible traversal improvements

banyan/src/forest/read.rs

… of the child

rklaehn

I might still try to DRY this, especially the forward/backwards branches. But I think it reads quite well now. Great work.

rkuhn

Yes, quite readable, nice and tidy! Just a few nitpicks.

banyan/src/forest/read.rs

rkuhn · 2021-03-18T09:45:26Z

banyan/src/forest/read.rs

+    type Item = Result<FilteredChunk<T, V, E>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let res: FilteredChunk<T, V, E> = loop {


given that I’ll find a break x somewhere in here, maybe just return Some(Ok(x)) instead?

Hmm, that's a matter of preference I guess. I like the break approach better. Less boilerplate in the loop.

banyan/src/forest/read.rs

rkuhn · 2021-03-18T10:51:39Z

banyan/src/forest/read.rs

+                continue;
+            }
+
+            match self.forest.load_node(&head.index) {


side remark: it feels weird to apply a “load node” function to an index data structure when in my head the index is part of said node — maybe I got this wrong

no, it is indeed a bit weird. All nodes in banyan are "split in half". It is not very object oriented, since data that you would logically group together is separate so it is ordered by probability / frequency of access.

banyan/src/forest/read.rs

rkuhn · 2021-03-18T11:00:54Z

banyan/src/forest/read.rs

+                            extra: (self.mk_extra)(index.as_index_ref()),
+                        };
+
+                        break placeholder;


why is it necessary to emit these?

the placeholders?

If you skip a subtree because either the filter does not match, or the subtree has been purged, you still want a placeholder to indicate that you have skipped offsets x..x+n. There is also the mk_extra fn that can extract some info from the tree node, e.g. the last lamport.

Note that iter_filtered_chunked is a low level API that can be used in multiple ways. If you want just the data, you can just flatten them away.

banyan/src/forest/read.rs

rkuhn · 2021-03-18T11:06:46Z

banyan/src/forest/read.rs

+                // tree by using an appropriate mk_extra fn, or check
+                // `data.len()`.
+                Ok(_) => {
+                    let TraverseState { index, .. } = self.stack.pop().expect("not empty");


This assumes that the tree always has at least one branch, correct? With time-based expiry I could imagine a tree that is pruned completely empty.

A tree where all events have been pruned will be branch containing a summary of the pruned data, including the offset range. You can never go from a non-empty tree to an empty tree.

The initial empty tree is peeled off with the struct Tree, which is an option of a non empty tree.

But - it would be nicer to more explicitly spell out the invariants. So might be worth trying out https://crates.io/crates/contracts for banyan - if there is a way to switch the stuff off without switching to release mode...

Made an issue: #60

In other words: If the root node is a PrunedBranch, we will end up in this case with Ok(PrunedBranch(index)) and everything is fine. This code won't panic.

wngr added 3 commits March 16, 2021 14:44

restructure test organization

6ef3897

add non-recursive tree traversal

d989eaf

replace iter_filtered_chunked0 with traverse0

245f3b2

wngr commented Mar 16, 2021

View reviewed changes

wngr requested a review from rklaehn March 16, 2021 15:25

change stream_filtered_chunked to use traverse

455185c

wngr force-pushed the ow/iter-iter-iter branch from ed83a94 to 5f7ce1e Compare March 16, 2021 20:31

add DoubleEndedIter impl for ForestIter

a105154

wngr force-pushed the ow/iter-iter-iter branch from 5f7ce1e to a105154 Compare March 16, 2021 20:42

wngr added 3 commits March 16, 2021 21:05

add some more iter based convenience methods and cleanup

86a2f8b

honor offset passed into reverse traversal

da5b67b

dry forward and backward iteration

c91803c

wngr force-pushed the ow/iter-iter-iter branch from 24032ca to c91803c Compare March 16, 2021 21:35

wngr added 2 commits March 17, 2021 07:25

dont expose offset for reverse traversal

02dde24

add test comparing forward and reverse case

6fb7563