From d1f43d993a90f7b6e67aef516df91a281ebb3628 Mon Sep 17 00:00:00 2001
From: Rusty Russell <rusty@rustcorp.com.au>
Date: Tue, 21 May 2019 16:43:28 +0930
Subject: [PATCH] gossipd: use explicit destructor for struct chan.

Each destructor2 costs 40 bytes, and struct chan is only 120 bytes.  So
this drops our memory usage quite a bit:

MCP bench results change:
   -vsz_kb:580004-580016(580006+/-4.8)
   +vsz_kb:533148

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 gossipd/gossip_store.c                 |  2 +-
 gossipd/gossipd.c                      |  4 ++--
 gossipd/routing.c                      | 33 ++++++++++++++++----------
 gossipd/routing.h                      |  3 +++
 gossipd/test/run-bench-find_route.c    | 10 +++++++-
 gossipd/test/run-find_route-specific.c |  7 ++++++
 gossipd/test/run-find_route.c          |  7 ++++++
 gossipd/test/run-overlong.c            |  7 ++++++
 8 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/gossipd/gossip_store.c b/gossipd/gossip_store.c
index 16da5b339..1d71bca9d 100644
--- a/gossipd/gossip_store.c
+++ b/gossipd/gossip_store.c
@@ -604,7 +604,7 @@ void gossip_store_load(struct routing_state *rstate, struct gossip_store *gs)
 				bad = "Bad channel_delete scid";
 				goto truncate;
 			}
-			tal_free(c);
+			free_chan(rstate, c);
 			stats[3]++;
 			break;
 		case WIRE_GOSSIPD_LOCAL_ADD_CHANNEL:
diff --git a/gossipd/gossipd.c b/gossipd/gossipd.c
index 9461be5ca..be0a9dc7a 100644
--- a/gossipd/gossipd.c
+++ b/gossipd/gossipd.c
@@ -2806,9 +2806,9 @@ static struct io_plan *handle_outpoint_spent(struct io_conn *conn,
 		    "spent",
 		    type_to_string(msg, struct short_channel_id, &scid));
 		/* Freeing is sufficient since everything else is allocated off
-		 * of the channel and the destructor takes care of unregistering
+		 * of the channel and this takes care of unregistering
 		 * the channel */
-		tal_free(chan);
+		free_chan(rstate, chan);
 		/* We put a tombstone marker in the channel store, so we don't
 		 * have to replay blockchain spends on restart. */
 		gossip_store_add_channel_delete(rstate->broadcasts->gs, &scid);
diff --git a/gossipd/routing.c b/gossipd/routing.c
index 62e2b8c6f..f67d82f0a 100644
--- a/gossipd/routing.c
+++ b/gossipd/routing.c
@@ -220,7 +220,7 @@ static void destroy_node(struct node *node, struct routing_state *rstate)
 
 	/* These remove themselves from chans[]. */
 	while ((c = first_chan(node, &i)) != NULL)
-		tal_free(c);
+		free_chan(rstate, c);
 
 	/* Free htable if we need. */
 	if (node_uses_chan_map(node))
@@ -344,7 +344,9 @@ static void remove_chan_from_node(struct routing_state *rstate,
 	}
 }
 
