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

[matheusd]

Environment

lnd version
lnd version 0.15.99-beta commit=tor/v1.1.0-41-gb8545de5c
(This is a development version I just happen to have compiled, I can test on another one if requested).

go version
go version go1.19.3 linux/amd64

platform
linux/amd64

Description

I have setup the following test env to profile payment performance with 3 nodes (Alice <> Bob <> Charlie) and started a continuous stream of payments from Charlie to Alice, while profiling (with --profile <port>) on Bob.

While the payments were made, I fetched a profile with curl http://127.0.0.1:<port>/debug/pprof/profile?seconds=60 and reviewed the results.

The top offenders for CPU load during payment that I can eyeball are:

1. bbolt.Tx.Commit()
2. lnwallet.DeriveCommitmentKeys()
3. channeldb.OpenChannel.isBorked()

While the first two are necessary (as they directly involve the protocol for updating channel state), upon reviewing the code for the third one, it seems less efficient than it could be. Specifically, it recreates the entire channel structure just to check the status of one specific field.

While I believe it's not absolutely safe to rely on the c.channStatus to check on the status (because other parts of the app might be using a different *OpenChannel instance to refer to the same underlying channel), this function could at least be improved upon by either reading the offset to the chanStatus field directly or reading and discarding the fields up to chanStatus and then returning immediately, which would improve performance.

I'm opening this issue prior to submitting a PR so that the best approach to solving this can be discussed.

[Roasbeef]

See this issue for some prior discussion re reducing the number of db transactions in the critical path: #5186.

Also can you amend the OP to include the lnd/go/platform runtime information? Thanks.

[matheusd]

This is indeed a subset of #5186, but note that this specific issue isn't so much about the db access pattern, but more of what exactly that isBorked function does (while inside a tx): even reducing/batching the db txs, it would still be the case that most of the work done by the isBorked function is wasted, due to only really using a single field of the underlying data.

[bhandras]

this function could at least be improved upon by either reading the offset to the chanStatus field directly or reading and discarding the fields up to chanStatus and then returning immediately

Alternatively we could consider a design where the in memory OpenChannel behaves as a write through cache and we don't instantiate new OpenChannel objects but only use one instance per channel (reference counted). That way shared ownership would result in fast concurrent reads since all of the members are cached and exists only once. We experimented with something similar in #5595, but in the end it's more of a hack around the proper design than a good solution.

Also that cache mentioned above didn't improve that much when using bbolt, due to BoltDB's design choice of memory mapping pages (and because deserialization still happened), but helped a lot when using Postgres or etcd.

[morehouse]

How big of a performance issue is this really? @matheusd could you attach the profiles?

A dead simple improvement we could do immediately is to use fetchChanInfo instead of fetchOpenChannel in isBorked. In general, it looks like there a many places we are doing full reading/writing with fetchOpenChannel/putOpenChannel when we don't need to.

@matheusd's idea to read just the chanStatus offset is a further optimization. But it seems like a bit of a bandaid to avoid changing the DB structure. If we really need to check channel status so frequently, maybe we should put them in a special subbucket. This would also improve performance for putChanStatus and clearChanStatus.

[matheusd]

How big of a performance issue is this really? @matheusd could you attach the profiles?

I can do that later today, but an easy way to see the impact of this is to run

make build-itest itest-only icase="async_payments_benchmark"

both with the current code and then with this following diff:

diff --git a/channeldb/channel.go b/channeldb/channel.go
index cd149c861..fc513f2aa 100644
--- a/channeldb/channel.go
+++ b/channeldb/channel.go
@@ -1454,12 +1454,7 @@ func (c *OpenChannel) ChanSyncMsg() (*lnwire.ChannelReestablish, error) {
 //
 // NOTE: The primary mutex should already be held before this method is called.
 func (c *OpenChannel) isBorked(chanBucket kvdb.RBucket) (bool, error) {
-	channel, err := fetchOpenChannel(chanBucket, &c.FundingOutpoint)
-	if err != nil {
-		return false, err
-	}
-
-	return channel.chanStatus != ChanStatusDefault, nil
+	return c.chanStatus != ChanStatusDefault, nil
 }
 
 // MarkCommitmentBroadcasted marks the channel as a commitment transaction has

In my personal machine this makes the benchmark's TPS go from ~130 to ~200. With the change you suggested (using fetchChanInfo and discarding the unused fields) it goes to ~165. Here's a representative run:

Without the diff:
=== RUN   TestLightningNetworkDaemon/tranche00/15-of-67/btcd/async_payments_benchmark
   test_harness.go:123:        Benchmark info: Elapsed time:  3.417928719s
   test_harness.go:123:        Benchmark info: TPS:  141.31365505519193

With the diff:
=== RUN   TestLightningNetworkDaemon/tranche00/15-of-67/btcd/async_payments_benchmark
   test_harness.go:123:        Benchmark info: Elapsed time:  2.528119663s
   test_harness.go:123:        Benchmark info: TPS:  191.05108317018775

[matheusd]

Alternatively we could consider a design where the in memory OpenChannel behaves as a write through cache and we don't instantiate new OpenChannel objects but only use one instance per channel (reference counted).

Yes, IMO a design where only one OpenChannel instance exists throughout the app (and any channel-modifying operations go through that) would be ideal (because then you enforce that every subsystem is getting the most up to date view of the channel and all fields are already in memory).