Pre-calculate heap element scores (retaining RouteGraphNode size) 2023-12-2551-followups
authorMatt Corallo <git@bluematt.me>
Wed, 6 Dec 2023 05:29:28 +0000 (05:29 +0000)
committerMatt Corallo <git@bluematt.me>
Fri, 8 Dec 2023 20:45:06 +0000 (20:45 +0000)
`RouteGraphNode` currently recalculates scores in its `Ord`
implementation, wasting time while sorting the main Dijkstra's
heap.

Further, some time ago, when implementing the `htlc_maximum_msat`
amount reduction while walking the graph, we added
`PathBuildingHop::was_processed`, looking up the source node in
`dist` each time we pop'ed an element off of the binary heap.
As a result, we now have a reference to our `PathBuildingHop` when
processing a best-node's channels, leading to several fields in
`RouteGraphNode` being entirely redundant.

Here we drop those fields, but add a pre-calculated score field,
as well as force a suboptimal `RouteGraphNode` layout, retaining
its existing 64 byte size.

Without the suboptimal layout, performance is very mixed, but with
it performance is mostly improved, by around 10% in most tests.

lightning/src/routing/router.rs

index e2ccd1f73ed0ff41c257d6022fb47db42d27ea33..9423078749d39175790a6f298831048db5482a63 100644 (file)
@@ -968,33 +968,24 @@ impl_writeable_tlv_based!(RouteHintHop, {
 });
 
 #[derive(Eq, PartialEq)]
+#[repr(align(64))] // Force the size to 64 bytes
 struct RouteGraphNode {
        node_id: NodeId,
-       lowest_fee_to_node: u64,
-       total_cltv_delta: u32,
+       score: u64,
        // The maximum value a yet-to-be-constructed payment path might flow through this node.
        // This value is upper-bounded by us by:
        // - how much is needed for a path being constructed
        // - how much value can channels following this node (up to the destination) can contribute,
        //   considering their capacity and fees
        value_contribution_msat: u64,
-       /// The effective htlc_minimum_msat at this hop. If a later hop on the path had a higher HTLC
-       /// minimum, we use it, plus the fees required at each earlier hop to meet it.
-       path_htlc_minimum_msat: u64,
-       /// All penalties incurred from this hop on the way to the destination, as calculated using
-       /// channel scoring.
-       path_penalty_msat: u64,
+       total_cltv_delta: u32,
        /// The number of hops walked up to this node.
        path_length_to_node: u8,
 }
 
 impl cmp::Ord for RouteGraphNode {
        fn cmp(&self, other: &RouteGraphNode) -> cmp::Ordering {
-               let other_score = cmp::max(other.lowest_fee_to_node, other.path_htlc_minimum_msat)
-                       .saturating_add(other.path_penalty_msat);
-               let self_score = cmp::max(self.lowest_fee_to_node, self.path_htlc_minimum_msat)
-                       .saturating_add(self.path_penalty_msat);
-               other_score.cmp(&self_score).then_with(|| other.node_id.cmp(&self.node_id))
+               other.score.cmp(&self.score).then_with(|| other.node_id.cmp(&self.node_id))
        }
 }
 
@@ -1004,6 +995,16 @@ impl cmp::PartialOrd for RouteGraphNode {
        }
 }
 
+// While RouteGraphNode can be laid out with fewer bytes, performance appears to be improved
+// substantially when it is laid out at exactly 64 bytes.
+//
+// Thus, we use `#[repr(C)]` on the struct to force a suboptimal layout and check that it stays 64
+// bytes here.
+#[cfg(any(ldk_bench, not(any(test, fuzzing))))]
+const _GRAPH_NODE_SMALL: usize = 64 - core::mem::size_of::<RouteGraphNode>();
+#[cfg(any(ldk_bench, not(any(test, fuzzing))))]
+const _GRAPH_NODE_FIXED_SIZE: usize = core::mem::size_of::<RouteGraphNode>() - 64;
+
 /// A wrapper around the various hop representations.
 ///
 /// Can be used to examine the properties of a hop,
@@ -2120,15 +2121,6 @@ where L::Target: Logger {
                                                                                score_params);
                                                                let path_penalty_msat = $next_hops_path_penalty_msat
                                                                        .saturating_add(channel_penalty_msat);
-                                                               let new_graph_node = RouteGraphNode {
-                                                                       node_id: src_node_id,
-                                                                       lowest_fee_to_node: total_fee_msat,
-                                                                       total_cltv_delta: hop_total_cltv_delta,
-                                                                       value_contribution_msat,
-                                                                       path_htlc_minimum_msat,
-                                                                       path_penalty_msat,
-                                                                       path_length_to_node,
-                                                               };
 
                                                                // Update the way of reaching $candidate.source()
                                                                // with the given short_channel_id (from $candidate.target()),