-static void destroy_chan(struct chan *chan, struct routing_state *rstate)
+/* We used to make this a tal_add_destructor2, but that costs 40 bytes per
+ * chan, and we only ever explicitly free it anyway. */
+void free_chan(struct routing_state *rstate, struct chan *chan)
 {
 	remove_chan_from_node(rstate, chan->nodes[0], chan);
 	remove_chan_from_node(rstate, chan->nodes[1], chan);
@@ -358,6 +360,7 @@ static void destroy_chan(struct chan *chan, struct routing_state *rstate)
 
 	/* Remove from local_disabled_map if it's there. */
 	chan_map_del(&rstate->local_disabled_map, chan);
+	tal_free(chan);
 }
 
 static void init_half_chan(struct routing_state *rstate,
@@ -416,8 +419,6 @@ struct chan *new_chan(struct routing_state *rstate,
 	init_half_chan(rstate, chan, !n1idx);
 
 	uintmap_add(&rstate->chanmap, scid->u64, chan);
-
-	tal_add_destructor2(chan, destroy_chan, rstate);
 	return chan;
 }
 
@@ -1395,7 +1396,8 @@ bool routing_add_channel_announcement(struct routing_state *rstate,
 	}
 
 	/* Pretend it didn't exist, for the moment. */
-	tal_free(chan);
+	if (chan)
+		free_chan(rstate, chan);
 
 	uc = tal(rstate, struct unupdated_channel);
 	uc->channel_announce = tal_dup_arr(uc, u8, msg, tal_count(msg), 0);
@@ -2286,6 +2288,8 @@ void routing_failure(struct routing_state *rstate,
 		     enum onion_type failcode,
 		     const u8 *channel_update)
 {
+	struct chan **pruned = tal_arr(tmpctx, struct chan *, 0);
+
 	status_trace("Received routing failure 0x%04x (%s), "
 		     "erring node %s, "
 		     "channel %s/%u",
@@ -2331,7 +2335,7 @@ void routing_failure(struct routing_state *rstate,
 						    &node->id));
 			for (c = first_chan(node, &i); c; c = next_chan(node, &i)) {
 				/* Set it up to be pruned. */
-				tal_steal(tmpctx, c);
+				tal_arr_expand(&pruned, c);
 			}
 		}
 	} else {
@@ -2356,9 +2360,13 @@ void routing_failure(struct routing_state *rstate,
 						    struct short_channel_id,
 						    scid));
 			/* Set it up to be deleted. */
-			tal_steal(tmpctx, chan);
+			tal_arr_expand(&pruned, chan);
 		}
 	}
+
+	/* Now free all the chans and maybe even nodes. */
+	for (size_t i = 0; i < tal_count(pruned); i++)
+		free_chan(rstate, pruned[i]);
 }
 
 
@@ -2367,7 +2375,7 @@ void route_prune(struct routing_state *rstate)
 	u64 now = gossip_time_now(rstate).ts.tv_sec;
 	/* Anything below this highwater mark ought to be pruned */
 	const s64 highwater = now - rstate->prune_timeout;
-	const tal_t *pruned = tal(NULL, char);
+	struct chan **pruned = tal_arr(tmpctx, struct chan *, 0);
 	struct chan *chan;
 	struct unupdated_channel *uc;
 	u64 idx;
@@ -2394,7 +2402,7 @@ void route_prune(struct routing_state *rstate)
 			    : now - chan->half[1].bcast.timestamp);
 
 			/* This may perturb iteration so do outside loop. */
-			tal_steal(pruned, chan);
+			tal_arr_expand(&pruned, chan);
 		}
 	}
 
@@ -2403,12 +2411,13 @@ void route_prune(struct routing_state *rstate)
 	     uc;
 	     uc = uintmap_after(&rstate->unupdated_chanmap, &idx)) {
 		if (uc->added.ts.tv_sec < highwater) {
-			tal_steal(pruned, uc);
+			tal_arr_expand(&pruned, chan);
 		}
 	}
 
-	/* This frees all the chans and maybe even nodes. */
-	tal_free(pruned);
+	/* Now free all the chans and maybe even nodes. */
+	for (size_t i = 0; i < tal_count(pruned); i++)
+		free_chan(rstate, pruned[i]);
 }
 
 #if DEVELOPER
diff --git a/gossipd/routing.h b/gossipd/routing.h
index f1b68a3bd..802c55ae3 100644
--- a/gossipd/routing.h
+++ b/gossipd/routing.h
@@ -54,6 +54,9 @@ struct chan {
 	struct amount_sat sat;
 };
 
+/* Use this instead of tal_free(chan)! */
+void free_chan(struct routing_state *rstate, struct chan *chan);
+
 /* A local channel can exist which isn't announced; normal channels are only
  * created once we have both an announcement *and* an update. */
 static inline bool is_chan_public(const struct chan *chan)
