neon: Investigate possible bug in `im` crate

If the bug is not in the im crate then it’s in our layer map so it’s good to check. If the bug is in the im crate, it’s good to minimize and report/fix

More context:

_Originally posted by @bojanserafimov in https://github.com/neondatabase/neon/issues/2998#issuecomment-1381432473_

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 20 (20 by maintainers)

Most upvoted comments

with the following IM patch layer bench normally completed:

diff --git a/src/nodes/btree.rs b/src/nodes/btree.rs
index 84f63fa..2e6452e 100644
--- a/src/nodes/btree.rs
+++ b/src/nodes/btree.rs
@@ -235,6 +235,24 @@ impl<A: BTreeValue> Node<A> {
         }
     }
 
+    fn child<'a, BK>(&'a self, key: &BK, index: usize) -> Option<&A>
+    where
+        BK: Ord + ?Sized,
+        A::Key: Borrow<BK>,
+    {
+        match self.children[index] {
+            None if index == 0 => None,
+            None => self.keys.get(index - 1).map(|_| &self.keys[index - 1]),
+            Some(ref node) => node.lookup_prev(key).or_else(|| {
+                if index == 0 {
+                    None
+                } else {
+                    self.child(key, index - 1)
+                }
+            }),
+        }
+    }
+
     pub(crate) fn lookup_prev<'a, BK>(&'a self, key: &BK) -> Option<&A>
     where
         BK: Ord + ?Sized,
@@ -245,11 +263,7 @@ impl<A: BTreeValue> Node<A> {
         }
         match A::search_key(&self.keys, key) {
             Ok(index) => Some(&self.keys[index]),
-            Err(index) => match self.children[index] {
-                None if index == 0 => None,
-                None => self.keys.get(index - 1).map(|_| &self.keys[index - 1]),
-                Some(ref node) => node.lookup_prev(key),
-            },
+            Err(index) => self.child(key, index),
         }
     }

Performance with patched IM and get_prev is the following:

captest_uniform_queries time:   [3.4951 ms 3.5300 ms 3.5703 ms]
captest_rel_dir_query   time:   [188.40 ns 190.01 ns 191.80 ns]
Finished layer map init in 764.03677ms
real_map/uniform_queries
                        time:   [3.7333 ms 3.8060 ms 3.8843 ms]
real_map/get_difficulty_map
                        time:   [19.668 ms 20.244 ms 20.842 ms]
Finished layer map init in 625.197474ms
sequential/uniform_queries
                        time:   [10.334 µs 10.756 µs 11.213 µs]

Seems to be not “times” faster I have replaced next_back() only in one place - in query(&self, key: i128). It seems to me that you wrote that get_prev() is 70% faster. On which workload?