@@ -2153,6 +2145,13 @@ where L::Target: Logger {
                                                                        .saturating_add(path_penalty_msat);
 
                                                                if !old_entry.was_processed && new_cost < old_cost {
+                                                                       let new_graph_node = RouteGraphNode {
+                                                                               node_id: src_node_id,
+                                                                               score: cmp::max(total_fee_msat, path_htlc_minimum_msat).saturating_add(path_penalty_msat),
+                                                                               total_cltv_delta: hop_total_cltv_delta,
+                                                                               value_contribution_msat,
+                                                                               path_length_to_node,
+                                                                       };
                                                                        targets.push(new_graph_node);
                                                                        old_entry.next_hops_fee_msat = $next_hops_fee_msat;
                                                                        old_entry.hop_use_fee_msat = hop_use_fee_msat;
@@ -2226,18 +2225,26 @@ where L::Target: Logger {
        // meaning how much will be paid in fees after this node (to the best of our knowledge).
        // This data can later be helpful to optimize routing (pay lower fees).
        macro_rules! add_entries_to_cheapest_to_target_node {
-               ( $node: expr, $node_id: expr, $fee_to_target_msat: expr, $next_hops_value_contribution: expr,
-                 $next_hops_path_htlc_minimum_msat: expr, $next_hops_path_penalty_msat: expr,
+               ( $node: expr, $node_id: expr, $next_hops_value_contribution: expr,
                  $next_hops_cltv_delta: expr, $next_hops_path_length: expr ) => {
+                       let fee_to_target_msat;
+                       let next_hops_path_htlc_minimum_msat;
+                       let next_hops_path_penalty_msat;
                        let skip_node = if let Some(elem) = dist.get_mut(&$node_id) {
                                let was_processed = elem.was_processed;
                                elem.was_processed = true;
+                               fee_to_target_msat = elem.total_fee_msat;
+                               next_hops_path_htlc_minimum_msat = elem.path_htlc_minimum_msat;
+                               next_hops_path_penalty_msat = elem.path_penalty_msat;
                                was_processed
                        } else {
                                // Entries are added to dist in add_entry!() when there is a channel from a node.
                                // Because there are no channels from payee, it will not have a dist entry at this point.
                                // If we're processing any other node, it is always be the result of a channel from it.
                                debug_assert_eq!($node_id, maybe_dummy_payee_node_id);
+                               fee_to_target_msat = 0;
+                               next_hops_path_htlc_minimum_msat = 0;
+                               next_hops_path_penalty_msat = 0;
                                false
                        };
 
@@ -2247,9 +2254,9 @@ where L::Target: Logger {
                                                let candidate = CandidateRouteHop::FirstHop {
                                                        details, payer_node_id: &our_node_id,
                                                };
-                                               add_entry!(&candidate, $fee_to_target_msat,
+                                               add_entry!(&candidate, fee_to_target_msat,
                                                        $next_hops_value_contribution,
-                                                       $next_hops_path_htlc_minimum_msat, $next_hops_path_penalty_msat,
+                                                       next_hops_path_htlc_minimum_msat, next_hops_path_penalty_msat,
                                                        $next_hops_cltv_delta, $next_hops_path_length);
                                        }
                                }
@@ -2272,10 +2279,10 @@ where L::Target: Logger {
                                                                                        short_channel_id: *chan_id,
                                                                                };
                                                                                add_entry!(&candidate,
-                                                                                       $fee_to_target_msat,
+                                                                                       fee_to_target_msat,
                                                                                        $next_hops_value_contribution,
-                                                                                       $next_hops_path_htlc_minimum_msat,
-                                                                                       $next_hops_path_penalty_msat,
+                                                                                       next_hops_path_htlc_minimum_msat,
+                                                                                       next_hops_path_penalty_msat,
                                                                                        $next_hops_cltv_delta, $next_hops_path_length);
                                                                        }
                                                                }
@@ -2320,7 +2327,7 @@ where L::Target: Logger {
                        // If not, targets.pop() will not even let us enter the loop in step 2.
                        None => {},
                        Some(node) => {
-                               add_entries_to_cheapest_to_target_node!(node, payee, 0, path_value_msat, 0, 0u64, 0, 0);
+                               add_entries_to_cheapest_to_target_node!(node, payee, path_value_msat, 0, 0);
                        },
                });
 
@@ -2527,7 +2534,7 @@ where L::Target: Logger {
                // Both these cases (and other cases except reaching recommended_value_msat) mean that
                // paths_collection will be stopped because found_new_path==false.
                // This is not necessarily a routing failure.
-               'path_construction: while let Some(RouteGraphNode { node_id, lowest_fee_to_node, total_cltv_delta, mut value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat, path_length_to_node, .. }) = targets.pop() {
+               'path_construction: while let Some(RouteGraphNode { node_id, total_cltv_delta, mut value_contribution_msat, path_length_to_node, .. }) = targets.pop() {
 
                        // Since we're going payee-to-payer, hitting our node as a target means we should stop
                        // traversing the graph and arrange the path out of what we found.
@@ -2662,8 +2669,8 @@ where L::Target: Logger {
                        match network_nodes.get(&node_id) {
                                None => {},
                                Some(node) => {
-                                       add_entries_to_cheapest_to_target_node!(node, node_id, lowest_fee_to_node,
-                                               value_contribution_msat, path_htlc_minimum_msat, path_penalty_msat,
+                                       add_entries_to_cheapest_to_target_node!(node, node_id,
+                                               value_contribution_msat,
                                                total_cltv_delta, path_length_to_node);
                                },
                        }