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

bhandras · 2021-08-02T13:56:03Z

This PR adds a generic cache to the kvdb layer as well as demonstrates its use caching the channel state and the graph (graph cache being reworked to be more specific: #5642).

With this cache we can prefetch frequently accessed buckets upon start and reduce DB operations to puts and deletes which will help with performance when LND is running on replicated remote databases (etcd, Postgres).

Roasbeef · 2021-08-06T23:56:08Z

Ready to remove the draft tag on this one?

bhandras · 2021-08-09T16:29:09Z

Ready to remove the draft tag on this one?

Yep, ready for a round of reviews imho. The thing I'm thinking about is if we want to speed up the payments bucket this way too. For that to work out we'd surely need to fetch on-demand and LRU evict since we don't want to cache old payments. Perhaps best to do that in a separate PR.

Roasbeef

Amazing PR!

TBH, I was a bit skeptical when you mentioned you were going with a more generic approach here vs a more context-specific cache for the channel graph itself, but the ultimate implementation is a lot more compact than I anticipated.

I've completed an initial review pass, and a few things popped out to me:

Do we have any tangible benchmarks on a mainnet-loaded node to gauge the perf increase, as well as the memory increase trade off? I'm a bit confined about the impact of caching larger buckets like the forwarding package state and channel revocation log.
- If it turns out to be prohibitive in practice, then at least we gain the added benefit of graph operations being mostly cached which should speed up path finding attempts (mainly the initial read at the start of each attempt).
In the context of boltdb, how much faster is a vanilla read from the database (which is memory mapped) vs fetching an item from the cache?

Roasbeef · 2021-08-07T00:25:33Z

kvdb/cache.go

+
+type cacheBucket struct {
+	seq  *uint64
+	tree *btree.BTree


yo dawg, I heard you like b-trees 🤣

So with this, we always end up caching the contents of an entire bucket? I'd be concerned about the memory blow up here for larger nodes that have a large revocation log (ever growing append only log for channels), invoices, payments, etc.

Do we have any profiles that show the resident memory overhead of this patch on mainnet?

Yeah, this simple implementation is only allowing us to cache whole buckets from top-level down to the leafes. This is because if we want to evict whole buckets from the memory when they're inacive (LRU strategy) we'd need a more compex architecture:

single reader/writer since both can modify the cache

need to bubble up and create the DB read buckets if the cache needs to read in an evicted bucket (this is needed, because a huge performance bottleneck alone is just getting the bucket key/values when structure is deep).

Current PR only caches channel-state. With the payments bucket we really wouldn't want to read the whole bucket on start. Invoices bucket didn't seem to be very problematic in my benchmarks so far.

Roasbeef · 2021-08-07T00:27:58Z

kvdb/cache.go

+}
+
+// Enforce that Cache implements the ExtendedBackend interface.
+var _ walletdb.DB = (*Cache)(nil)


Move this down below where the definition of Cache is first added?

Roasbeef · 2021-08-07T00:29:08Z

kvdb/cache.go

+		currRoot := root
+
+		for {
+			currBucket.ForEach(func(k, v []byte) error {


Doesn't return the error, seems to be assuming here that since we return nil ourselves, an error can never happen here?

fixed, ptal

Roasbeef · 2021-08-10T23:03:12Z

kvdb/cache.go

+)
+
+type cacheBucket struct {
+	seq  *uint64


Any reason this is a pointer vs the raw value?

This is to lazy initialize the seq (no need to fetch it for all buckets when prefetching). Fortunately Sequence is only part of the ReadWriteBucket interface so we can do this.

Roasbeef · 2021-08-10T23:05:20Z

kvdb/cache.go

+)
+
+const (
+	treeDeg = 3


Arbitrary value or was there some tuning here?

Not much testing yet (I tested with 3 and 5) maybe best to change it to be a parameter instead.

channeldb/db.go

Roasbeef · 2021-08-10T23:32:07Z

channeldb/channel.go

@@ -2353,7 +2353,7 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg,

 	var newRemoteCommit *ChannelCommitment

-	err := kvdb.Update(c.Db, func(tx kvdb.RwTx) error {
+	err := kvdb.Update(c.Db.chanCache, func(tx kvdb.RwTx) error {


I don't think we want to cache this bucket (or rather the operations in this DB closure) since we end up accessing an ever-growing bucket (all the past revoked states). On larger nodes this may be in the GBs range (would need to use the bolt browser on a volunteer to get a better picture of things).

With the current design, is it correct that we must use the cache here, since otherwise we'd have consistency issues since other buckets like the revocation state get modified here?

Okay this is somehting I didn't see properly, but yes you're correct if some of these channel state buckets grow indefinitely then we need to do something about evicting on the fly. Yes for now we need things together for consistency.

PR updated to skip some buckets.

Roasbeef · 2021-08-10T23:32:30Z

channeldb/channel.go

@@ -2528,7 +2528,7 @@ func (c *OpenChannel) LoadFwdPkgs() ([]*FwdPkg, error) {
 	defer c.RUnlock()

 	var fwdPkgs []*FwdPkg
-	if err := kvdb.View(c.Db, func(tx kvdb.RTx) error {
+	if err := kvdb.View(c.Db.chanCache, func(tx kvdb.RTx) error {


Similar comment here re not caching this bucket as it may be very large for long lived larger routing nodes.

PR updated to skip some buckets that grow indefinitely.

Roasbeef · 2021-08-10T23:33:25Z

channeldb/db.go

+		return err
+	}
+
+	if err := cache.AddTopLevelBucket(edgeIndexBucket); err != nil {


IIRC, we already have a cache here there that was added to speed up the gossip queries...

Yes I kept it there for the moment. If we end up liking the ideas in this PR I'll remove that. For now we need all top-level buckets to be cached since we do not read on-demand if the cache is empty for a bucket.

Roasbeef · 2021-08-10T23:34:13Z

channeldb/db.go

@@ -1337,9 +1381,11 @@ func fetchHistoricalChanBucket(tx kvdb.RTx,

 // FetchHistoricalChannel fetches open channel data from the historical channel
 // bucket.
-func (d *DB) FetchHistoricalChannel(outPoint *wire.OutPoint) (*OpenChannel, error) {
+func (c *ChannelStateDB) FetchHistoricalChannel(outPoint *wire.OutPoint) (


Ok I see what type of refactors you were referring to now.

I think we could really do more work re: separating things in channeldb. The idea would be to hide things behind intefaces in the end and never allow end-user code to access these buckets at all. This would also allow us to gradually refactor things to work better with other backends.

guggero

Yeah, awesome PR indeed! I did a high-level code only review to get the gist of what's needed. Going to try and performance test this on mainnet to see how much memory the graph cache takes. And then maybe run another test on regtest to see what 500 channels with many payments look like.

guggero · 2021-08-11T15:03:41Z

channeldb/nodes.go

+	db kvdb.Backend
+}
+
+func (l *LinkNodeDB) GetBackend() kvdb.Backend {


This doesn't seem to be used at all?

guggero · 2021-08-11T15:10:45Z

channeldb/channel.go

@@ -729,7 +729,7 @@ type OpenChannel struct {
 	RevocationKeyLocator keychain.KeyLocator

 	// TODO(roasbeef): eww
-	Db *DB
+	Db *ChannelStateDB


I think it's finally time to remove that comment above 😅

I still think it's somewhat eww 😃

guggero · 2021-08-11T15:11:22Z

channeldb/db.go

-	// LinkNodeDB separates all DB operations on LinkNodes.
-	LinkNodeDB
+	// ChannelStateDB separates all DB operations on channel state.
+	ChannelStateDB


Any reason not to embed this as a pointer?

bhandras · 2021-08-11T15:59:01Z

Yeah, awesome PR indeed! I did a high-level code only review to get the gist of what's needed. Going to try and performance test this on mainnet to see how much memory the graph cache takes. And then maybe run another test on regtest to see what 500 channels with many payments look like.

Thanks for taking a look Oliver!

Quick note about benchmarking: If you use the Bottlepay benchmark, ideally you'd need changes in the "wip etcd performance improvements PR" (#5392). I rebased that on top of this. My benchmarks showed great performance of the graph itself, since it's all in memory, we only pay the serializaton/deserializaton cost. We could mitigate that too, although it's a bigger refactor.

Currently the main bottleneck remaining after this is the payments bucket and its deep bucketed payment-htlcs-bucket. If we don't mitigate those roundtrips the bottlepay benchmark doesn't perform well.

bhandras · 2021-08-11T16:32:51Z

Amazing PR!

TBH, I was a bit skeptical when you mentioned you were going with a more generic approach here vs a more context-specific cache for the channel graph itself, but the ultimate implementation is a lot more compact than I anticipated.

I've completed an initial review pass, and a few things popped out to me:

Do we have any tangible benchmarks on a mainnet-loaded node to gauge the perf increase, as well as the memory increase trade off? I'm a bit confined about the impact of caching larger buckets like the forwarding package state and channel revocation log.

If it turns out to be prohibitive in practice, then at least we gain the added benefit of graph operations being mostly cached which should speed up path finding attempts (mainly the initial read at the start of each attempt).

In the context of boltdb, how much faster is a vanilla read from the database (which is memory mapped) vs fetching an item from the cache?

Thanks for the thorough review @Roasbeef !!

I was only testing with the bottlepay benchamrks for now and also with the changes in wip: etcd fixes and performance improvements #5392. While it's a limited test, it shows bottlenecks pretty well. Numbers in my benchmarks were stable, around 30 TPS on my cloud machine (using etcd). This is also now fully remote, non-mixed so the numbers in that PR don't apply anymore. Now that you mentioned that the fwding package and channel revocation log can grow hug I think we need to rething the caching to be LRU design and to read on-demand (which could also help with the payments bucket)
In my tests it's actually about the same or a bit faster using this cache.

bhandras · 2022-06-22T16:53:25Z

I'm wondering that given the recent DB changes maybe we could drop this? @Roasbeef
Happy to rebase/rework if we still think this PR adds value for 0.16.

Roasbeef · 2022-06-23T22:23:18Z

@bhandras drop as in close? Has been a while since I've looked at this, but I think since then we've gone w/ the in-memory route for the graph which gave us a nice speed up and a reduction in the number of round trips. I think as we start to do more SQL specific stuff and add instrumentation to see on which operations we're waiting the longest on (which might still be in KV land) this could be useful in adding more caching to minimize network round trips there. So maybe it's an 0.17 thing? It's also been a while since I've run the bottlepay bechmarking stuff as well.

Roasbeef · 2022-07-12T00:42:42Z

So this might be a bit more relevant now, based on some of the profiles gathered in this issue: #6683.

TL;DR: in the wild a user's script ends up fetching the channel state a bunch, this makes everything else slower as either operations are blocked on this, or other transactions are held up (?) due to the constant queries.

bhandras · 2022-08-09T10:38:41Z

So this might be a bit more relevant now, based on some of the profiles gathered in this issue: #6683.

TL;DR: in the wild a user's script ends up fetching the channel state a bunch, this makes everything else slower as either operations are blocked on this, or other transactions are held up (?) due to the constant queries.

Happy to resurrect the PR for 0.16 if you think it's relevant. I think the issue reference is wrong, as it points to the payment lifecycle refactor, but in general this change list is more relevant for the case when using a remote database, since bolt will mmap most of it anyway (so not much to gain with bbolt).

lightninglabs-deploy · 2023-07-25T11:00:40Z

@bhandras, remember to re-request review from reviewers when ready

lightninglabs-deploy · 2023-07-28T13:46:07Z

Closing due to inactivity

lightninglabs-deploy · 2023-07-28T14:49:33Z

Closing due to inactivity

lightninglabs-deploy · 2023-07-28T15:52:49Z

Closing due to inactivity

bhandras added this to In progress in v0.14.0-beta via automation Aug 2, 2021

bhandras linked an issue Aug 2, 2021 that may be closed by this pull request

routing: maintain in-memory graph for path finding #5389

Closed

bhandras force-pushed the graph-tuning branch 4 times, most recently from 21c5899 to deeb284 Compare August 4, 2021 18:06

guggero mentioned this pull request Aug 5, 2021

etcd: enable full remote database support #5484

Merged

bhandras force-pushed the graph-tuning branch 9 times, most recently from ac7ceac to d0a9f12 Compare August 6, 2021 09:19

bhandras requested review from Roasbeef and guggero August 6, 2021 13:14

bhandras mentioned this pull request Aug 6, 2021

wip: etcd fixes and performance improvements #5392

Closed

bhandras force-pushed the graph-tuning branch from a84238c to 4c13231 Compare August 9, 2021 16:04

bhandras changed the title ~~[wip] write through cache for the graph and channel state~~ channeldb: write through cache for the graph and channel state Aug 9, 2021

bhandras marked this pull request as ready for review August 9, 2021 16:24

Roasbeef reviewed Aug 10, 2021

View reviewed changes

Roasbeef mentioned this pull request Aug 10, 2021

kvdb: add postgres #5366

Merged

guggero reviewed Aug 11, 2021

View reviewed changes

guggero self-requested a review August 11, 2021 15:26

bhandras added 4 commits October 7, 2021 16:36

kvdb: add extra test coverage for kvdb.Cache

bddb89c

channeldb: cache channel state and link nodes

9e1b2d4

channeldb: make the channel state cache optional

f6e2e0a

docs: update release 0.14 notes

e1632b6

bhandras force-pushed the graph-tuning branch from ddb04be to e1632b6 Compare October 7, 2021 14:37

joostjager removed their request for review December 13, 2021 13:48

joostjager mentioned this pull request Dec 22, 2021

In-memory graph population is very slow when running with Postgres backend #6107

Closed

Roasbeef added this to In progress in v0.15.0-beta via automation Feb 2, 2022

Roasbeef moved this from In progress to Blocked / Chopping Block / Up For Adoption in v0.15.0-beta Feb 2, 2022

Roasbeef modified the milestones: v0.15.0, v0.16.0 Apr 13, 2022

Roasbeef removed this from Blocked / Chopping Block / Up For Adoption in v0.15.0-beta Apr 13, 2022

Roasbeef modified the milestones: v0.16.0, Future Aug 23, 2022

Roasbeef mentioned this pull request Aug 23, 2022

Running getinfo with postgresql db is very slow #6702

Open

saubyk modified the milestones: Future, v0.17.0 Sep 20, 2022

bhandras mentioned this pull request Dec 20, 2022

[performance]: *OpenChannel.isBorked() is innefficient #7263

Open

ellemouton closed this Jul 28, 2023

saubyk removed this from the v0.18.0 milestone Aug 4, 2023

bhandras deleted the graph-tuning branch September 12, 2023 15:29

bhandras restored the graph-tuning branch September 12, 2023 15:31

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

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

Conversation

bhandras commented Aug 2, 2021 • edited

Roasbeef commented Aug 6, 2021

bhandras commented Aug 9, 2021 • edited

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhandras commented Aug 11, 2021

bhandras commented Aug 11, 2021 • edited

bhandras commented Jun 22, 2022

Roasbeef commented Jun 23, 2022

Roasbeef commented Jul 12, 2022

bhandras commented Aug 9, 2022 • edited

lightninglabs-deploy commented Jul 25, 2023

lightninglabs-deploy commented Jul 28, 2023

lightninglabs-deploy commented Jul 28, 2023

lightninglabs-deploy commented Jul 28, 2023

bhandras commented Aug 2, 2021 •

edited

bhandras commented Aug 9, 2021 •

edited

bhandras commented Aug 11, 2021 •

edited

bhandras commented Aug 9, 2022 •

edited