NetworkGraph: Allow to construct with custom count estimates, update numbers, determine pre-allocation dynamically on read
#4306
Conversation
|
👋 I see @wpaulino was un-assigned. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4306 +/- ##
==========================================
- Coverage 86.59% 86.58% -0.01%
==========================================
Files 158 158
Lines 102408 102382 -26
Branches 102408 102382 -26
==========================================
- Hits 88678 88647 -31
- Misses 11309 11319 +10
+ Partials 2421 2416 -5
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:
|
lightning/src/routing/gossip.rs
Outdated
| /// | ||
| /// 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.
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.
Sure, but do we really expect downstream devs to update their estimates more often than us?
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).
Yea, we should definitely use Network to disable the pre-allocation.
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?
Yea, makes sense. We could still use the constants on mainnet loads, even, to ensure we pre-allocate even if loading with an empty (or partially-synced) graph but I guess it doesn't matter that much either way.
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.
Yea, I mean it would be simpler for LDK Node, sure, but it definitely feels like the wrong way :)
There was a problem hiding this comment.
Alright, will drop the first commit then, and add one that disables the estimates on non-mainnet networks.
There was a problem hiding this comment.
Done, let me know if I can squash.
|
🔔 1st Reminder Hey @wpaulino! This PR has been waiting for your review. |
tnull
force-pushed
the
2026-01-allow-empty-network-graph
branch
from
01e163e to
9138bb2
Compare
…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.
Previously, we'd always pre-allocate memory for the node and channel maps based on mainnet numbers, even if we're on another network like `Regest`. Here, we only apply the estimates if we're actually on `Network::Bitcoin`, which should reduce the `NetworkGraph`'s memory footprint considerably in tests.
tnull
force-pushed
the
2026-01-allow-empty-network-graph
branch
from
9138bb2 to
070002c
Compare
|
Squashed without further changes. |
| const CHAN_COUNT_ESTIMATE: usize = 50_000; | ||
| /// In Jan, 2026 there were about 13K nodes | ||
| /// | ||
| /// We over-allocate by a bit because 15% more is better than the double we get if we're slightly | ||
| /// too low. | ||
| const NODE_COUNT_ESTIMATE: usize = 15_000; |
There was a problem hiding this comment.
Oh, oops, these are actives. My node (restarted less than a week ago, i believe, so hasn't pruned channels where one side has been disabled for a week) shows network_nodes: 17013, network_channels: 54264. Thus, if we use 50k/15k what we'll actually end up with is 100k/30k as the allocation (I assume vec doubles? maybe not for large allocations?)
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
| let channels_map_capacity = (channels_count as u128 * 115 / 100) | ||
| .try_into() | ||
| .map_err(|_| DecodeError::InvalidValue)?; | ||
| if channels_map_capacity > MAX_CHAN_COUNT_LIMIT { |
There was a problem hiding this comment.
Maybe someone is doing a 1M channels test? Let's just limit the pre-allocation rather than failing.
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.