diff --git a/gossipd/test/run-bench-find_route.c b/gossipd/test/run-bench-find_route.c
index 1e49dcf60..fc61a3c62 100644
--- a/gossipd/test/run-bench-find_route.c
+++ b/gossipd/test/run-bench-find_route.c
@@ -131,9 +131,10 @@ static void add_connection(struct routing_state *rstate,
 	memcpy((char *)&scid + (!idx) * sizeof(to), &to, sizeof(to));
 
 	chan = get_channel(rstate, &scid);
-	if (!chan)
+	if (!chan) {
 		chan = new_chan(rstate, &scid, &nodes[from], &nodes[to],
 				AMOUNT_SAT(1000000));
+	}
 
 	c = &chan->half[idx];
 	c->base_fee = base_fee;
@@ -277,6 +278,13 @@ int main(int argc, char *argv[])
 		if (route_lengths[i])
 			printf(" Length %zu: %zu\n", i, route_lengths[i]);
 
+	/* Since we omitted destructors on these, clean up manually */
+	u64 idx;
+	for (struct chan *chan = uintmap_first(&rstate->chanmap, &idx);
+	     chan;
+	     chan = uintmap_after(&rstate->chanmap, &idx))
+		free_chan(rstate, chan);
+
 	tal_free(tmpctx);
 	secp256k1_context_destroy(secp256k1_ctx);
 	opt_free_table();
diff --git a/gossipd/test/run-find_route-specific.c b/gossipd/test/run-find_route-specific.c
index 0a934894d..3c2dc90b9 100644
--- a/gossipd/test/run-find_route-specific.c
+++ b/gossipd/test/run-find_route-specific.c
@@ -261,6 +261,13 @@ int main(void)
 			   ROUTING_MAX_HOPS, &fee);
 	assert(!route);
 
+	/* Since we omitted destructors on these, clean up manually */
+	u64 idx;
+	for (struct chan *chan = uintmap_first(&rstate->chanmap, &idx);
+	     chan;
+	     chan = uintmap_after(&rstate->chanmap, &idx))
+		free_chan(rstate, chan);
+
 	tal_free(tmpctx);
 	secp256k1_context_destroy(secp256k1_ctx);
 	return 0;
diff --git a/gossipd/test/run-find_route.c b/gossipd/test/run-find_route.c
index 9cca0f9c7..4640bf4fa 100644
--- a/gossipd/test/run-find_route.c
+++ b/gossipd/test/run-find_route.c
@@ -278,6 +278,13 @@ int main(void)
 	assert(channel_is_between(route[1], &d, &c));
 	assert(amount_msat_eq(fee, AMOUNT_MSAT(0 + 6)));
 
+	/* Since we omitted destructors on these, clean up manually */
+	u64 idx;
+	for (struct chan *chan = uintmap_first(&rstate->chanmap, &idx);
+	     chan;
+	     chan = uintmap_after(&rstate->chanmap, &idx))
+		free_chan(rstate, chan);
+
 	tal_free(tmpctx);
 	secp256k1_context_destroy(secp256k1_ctx);
 	return 0;
diff --git a/gossipd/test/run-overlong.c b/gossipd/test/run-overlong.c
index c59c19e9f..fe0e5fadf 100644
--- a/gossipd/test/run-overlong.c
+++ b/gossipd/test/run-overlong.c
@@ -197,6 +197,13 @@ int main(void)
 		last_fee = fee;
 	}
 
+	/* Since we omitted destructors on these, clean up manually */
+	u64 idx;
+	for (struct chan *chan = uintmap_first(&rstate->chanmap, &idx);
+	     chan;
+	     chan = uintmap_after(&rstate->chanmap, &idx))
+		free_chan(rstate, chan);
+
 	tal_free(tmpctx);
 	secp256k1_context_destroy(secp256k1_ctx);
 	return 0;