In-memory graph cache for faster pathfinding by guggero · Pull Request #5642 · lightningnetwork/lnd

guggero · 2021-08-19T12:22:43Z

Replaces #5631.

This PR adds an in-memory cache for the graph that is used for pathfinding.
This brings down the average time for finding a path from around 3000ms with bbolt on my machine to between 250 and 300ms.
For remote database backends this gain should be even more significant.

TODO:

~~Make sure all itests pass~~
~~Further optimize memory footprint (currently this adds about 60MB of heap)~~
~~Add more unit tests~~
~~Get dependent PR channeldb: write through cache for the graph and channel state #5595 merged.~~

Fixes #3266

bhandras · 2021-08-26T16:44:45Z

Can I now completely remove the graph cache from the parent PR (#5595) @guggero ?

guggero · 2021-08-27T08:08:29Z

Can I now completely remove the graph cache from the parent PR (#5595) @guggero ?

Yes please! Then I can rebase and update fbf52b6.

bhandras · 2021-08-27T16:57:19Z

Can I now completely remove the graph cache from the parent PR (#5595) @guggero ?

Yes please! Then I can rebase and update fbf52b6.

Okay, PTAL, #5595 now has things a bit reordered. Last 3 commits is the cache + integration and the first 4 commits is pure refactors. Should work well and even tests pass with skipping graph buckets.

guggero · 2021-08-30T11:05:17Z

Okay, PTAL, #5595 now has things a bit reordered. Last 3 commits is the cache + integration and the first 4 commits is pure refactors. Should work well and even tests pass with skipping graph buckets.

Thanks, I added the first 4 commits from #5595 to this PR and removed the WIP state 🎉

bhandras

Looking really good! I'm not super familiar with the routing code base but following the changes in the PR was easy and to me apart from a few nits looks almost ready. Great job Oliver!

yyforyongyu · 2021-09-02T10:39:10Z

+
+	// db points to the actual backend holding the channel state database.
+	// This may be a real backend or a cache middleware.
+	db kvdb.Backend


I think we don't need this db as it's already in LinkNodeDB.db?

The idea is to give the LinkNodeDB its own instance so we don't share the same reference necessarily. At the moment it will be the same instance but in the future we might point the LinkNodeDB to its own separate namespace.

Cool. Since LinkNodeDB is embedded here I'd expect to call ChannelStateDB.db to get the LinkNodeDB.db, but then it's overwritten with its own db. Maybe we could choose not to do the embedding here. Non-blocking, just a thought for future changes.

I decided to take things apart even more clearly in the latest state. Let me know what you think.

bhandras

LGTM 🥇 🚀 ⚡

bhandras · 2021-09-02T13:27:24Z

+	startTime := time.Now()
+	log.Debugf("Populating in-memory channel graph, this might take a " +
+		"while...")
+	err := g.ForEachNode(func(tx kvdb.RTx, node *LightningNode) error {


Would be interesting to test this on etcd with the current mainnet graph to see if we encounter any transaction limits. Maybe we'll need a more specific solution without a transaction since this is at startup.

Quick calculation: assuming we have ~15k nodes => 15k * (32 + 33 + 1) raw bytes for keys which is roughly 1 MiB (per key 32 bytes = bucket, 33 bytes = pubkey, 1 byte = value suffix).

Plus for each we also send the last mod revision which is a 8 byte so another 100 KiB. Plus the protocol overhead, so the txn should be less than 2 MiB. Still manageble.

Good idea to test this! I'm going to fix up #5561 this week and then attempt a migration of the mainnet graph data to etcd and test this PR with it.

I wonder if we should build tag this out for mobile clients? So if the mobile build tags are active, we don't load all this in....would be good to get some testing on this front, as worst case maybe this causes some weird OOM stuff for mobile nodes inadvertently.

Yes, but that would completely disable path finding on mobile devices since we currently don't have a fall back version that uses the graph in the database. Or are you saying we should make it possible to enable the DB based fall back and implement it in this PR?

I'm suspicious that way bbolt works at some point we probably have most of the graph in memory when we ForEach over all nodes/edges.

The difference is that w/ bolt, we'll map everything into virtual memory, then as we iterate over the graph, the kernel will automatically swap the necessary pages into resident memory when we access it (page fault).

With this, we'll read everything at the very start (allocating enough resident memory for it all to be held), then over time the unused pages may be swapped back onto disk. In the worst case, everything will need ot be swapped back in if a path finding attempt fails. In the average case, we save a lot though since we cut down on context switching from user <-> kernel space, and also all the locking+copying mechanisms in bolt.

Re the chunky allocation at the very start, we could possibly pre-size the cache itself so the runtime can make one larger allocation instead of a series of smaller ones.

Re the question of if we can add a flag or not here, this was brought up again last week by some users concerned about the memory footprint on mobile phones. At this point, we simply haven't tested that path, but it may not be an issue in practice, we won't know until we either get someone to take this PR for a spin, or spin up a test env ourselves.

orijbot · 2021-09-06T14:30:26Z

Visit https://dashboard.github.orijtech.com?back=0&pr=5642&remote=true&repo=guggero%2Flnd to see benchmark details.

yyforyongyu

Very cool feature. This will also fix some pain points we've encountered in our itests, regarding the deadlocks and etc. Got several questions and a few nits.

yyforyongyu · 2021-09-07T10:11:11Z

nit: missing docs here. I'm still learning the updated structure here. So we have a LinkNodeDB.db and a LinkNode.db, and they might be different?

Yeah bit of a naming collision here...

Not clear why we need this intermediate struct actually, givne we just need to pass in the kvdb.Backend directly?

Just to make it more clear where a future code separation could be done. So everything "owned" by the LinkNodeDB could go into its own SQL table.

yyforyongyu · 2021-09-07T10:18:31Z

unrelated to this PR, so the *LinkNode returned here doesn't have a database backend attached and we could not do operations like *LinkNode.Sync() here right?

Yes, the backend is only used for the Sync() method. Added a comment and refactored it a bit to make it more clear.

yyforyongyu · 2021-09-07T10:37:27Z

nit: all the LinkNode related methods seem to belong to LinkNodeDB.

Not sure what you mean. This change turns the method into a function because no reference to the LinkNodeDB is required here, since we have the transaction instead.

yyforyongyu · 2021-09-07T10:39:56Z

So we are separating the old big DB into more defined structs right?

Yes, that's the idea. To eventually (further down the line) separate them into their own databases and possibly SQL tables.

yyforyongyu · 2021-09-07T11:53:20Z

seems like the graph cache needs a bit more unit tests.

I think one thing we'll want to do minimally is update the prior set of channel DB tests to have pre and post test assertions w.r.t the state of the channel cache. This should help to catch any instance we may have missed re cache consistency.

Will start with those additional assertions now.

Roasbeef

Excellent PR!

Completed an initial pass, no major comments so far, planning on also giving this a spin on mainnet as well to see the impact of the added memory load on the newly optimized mater, with the 60k channel loaded in (w/ and w/o strict pruning).

Roasbeef · 2021-09-09T00:23:58Z

Yeah bit of a naming collision here...

Not clear why we need this intermediate struct actually, givne we just need to pass in the kvdb.Backend directly?

Roasbeef · 2021-09-09T00:28:52Z

In theory, the caller of this method could query the link node then query the graph, and merge it themselves manually. It's down as is for mainly historical and convenience reasons.

The only places that use this method atm is the chanbackup package (to include all the known addresses of a peer in the latest SCB instance). The chanbackup package uses the LiveChannelSource so it isn't tightly bound to the way we extract things here at the database level. Don't think this is super blocking though, just a nice to have to have cleaner seperation here, which may help with some other remote DB features in the future.

Roasbeef · 2021-09-09T00:31:03Z

+	startTime := time.Now()
+	log.Debugf("Populating in-memory channel graph, this might take a " +
+		"while...")
+	err := g.ForEachNode(func(tx kvdb.RTx, node *LightningNode) error {


I wonder if we should build tag this out for mobile clients? So if the mobile build tags are active, we don't load all this in....would be good to get some testing on this front, as worst case maybe this causes some weird OOM stuff for mobile nodes inadvertently.

Roasbeef · 2021-09-09T00:44:32Z

If we somehow attempt to insert a channel edge policy before the actual node?

Roasbeef · 2021-09-09T00:46:28Z

Can also avoid this w/ a nested map layer, so map[Vertex]map[ChannelID]Edge (assuming we don't need to preserve ordering of iteration, maybe it's better not to as then we also get some slight randomization here from the Go runtime?).

I've updated the cache to use the map[Vertex]map[ChannelID]Edge as suggested. Makes sense here, even if it is perhaps slightly bigger.

Roasbeef · 2021-09-09T00:47:31Z

This is for some future where like neutrino nodes eventually fully validate their channel graph? Or splicing perhaps?

Yeah, currently this is mostly a no-op for the cache. But maybe we'll actually update something later on that needs to be tracked in the graph. That way we already have the update in place. Currently the only call path here is router.AddProof() -> graph.UpdateChannelEdge() -> cache.UpdateChannel().

Roasbeef · 2021-09-09T00:48:30Z

I think one thing we'll want to do minimally is update the prior set of channel DB tests to have pre and post test assertions w.r.t the state of the channel cache. This should help to catch any instance we may have missed re cache consistency.

Roasbeef · 2021-09-09T00:57:08Z

Might be useful to tack on a comment that we do path finding backards, so we're always interested in the edge that arrives to us from the other node. Haven't really found the prefect terminology for all this though, mainly depends on like how one visualizes it in a sense.

guggero · 2021-09-28T17:59:58Z

I think this should behave a bit better concerning memory now. The improvements will be more noticeable with a larger number of nodes.
So I think the PR is ready for another round of review.

The itest is still failing though I cannot really consistently reproduce it locally. Will try again tomorrow.

guggero · 2021-09-28T18:32:30Z

On top of that, will add a flag for just bolt-only that allows this to be disabled for mobile nodes.

Yeah, I think I'm going to add that during the RC phase in a separate PR to get this moving. Feels like the PR is large enough as is.

bhandras

Re-reviewed the optimizaton part, PR LGTM 👍 , just one question regarding the scratch buffer.

The funding manager doesn't need to know the details of the underlying storage of the opening channel state, so we move the actual store and retrieval into the channel database.

As a preparation to have the method for querying the addresses of a node separate from the channel state, we extract that method out into its own interface.

To further separate the channel graph from the channel state, we refactor the AddrsForNode method to use the graphs's public methods instead of directly accessing any buckets. This makes sure that we can have the channel state cached with just its buckets while not using a kvdb level cache for the graph. At the same time we refactor the graph's test to also be less dependent upon the channel state DB.

Adds an in-memory channel graph cache for faster pathfinding. Original PoC by: Joost Jager Co-Authored by: Oliver Gugger

To avoid the channel map needing to be re-grown while we fill the cache initially, we might as well pre-allocate it with a somewhat sane value to decrease the number of grow events.

With this commit we use an optimized version of the node iteration that causes fewer memory allocations by only loading the part of the graph node that we actually need to know for the cache.

This commit fixes a flake in the channel status update itest that occurred if Carol got a channel edge update for a channel before it heard of the channel in the first place. To avoid that, we wait for Carol to sync her graph before sending out channel edge or policy updates. As always when we touch itest code, we bring the formatting and use of the require library up to date.

guggero · 2021-09-29T16:01:09Z

I think I finally nailed the flaky itest (at least the update_channel_status one).

guggero · 2021-09-30T12:12:33Z

There are still some flakes. I think I was able to identify and fix some of them in #5811.
Other than that I think this PR is good to go, pending your final review, @Roasbeef.

Roasbeef · 2021-10-01T03:23:44Z

There are still some flakes. I think I was able to identify and fix some of them in #5811.

You consider these a blocker as well?

guggero · 2021-10-01T08:02:06Z

Not a blocker necessarily. Just feels a bit scary to see the "vanilla" itest fail (the btcd on Linux one). But I think #5811 should take care of it. We can merge this if we want to get it in.

Roasbeef · 2021-10-01T21:53:53Z

Gave this another spin, ended up nocking down that initial burst quite a bit. I think we still want to add the flag to disable for mobile however (along w/ additional testing)

Roasbeef

LGTM 🦕

Roasbeef · 2021-10-01T22:04:24Z

I've fixed up my btcd PR, so once that lands in the repo, we can rebase #5811

guggero mentioned this pull request Aug 19, 2021

channeldb: node channels cache [poc] #5631

Closed

bhandras self-requested a review August 19, 2021 12:25

joostjager reviewed Aug 19, 2021

View reviewed changes

Comment thread channeldb/graph.go Outdated

Comment thread channeldb/graph.go Outdated

Comment thread channeldb/graph_cache.go Outdated

Comment thread channeldb/graph_cache.go Outdated

Comment thread channeldb/graph_cache.go Outdated

Comment thread routing/graph.go Outdated

bhandras mentioned this pull request Aug 19, 2021

routing/localchans: fix nested db tx #5643

Merged

guggero force-pushed the in-memory-graph branch 2 times, most recently from f2382bf to 64a4966 Compare August 26, 2021 16:26

bhandras mentioned this pull request Aug 26, 2021

channeldb: write through cache for the graph and channel state #5595

Closed

guggero force-pushed the in-memory-graph branch from 64a4966 to dde1d22 Compare August 30, 2021 11:03

guggero changed the title ~~[WIP] In-memory graph cache for faster pathfinding~~ In-memory graph cache for faster pathfinding Aug 30, 2021

guggero requested a review from Roasbeef August 30, 2021 11:05

Roasbeef added this to the v0.14.0 milestone Aug 30, 2021

Roasbeef added the P2 should be fixed if one has time label Aug 31, 2021

bhandras reviewed Aug 31, 2021

View reviewed changes

guggero force-pushed the in-memory-graph branch from dde1d22 to f6ccffe Compare September 1, 2021 13:47

guggero requested a review from bhandras September 1, 2021 13:48

guggero force-pushed the in-memory-graph branch from f6ccffe to 229ca32 Compare September 1, 2021 13:51

yyforyongyu reviewed Sep 2, 2021

View reviewed changes

bhandras approved these changes Sep 2, 2021

View reviewed changes

carlaKC mentioned this pull request Sep 3, 2021

routing: include htlc amount in bandwidth hint queries #5512

Merged

guggero force-pushed the in-memory-graph branch from 229ca32 to f27ec45 Compare September 6, 2021 14:30

yyforyongyu reviewed Sep 7, 2021

View reviewed changes

Roasbeef requested changes Sep 9, 2021

View reviewed changes

yyforyongyu mentioned this pull request Sep 14, 2021

itest-flake: revocation test, process fails to exit #5497

Closed

guggero force-pushed the in-memory-graph branch 2 times, most recently from 90027fe to 3295a1a Compare September 15, 2021 13:32

guggero requested review from Roasbeef and bhandras September 28, 2021 18:00

bhandras approved these changes Sep 29, 2021

View reviewed changes

Comment thread channeldb/graph.go Outdated

Comment thread channeldb/graph_test.go Outdated

bhandras and others added 14 commits September 29, 2021 17:00

channeldb: use kvdb.Backend instead of channeldb.DB for the Graph

639faee

channeldb: fix dangerous type casting hack

292b8e1

multi: carve out LinkNodeDB from channeldb.DB for cleaner separation

60cccf8

channeldb+funding: move opening channel state to DB

c1f686f

The funding manager doesn't need to know the details of the underlying storage of the opening channel state, so we move the actual store and retrieval into the channel database.

multi: extract address source into interface

ddea833

As a preparation to have the method for querying the addresses of a node separate from the channel state, we extract that method out into its own interface.

multi: move all channelstate operations to ChannelStateDB

11cf421

channeldb+routing: add in-memory graph

369c09b

Adds an in-memory channel graph cache for faster pathfinding. Original PoC by: Joost Jager Co-Authored by: Oliver Gugger

multi: use cache for source channels

15d3f62

multi: use minimal policy in cache

1d1c42f

routing+server: use cached graph interface

bf27d05

lnd+channeldb: pre-allocate cache size

a95a372

To avoid the channel map needing to be re-grown while we fill the cache initially, we might as well pre-allocate it with a somewhat sane value to decrease the number of grow events.

docs: add release notes

a5202a8

channeldb: optimize memory usage of initial cache fill

6240851

With this commit we use an optimized version of the node iteration that causes fewer memory allocations by only loading the part of the graph node that we actually need to know for the cache.

guggero force-pushed the in-memory-graph branch from 245c6db to 6240851 Compare September 29, 2021 15:00

Roasbeef approved these changes Oct 1, 2021

View reviewed changes

guggero merged commit 692ea25 into lightningnetwork:master Oct 4, 2021

guggero deleted the in-memory-graph branch October 4, 2021 09:20

Conversation

guggero commented Aug 19, 2021 • edited by Roasbeef Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bhandras commented Aug 26, 2021

Uh oh!

guggero commented Aug 27, 2021

Uh oh!

bhandras commented Aug 27, 2021

Uh oh!

guggero commented Aug 30, 2021

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bhandras left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

orijbot commented Sep 6, 2021

Uh oh!

yyforyongyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guggero commented Aug 19, 2021 •

edited by Roasbeef

Loading