NetworkGraph: Allow to construct with custom count estimates, update numbers, determine pre-allocation dynamically on read
#4306
Conversation
Previously, we'd always pre-allocate a lot of memory in `NetworkGraph::new`. While this can be a nice efficiency improvements to avoid unnecessary re-allocations, it can also result in unnecessarily allocating memory. Here, we add a new constructor that allows specifying the node and channel count estimates for pre-allocation.
…eading When reading a persisted network graph, we previously pre-allocated our default node/channels estimate count for the respective `IndexedMap` capacities. However, this might unnecessarily allocate memory on reading, for example if we have an (almost) empty network graph for one reason or another. As we have the actual counts of persisted nodes and channels available, we here simply opt to allocate these numbers (plus 15%). This will also ensure that our pre-allocations will keep up-to-date over time as the network grows or shrinks.
|
👋 I see @wpaulino was un-assigned. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4306 +/- ##
==========================================
+ Coverage 86.59% 86.60% +0.01%
==========================================
Files 158 158
Lines 102408 102417 +9
Branches 102408 102417 +9
==========================================
+ Hits 88678 88697 +19
+ Misses 11309 11303 -6
+ Partials 2421 2417 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// | ||
| /// To improve efficiency, this will pre-allocate memory for `node_count_estimate` nodes and | ||
| /// `channel_count_estimate` channels. | ||
| pub fn from_node_and_channel_count_estimates( |
There was a problem hiding this comment.
I'm pretty skeptical of exposing this. If you don't want a network graph, don't build one. If you want a network graph, we should allocate for a network graph. How is a downstream dev better positioned to provide estimates here than we are?
There was a problem hiding this comment.
Hmm, from my point of view it is a bit odd to pre-allocate a lot of memory based on static estimations that will become stale over time. Plus, we currently make no distinction based on Network here, so we will always allocate that much memory even for small Regtest or Signet environments with only a hand full of nodes (e.g., in tests). If you're skeptical, maybe we can drop the extra constructor, but leave the dynamic allocation on read, and only apply the estimates for Network::Bitcoin?
Of course, I have to admit that I first had planned to use that constructor for the minimal-mode Node over at LDK Node, but you're right, for that application we probably need to completely change up our types there and simply rip out anything related to the Router/NetworkGraph entirely.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
3 participants
Previously, we'd always pre-allocate a lot of memory in
NetworkGraph::new. While this can be a nice efficiency improvements to avoid unnecessary re-allocations, it can also result in unnecessarily allocating memory. Here, we add a new constructor that allows specifying the node and channel count estimates for pre-allocation.Apart from that, we also update the numbers for Jan 2026, and now determine the pre-allocation factors dynamically based on the actual numbers when reading a persisted graph.