From cfa632b0e94587dc41e6b4be143e7a8abdc09c04 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Wed, 18 Jan 2023 15:34:32 +1030 Subject: [PATCH] lightningd: use hash map for peers instead of linked list. After connecting 100,000 peers with one channel each (not all at once!), we see various places where we exhibit O(N^2) behaviour. Fix these by keeping a map of id->peer instead of a simple linked-list. Signed-off-by: Rusty Russell --- lightningd/channel.c | 20 ++++- lightningd/channel_control.c | 15 ++-- lightningd/coin_mvts.c | 5 +- lightningd/lightningd.c | 29 +++++--- lightningd/lightningd.h | 5 +- lightningd/memdump.c | 1 + lightningd/peer_control.c | 82 +++++++++++++++------ lightningd/peer_control.h | 21 +++++- lightningd/test/run-invoice-select-inchan.c | 5 +- wallet/test/run-wallet.c | 6 +- wallet/walletrpc.c | 5 +- 11 files changed, 143 insertions(+), 51 deletions(-) diff --git a/lightningd/channel.c b/lightningd/channel.c index 9faec5330..ec697518d 100644 --- a/lightningd/channel.c +++ b/lightningd/channel.c @@ -607,7 +607,12 @@ struct channel *any_channel_by_scid(struct lightningd *ld, { struct peer *p; struct channel *chan; - list_for_each(&ld->peers, p, list) { + struct peer_node_id_map_iter it; + + /* FIXME: Support lookup by scid directly! */ + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { list_for_each(&p->channels, chan, list) { /* BOLT-channel-type #2: * - MUST always recognize the `alias` as a @@ -640,7 +645,12 @@ struct channel *channel_by_dbid(struct lightningd *ld, const u64 dbid) { struct peer *p; struct channel *chan; - list_for_each(&ld->peers, p, list) { + struct peer_node_id_map_iter it; + + /* FIXME: Support lookup by id directly! */ + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { list_for_each(&p->channels, chan, list) { if (chan->dbid == dbid) return chan; @@ -654,8 +664,12 @@ struct channel *channel_by_cid(struct lightningd *ld, { struct peer *p; struct channel *channel; + struct peer_node_id_map_iter it; - list_for_each(&ld->peers, p, list) { + /* FIXME: Support lookup by cid directly! */ + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { if (p->uncommitted_channel) { /* We can't use this method for old, uncommitted * channels; there's no "channel" struct here! */ diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index e9f545723..7bef2284f 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -113,10 +113,11 @@ static void try_update_blockheight(struct lightningd *ld, void notify_feerate_change(struct lightningd *ld) { struct peer *peer; + struct peer_node_id_map_iter it; - /* FIXME: We should notify onchaind about NORMAL fee change in case - * it's going to generate more txs. */ - list_for_each(&ld->peers, peer, list) { + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { struct channel *channel; list_for_each(&peer->channels, channel, list) @@ -932,9 +933,13 @@ void channel_notify_new_block(struct lightningd *ld, struct channel *channel; struct channel **to_forget = tal_arr(NULL, struct channel *, 0); size_t i; + struct peer_node_id_map_iter it; - list_for_each (&ld->peers, peer, list) { - list_for_each (&peer->channels, channel, list) { + /* FIXME: keep separate block-aware channel structure instead? */ + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { + list_for_each(&peer->channels, channel, list) { if (channel_unsaved(channel)) continue; if (is_fundee_should_forget(ld, channel, block_height)) { diff --git a/lightningd/coin_mvts.c b/lightningd/coin_mvts.c index cdba2af1d..52445986a 100644 --- a/lightningd/coin_mvts.c +++ b/lightningd/coin_mvts.c @@ -94,6 +94,7 @@ void send_account_balance_snapshot(struct lightningd *ld, u32 blockheight) struct utxo **utxos; struct channel *chan; struct peer *p; + struct peer_node_id_map_iter it; /* Available + reserved utxos are A+, as reserved things have not yet * been spent */ enum output_status utxo_states[] = {OUTPUT_STATE_AVAILABLE, @@ -125,7 +126,9 @@ void send_account_balance_snapshot(struct lightningd *ld, u32 blockheight) snap->accts[0] = bal; /* Add channel balances */ - list_for_each(&ld->peers, p, list) { + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { list_for_each(&p->channels, chan, list) { if (report_chan_balance(chan)) { bal = tal(snap, struct account_balance); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index af02b99b1..1c12f922f 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -70,7 +70,6 @@ #include #include #include -#include #include #include #include @@ -139,7 +138,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->dev_no_ping_timer = false; #endif - /*~ These are CCAN lists: an embedded double-linked list. It's not + /*~ This is a CCAN list: an embedded double-linked list. It's not * really typesafe, but relies on convention to access the contents. * It's inspired by the closely-related Linux kernel list.h. * @@ -153,7 +152,6 @@ static struct lightningd *new_lightningd(const tal_t *ctx) * * This method of manually declaring the list hooks avoids dynamic * allocations to put things into a list. */ - list_head_init(&ld->peers); list_head_init(&ld->subds); /*~ These are hash tables of incoming and outgoing HTLCs (contracts), @@ -179,6 +177,11 @@ static struct lightningd *new_lightningd(const tal_t *ctx) ld->htlcs_out = tal(ld, struct htlc_out_map); htlc_out_map_init(ld->htlcs_out); + /*~ This is the hash table of peers: converted from a + * linked-list as part of the 100k-peers project! */ + ld->peers = tal(ld, struct peer_node_id_map); + peer_node_id_map_init(ld->peers); + /*~ For multi-part payments, we need to keep some incoming payments * in limbo until we get all the parts, or we time them out. */ ld->htlc_sets = tal(ld, struct htlc_set_map); @@ -533,6 +536,7 @@ static const char *find_daemon_dir(struct lightningd *ld, const char *argv0) static void free_all_channels(struct lightningd *ld) { struct peer *p; + struct peer_node_id_map_iter it; /*~ tal supports *destructors* using `tal_add_destructor()`; the most * common use is for an object to delete itself from a linked list @@ -549,13 +553,18 @@ static void free_all_channels(struct lightningd *ld) /*~ For every peer, we free every channel. On allocation the peer was * given a destructor (`destroy_peer`) which removes itself from the - * list. Thus we use list_top() not list_pop() here. */ - while ((p = list_top(&ld->peers, struct peer, list)) != NULL) { + * hashtable. + * + * Deletion from a hashtable is allowed, but it does mean we could + * skip entries in iteration. Hence we repeat until empty! + */ +again: + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { struct channel *c; - /*~ A peer can have multiple channels; we only allow one to be - * open at any time, but we remember old ones for 100 blocks, - * after all the outputs we care about are spent. */ + /*~ A peer can have multiple channels. */ while ((c = list_top(&p->channels, struct channel, list)) != NULL) { /* Removes itself from list as we free it */ @@ -571,9 +580,11 @@ static void free_all_channels(struct lightningd *ld) p->uncommitted_channel = NULL; tal_free(uc); } - /* Removes itself from list as we free it */ + /* Removes itself from htable as we free it */ tal_free(p); } + if (peer_node_id_map_first(ld->peers, &it)) + goto again; /*~ Commit the transaction. Note that the db is actually * single-threaded, so commits never fail and we don't need diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index 4e3689682..a7eaad336 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -3,6 +3,7 @@ #include "config.h" #include #include +#include #include #include #include @@ -178,8 +179,8 @@ struct lightningd { /* Daemon looking after peers during init / before channel. */ struct subd *connectd; - /* All peers we're tracking. */ - struct list_head peers; + /* All peers we're tracking (by node_id) */ + struct peer_node_id_map *peers; /* Outstanding connect commands. */ struct list_head connects; diff --git a/lightningd/memdump.c b/lightningd/memdump.c index b499c580a..1a14c349d 100644 --- a/lightningd/memdump.c +++ b/lightningd/memdump.c @@ -155,6 +155,7 @@ static void finish_report(const struct leak_detect *leaks) memleak_scan_htable(memtable, &ld->htlcs_in->raw); memleak_scan_htable(memtable, &ld->htlcs_out->raw); memleak_scan_htable(memtable, &ld->htlc_sets->raw); + memleak_scan_htable(memtable, &ld->peers->raw); /* Now delete ld and those which it has pointers to. */ memleak_scan_obj(memtable, ld); diff --git a/lightningd/peer_control.c b/lightningd/peer_control.c index dee6f83ca..24123ec26 100644 --- a/lightningd/peer_control.c +++ b/lightningd/peer_control.c @@ -71,7 +71,7 @@ static void destroy_peer(struct peer *peer) { - list_del_from(&peer->ld->peers, &peer->list); + peer_node_id_map_del(peer->ld->peers, peer); } static void peer_update_features(struct peer *peer, @@ -106,7 +106,7 @@ struct peer *new_peer(struct lightningd *ld, u64 dbid, peer->ignore_htlcs = false; #endif - list_add_tail(&ld->peers, &peer->list); + peer_node_id_map_add(ld->peers, peer); tal_add_destructor(peer, destroy_peer); return peer; } @@ -178,21 +178,21 @@ static void peer_channels_cleanup(struct lightningd *ld, struct peer *find_peer_by_dbid(struct lightningd *ld, u64 dbid) { struct peer *p; + struct peer_node_id_map_iter it; - list_for_each(&ld->peers, p, list) + /* FIXME: Support lookup by dbid directly! */ + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { if (p->dbid == dbid) return p; + } return NULL; } struct peer *peer_by_id(struct lightningd *ld, const struct node_id *id) { - struct peer *p; - - list_for_each(&ld->peers, p, list) - if (node_id_eq(&p->id, id)) - return p; - return NULL; + return peer_node_id_map_get(ld->peers, id); } struct peer *peer_from_json(struct lightningd *ld, @@ -333,8 +333,11 @@ void resend_closing_transactions(struct lightningd *ld) { struct peer *peer; struct channel *channel; + struct peer_node_id_map_iter it; - list_for_each(&ld->peers, peer, list) { + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { list_for_each(&peer->channels, channel, list) { if (channel->state == CLOSINGD_COMPLETE) drop_to_chain(ld, channel, true); @@ -1982,8 +1985,13 @@ static struct command_result *json_listpeers(struct command *cmd, if (peer) json_add_peer(cmd->ld, response, peer, ll); } else { - list_for_each(&cmd->ld->peers, peer, list) + struct peer_node_id_map_iter it; + + for (peer = peer_node_id_map_first(cmd->ld->peers, &it); + peer; + peer = peer_node_id_map_next(cmd->ld->peers, &it)) { json_add_peer(cmd->ld, response, peer, ll); + } } json_array_end(response); @@ -2021,20 +2029,23 @@ static struct command_result *json_staticbackup(struct command *cmd, struct json_stream *response; struct peer *peer; struct channel *channel; + struct peer_node_id_map_iter it; if (!param(cmd, buffer, params, NULL)) - return command_param_failed(); + return command_param_failed(); response = json_stream_success(cmd); json_array_start(response, "scb"); - - list_for_each(&cmd->ld->peers, peer, list) + for (peer = peer_node_id_map_first(cmd->ld->peers, &it); + peer; + peer = peer_node_id_map_next(cmd->ld->peers, &it)) { list_for_each(&peer->channels, channel, list){ if (!channel->scb) continue; json_add_scb(cmd, NULL, response, channel); } + } json_array_end(response); return command_success(cmd, response); @@ -2087,8 +2098,13 @@ static struct command_result *json_listpeerchannels(struct command *cmd, if (peer) json_add_peerchannels(cmd->ld, response, peer); } else { - list_for_each(&cmd->ld->peers, peer, list) + struct peer_node_id_map_iter it; + + for (peer = peer_node_id_map_first(cmd->ld->peers, &it); + peer; + peer = peer_node_id_map_next(cmd->ld->peers, &it)) { json_add_peerchannels(cmd->ld, response, peer); + } } json_array_end(response); @@ -2115,7 +2131,11 @@ command_find_channel(struct command *cmd, struct peer *peer; if (json_tok_channel_id(buffer, tok, &cid)) { - list_for_each(&ld->peers, peer, list) { + struct peer_node_id_map_iter it; + + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { list_for_each(&peer->channels, (*channel), list) { if (!channel_active(*channel)) continue; @@ -2189,8 +2209,11 @@ void setup_peers(struct lightningd *ld) struct peer *p; /* Avoid thundering herd: after first five, delay by 1 second. */ int delay = -5; + struct peer_node_id_map_iter it; - list_for_each(&ld->peers, p, list) { + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { setup_peer(p, delay > 0 ? delay : 0); delay++; } @@ -2201,13 +2224,16 @@ struct htlc_in_map *load_channels_from_wallet(struct lightningd *ld) { struct peer *peer; struct htlc_in_map *unconnected_htlcs_in = tal(ld, struct htlc_in_map); + struct peer_node_id_map_iter it; /* Load channels from database */ if (!wallet_init_channels(ld->wallet)) fatal("Could not load channels from the database"); /* First we load the incoming htlcs */ - list_for_each(&ld->peers, peer, list) { + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { struct channel *channel; list_for_each(&peer->channels, channel, list) { @@ -2223,7 +2249,9 @@ struct htlc_in_map *load_channels_from_wallet(struct lightningd *ld) htlc_in_map_copy(unconnected_htlcs_in, ld->htlcs_in); /* Now we load the outgoing HTLCs, so we can connect them. */ - list_for_each(&ld->peers, peer, list) { + for (peer = peer_node_id_map_first(ld->peers, &it); + peer; + peer = peer_node_id_map_next(ld->peers, &it)) { struct channel *channel; list_for_each(&peer->channels, channel, list) { @@ -2310,6 +2338,7 @@ static struct command_result *json_getinfo(struct command *cmd, unsigned int pending_channels = 0, active_channels = 0, inactive_channels = 0, num_peers = 0; size_t count_announceable; + struct peer_node_id_map_iter it; if (!param(cmd, buffer, params, NULL)) return command_param_failed(); @@ -2320,7 +2349,9 @@ static struct command_result *json_getinfo(struct command *cmd, json_add_hex_talarr(response, "color", cmd->ld->rgb); /* Add some peer and channel stats */ - list_for_each(&cmd->ld->peers, peer, list) { + for (peer = peer_node_id_map_first(cmd->ld->peers, &it); + peer; + peer = peer_node_id_map_next(cmd->ld->peers, &it)) { num_peers++; list_for_each(&peer->channels, channel, list) { @@ -2720,7 +2751,11 @@ static struct command_result *json_setchannel(struct command *cmd, /* If the users requested 'all' channels we need to iterate */ if (channels == NULL) { - list_for_each(&cmd->ld->peers, peer, list) { + struct peer_node_id_map_iter it; + + for (peer = peer_node_id_map_first(cmd->ld->peers, &it); + peer; + peer = peer_node_id_map_next(cmd->ld->peers, &it)) { struct channel *channel; list_for_each(&peer->channels, channel, list) { if (channel->state != CHANNELD_NORMAL && @@ -3095,8 +3130,11 @@ static void dualopend_memleak_req_done(struct subd *dualopend, void peer_dev_memleak(struct lightningd *ld, struct leak_detect *leaks) { struct peer *p; + struct peer_node_id_map_iter it; - list_for_each(&ld->peers, p, list) { + for (p = peer_node_id_map_first(ld->peers, &it); + p; + p = peer_node_id_map_next(ld->peers, &it)) { struct channel *c; if (p->uncommitted_channel && p->uncommitted_channel->open_daemon) { struct subd *openingd = p->uncommitted_channel->open_daemon; diff --git a/lightningd/peer_control.h b/lightningd/peer_control.h index 5bdcef569..c5f11bbee 100644 --- a/lightningd/peer_control.h +++ b/lightningd/peer_control.h @@ -15,10 +15,7 @@ struct peer_fd; struct wally_psbt; struct peer { - /* Inside ld->peers. */ - struct list_node list; - - /* Master context */ + /* Master context (we're in the hashtable ld->peers) */ struct lightningd *ld; /* Database ID of the peer */ @@ -143,4 +140,20 @@ command_find_channel(struct command *cmd, /* Ancient (0.7.0 and before) releases could create invalid commitment txs! */ bool invalid_last_tx(const struct bitcoin_tx *tx); +static const struct node_id *peer_node_id(const struct peer *peer) +{ + return &peer->id; +} + +static bool peer_node_id_eq(const struct peer *peer, + const struct node_id *node_id) +{ + return node_id_eq(&peer->id, node_id); +} + +/* Defines struct peer_node_id_map */ +HTABLE_DEFINE_TYPE(struct peer, + peer_node_id, node_id_hash, peer_node_id_eq, + peer_node_id_map); + #endif /* LIGHTNING_LIGHTNINGD_PEER_CONTROL_H */ diff --git a/lightningd/test/run-invoice-select-inchan.c b/lightningd/test/run-invoice-select-inchan.c index 0d3dfa82c..b49cbc502 100644 --- a/lightningd/test/run-invoice-select-inchan.c +++ b/lightningd/test/run-invoice-select-inchan.c @@ -999,7 +999,7 @@ static struct channel *add_peer(struct lightningd *ld, int n, memset(&peer->id, n, sizeof(peer->id)); list_head_init(&peer->channels); - list_add_tail(&ld->peers, &peer->list); + peer_node_id_map_add(ld->peers, peer); peer->ld = ld; c->state = state; @@ -1036,7 +1036,8 @@ int main(int argc, char *argv[]) common_setup(argv[0]); ld = tal(tmpctx, struct lightningd); - list_head_init(&ld->peers); + ld->peers = tal(ld, struct peer_node_id_map); + peer_node_id_map_init(ld->peers); ld->htlcs_in = tal(ld, struct htlc_in_map); htlc_in_map_init(ld->htlcs_in); chainparams = chainparams_for_network("regtest"); diff --git a/wallet/test/run-wallet.c b/wallet/test/run-wallet.c index 4421c59ae..2113ba84f 100644 --- a/wallet/test/run-wallet.c +++ b/wallet/test/run-wallet.c @@ -1370,11 +1370,12 @@ static struct channel *wallet_channel_load(struct wallet *w, const u64 dbid) { struct peer *peer; struct channel *channel; + struct peer_node_id_map_iter it; /* We expect only one peer, but reuse same code */ if (!wallet_init_channels(w)) return NULL; - peer = list_top(&w->ld->peers, struct peer, list); + peer = peer_node_id_map_first(w->ld->peers, &it); CHECK(peer); /* We load lots of identical dbid channels: use last one */ @@ -1931,7 +1932,8 @@ int main(int argc, const char *argv[]) ld->config = test_config; /* Only elements in ld we should access */ - list_head_init(&ld->peers); + ld->peers = tal(ld, struct peer_node_id_map); + peer_node_id_map_init(ld->peers); ld->rr_counter = 0; node_id_from_hexstr("02a1633cafcc01ebfb6d78e39f687a1f0995c62fc95f51ead10a02ee0be551b5dc", 66, &ld->id); /* Accessed in peer destructor sanity check */ diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index f02d380ec..4292781ac 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -313,6 +313,7 @@ static struct command_result *json_listfunds(struct command *cmd, { struct json_stream *response; struct peer *p; + struct peer_node_id_map_iter it; struct utxo **utxos, **reserved_utxos, **spent_utxos; bool *spent; @@ -339,7 +340,9 @@ static struct command_result *json_listfunds(struct command *cmd, /* Add funds that are allocated to channels */ json_array_start(response, "channels"); - list_for_each(&cmd->ld->peers, p, list) { + for (p = peer_node_id_map_first(cmd->ld->peers, &it); + p; + p = peer_node_id_map_next(cmd->ld->peers, &it)) { struct channel *c; list_for_each(&p->channels, c, list) { /* We don't print out uncommitted channels */