Skip to content

Swap NetworkGraph PublicKeys for [u8; 33] #960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
TheBlueMatt opened this issue Jun 18, 2021 · 7 comments · Fixed by #1107
Closed

Swap NetworkGraph PublicKeys for [u8; 33] #960

TheBlueMatt opened this issue Jun 18, 2021 · 7 comments · Fixed by #1107
Labels
good first issue Good for newcomers

Comments

@TheBlueMatt
Copy link
Collaborator

Deserializing the network graph takes quite a while, of which my initial testing shows 80+% of the time is spent deserializing the publickeys from compressed 33-byte form into secp 64-byte form (and presumably verifying they are on the curve). There is relatively little reason to be verifying all the public keys, let alone using them in 64 byte format everywhere in the network graph in memory. We should instead simply use [u8; 33] for them, checking their correctness only after we've built a route with them and need to construct a Route object. Note that I'd think this may also represent a material speedup for route finding as pubkey comparison can be inlined instead of needing to call out to C code due to lack of cross-language LTO in most builds.

@TheBlueMatt TheBlueMatt added the good first issue Good for newcomers label Jul 16, 2021
@dunxen
Copy link
Contributor

dunxen commented Aug 8, 2021

I'm going to take a look at this one. Will reach out for help / guidance.

@dunxen
Copy link
Contributor

dunxen commented Aug 9, 2021

I've migrated the NetworkGraph side of things mostly. Just busy with the router stuff. Should have a PR up within the next couple days if I don't get too caught up in work.

@TheBlueMatt
Copy link
Collaborator Author

Glad to hear someone is working on this! If its not too late, it may make sense to base this work off of #1043, as otherwise it may cause some disruptive conflicts.

@dunxen
Copy link
Contributor

dunxen commented Aug 23, 2021

@TheBlueMatt thanks, it's definitely not too late. Had to pause for a bit because of work but getting back to it today 😄

@Kixunil
Copy link
Contributor

Kixunil commented Aug 23, 2021

Randomly going by, perhaps struct NodeId([u8; 33]);?

@dunxen
Copy link
Contributor

dunxen commented Oct 3, 2021

I have this basically complete on my fork. Just making some tweaks and cleaning up before I open the PR.

@TheBlueMatt
Copy link
Collaborator Author

TheBlueMatt commented Oct 8, 2021

Results of #1107 are, roughly (from Github CI):

test ln::channelmanager::bench::bench_sends               ... bench:   8,679,569 ns/iter (+/- 1,160,867)
test routing::network_graph::benches::read_network_graph  ... bench: 2,495,846,755 ns/iter (+/- 145,851,981)
test routing::network_graph::benches::write_network_graph ... bench: 141,748,993 ns/iter (+/- 6,248,343)
test routing::router::benches::generate_mpp_routes        ... bench:  85,042,605 ns/iter (+/- 55,020,220)
test routing::router::benches::generate_routes            ... bench:  81,382,609 ns/iter (+/- 58,966,139)
test bench::bench_sends ... bench: 181,266,525 ns/iter (+/- 153,994,734)

to

test ln::channelmanager::bench::bench_sends               ... bench:   8,300,767 ns/iter (+/- 1,520,325)
test routing::network_graph::benches::read_network_graph  ... bench: 1,501,082,125 ns/iter (+/- 79,991,352)
test routing::network_graph::benches::write_network_graph ... bench: 132,349,099 ns/iter (+/- 8,865,711)
test routing::router::benches::generate_mpp_routes        ... bench:  57,506,312 ns/iter (+/- 38,795,099)
test routing::router::benches::generate_routes            ... bench:  58,337,687 ns/iter (+/- 44,755,523)
test bench::bench_sends ... bench: 200,507,491 ns/iter (+/- 158,389,150)

In other words, a slight regression in sending in a loop (maybe, it may just be actions being weird, most of that time is in I/O so I dunno why it would slow down), but otherwise a pretty nice win for routing and a huge win deserializing the network graph itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants