libplugin-pay: use map for channel hints #7636
Conversation
JssDWt
force-pushed
the
jssdwt-improve-pay-performance
branch
from
2c0a1c5 to
f23b2d9
Compare
JssDWt
force-pushed
the
jssdwt-improve-pay-performance
branch
from
f23b2d9 to
b333c66
Compare
plugins/libplugin-pay.c
Outdated
| if (!short_channel_id_eq(hints[j].scid.scid, r->short_channel_id)) | ||
| struct channel_hint *hint; | ||
| struct channel_hint_map_iter iter; | ||
| for (hint = channel_hint_map_first(hints, &iter); |
There was a problem hiding this comment.
Why loop over the hash map here? Couldn't we use lookup by hash?
There was a problem hiding this comment.
The channel_hint_map is indexed by scid and direction, while here we only have the scid. An alternative could be to make a channel hint map based on scid only, with a struct with 2 fields for the different directions. Then we wouldn't have to iterate here.
This part of the code is used once per path though, and not with every step of the dijkstra procedure. The things fixed in this PR were called approximately 35000 times per payment, while this one is called once per payment. So the performance overhead is a lot less.
There was a problem hiding this comment.
Actually it seems like we should be able to get the hint by getting from the channel hints map with 0 or 1 direction. I'll dig which one to use here. It seems weird that the direction wasn't used before, you could have gotten the route hint in the wrong direction and assumed the routehint was excluded 🤔
There was a problem hiding this comment.
Once per payment is not bad, I have no issues with leaving the loop here.
But the direction of the hint (the r) is already defined by the ordering of the nodes on both ends of the channel.
I think the previous code was lazy to check only the short channel id, maybe considering that we usually get information
about channels in one direction and rarely we get the other direction. IDK
There was a problem hiding this comment.
@Lagrang3 I added a commit that attempts to get the route hint from the map directly. Can you check that out? Especially the pubkeys used for the comparison there.
This commit fixes a "FIXME: This is slow" in the pay plugin. For nodes with many channels this is a tremendous improvement in pay performance. PR ElementsProject#7611 improves payment performance from 15 seconds to 13.5 seconds on one of our nodes. This commit improves payment performance from 13.5 seconds to 5.7 seconds. Changelog-Fixed: Improved pathfinding speed for large nodes.
JssDWt
force-pushed
the
jssdwt-improve-pay-performance
branch
from
b333c66 to
94babc2
Compare
|
BTW this 2x improvement in runtime is great! Do you know how big was this array of hints typically? I thought these hints were gathered on a per payment basis and hence there wasn't supposed to be many of them. |
Thing is your own private channels are part of the hints. Being a LSP, the node has many private channels. In this case close to 2000. |
| scidd.dir = node_id_cmp(&routehint->pubkey, | ||
| p->pay_destination) > 0 ? 1 : 0; |
There was a problem hiding this comment.
I think something like this should do the trick.
| scidd.dir = node_id_cmp(&routehint->pubkey, | |
| p->pay_destination) > 0 ? 1 : 0; | |
| scidd.dir = node_id_idx(&r->pubkey, p->pay_destination); |
Also notice that I replaced routehint with r.
|
Replaced by #7726 |
libplugin-pay: use map for channel hints
Description
This commit fixes a "FIXME: This is slow" in the pay plugin. For nodes with many channels this is a tremendous improvement in pay performance. PR #7611 improves payment performance from 15 seconds to 13.5 seconds on one of our nodes. This commit improves payment performance from 13.5 seconds to 5.7 seconds.
Changes Made
Checklist
Ensure the following tasks are completed before submitting the PR:
TODOshave been addressed or removed.