Avoid parsing PublicKeys when handling RGS updates
#3581
Conversation
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
a5810bf to
4df7223
Compare
|
Oh, new commit improves the bench some 25% or so, too. |
|
That said, CI is said as the |
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
4df7223 to
31390b2
Compare
|
Feel free to squash. |
There was a problem hiding this comment.
Overall, LGTM!
Since there aren’t any direct code references explaining why we switch to using NodeId in RGS, it might be helpful to add a quick note in the commit message (of ddc3326) on why skipping PublicKey verification is safe in this case.
I think adding a little extra context could make it clearer for future readers!
`PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Sadly, we were always parsing the broadcasting node's `PublicKey` from the `node_id` in the network graph whenever we see an update for that channel, whether we have a corresponding signature or not. Here we fix this, only parsing the public key (and hashing the message) if we're going to check a signature.
`PublicKey` parsing is relatively expensive as we have to check if the point is actually on the curve. To avoid it, our `NetworkGraph` uses `NodeId`s which don't have the validity requirement. Here, we take advantage of that in RGS application to avoid parsing `PublicKey`s, improving performance.
When we build a new `NetworkGraph` from empty, we're generally doing an initial startup and will be syncing the graph very soon. Using an initially-empty `IndexedMap` for the `channels` and `nodes` results in quite some memory churn, with the initial RGS application benchmark showing 15% of its time in pagefault handling alone (i.e. allocating new memory from the OS, let alone the 23% of time in `memmove`). Further, when deserializing a `NetworkGraph`, we'd swapped the expected node and channel count constants, leaving the node map too small and causing map doubling as we read entries from disk. Finally, when deserializing, allocating only exactly the amount of map entries we need is likely to lead to at least one doubling, so we're better off just over-estimating the number of nodes and channels and allocating what we want. Here we just always allocate `channels` and `nodes` based on constants, leading to a 20%-ish speedup in the initial RGS application benchmark.
TheBlueMatt
force-pushed
the
2025-01-rgs-speedups
branch
from
31390b2 to
0ff7fba
Compare
|
Squashed with one extra commit at the top that just adds more detail to the |
|
Mhh, seems fuzz is failing. Not sure if related, but it blocks merging. |
|
Hmm, its possible this caused performance of some fuzz target to regress, but I think more likely github just decided to run it on a slower machine or so, so gonna disable the fuzz-required and merge. |
|
Backported the first (fixing the perf regression) and third (more correctly reserving the graph structures for a large performance win) commits in #3613 |
The first commit fixes a regression in 0.1 and should be backported, the second is just an optimization but shouldn't be backported as it changes the public API.