From bd9a6bd6256dd394f06bb406596d98299f4b2781 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Jul 2018 21:41:16 -0700 Subject: [PATCH 1/4] htlcswitch/link: conditional batch ticker In this commit, we prevent the htlcManager from being woken up by the batchTicker when there is no work to be done. Profiling has shown a significant portion of CPU time idling, since the batch ticker endlessly demands resources. We resolve this by only selecting on the batch ticker when we have a non-empty batch of downstream packets from the switch. --- htlcswitch/link.go | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 22de510e8..2914f9f20 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -846,6 +846,13 @@ func (l *channelLink) htlcManager() { batchTick := l.cfg.BatchTicker.Start() defer l.cfg.BatchTicker.Stop() + // We'll only need the batch ticker if we have outgoing updates that are + // not covered by our last signature. This value will be nil unless a + // downstream packet forces the batchCounter to be positive. After the + // batch is cleared, it will return to nil to prevent wasteful CPU time + // caused by the batch ticker waking up the htlcManager needlessly. + var maybeBatchTick <-chan time.Time + out: for { // We must always check if we failed at some point processing @@ -926,10 +933,12 @@ out: break out } - case <-batchTick: + case <-maybeBatchTick: // If the current batch is empty, then we have no work - // here. + // here. We also disable the batch ticker from waking up + // the htlcManager while the batch is empty. if l.batchCounter == 0 { + maybeBatchTick = nil continue } @@ -955,6 +964,13 @@ out: l.handleDownStreamPkt(packet, true) + // If the downstream packet resulted in a non-empty + // batch, reinstate the batch ticker so that it can be + // cleared. + if l.batchCounter > 0 { + maybeBatchTick = batchTick + } + // A message from the switch was just received. This indicates // that the link is an intermediate hop in a multi-hop HTLC // circuit. @@ -977,6 +993,13 @@ out: l.handleDownStreamPkt(pkt, false) + // If the downstream packet resulted in a non-empty + // batch, reinstate the batch ticker so that it can be + // cleared. + if l.batchCounter > 0 { + maybeBatchTick = batchTick + } + // A message from the connected peer was just received. This // indicates that we have a new incoming HTLC, either directly // for us, or part of a multi-hop HTLC circuit. From 5af19bb2b409895c61a3f41cb69a8507592317d2 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Jul 2018 22:25:38 -0700 Subject: [PATCH 2/4] htlcswitch/link: reusable BatchTicker This commit modifies the default BatchTicker implementation such that it will generate a new ticker with each call to Start(). This allows us to create a new ticker after releasing an old one due to the batch being empty. --- htlcswitch/link.go | 28 +++++++++++++++++----------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 2914f9f20..543a2a8f9 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -107,22 +107,29 @@ type Ticker interface { // BatchTicker implements the Ticker interface, and wraps a time.Ticker. type BatchTicker struct { - ticker *time.Ticker + duration time.Duration + ticker *time.Ticker } // NewBatchTicker returns a new BatchTicker that wraps the passed time.Ticker. -func NewBatchTicker(t *time.Ticker) *BatchTicker { - return &BatchTicker{t} +func NewBatchTicker(d time.Duration) *BatchTicker { + return &BatchTicker{ + duration: d, + } } // Start returns the tick channel for the underlying time.Ticker. func (t *BatchTicker) Start() <-chan time.Time { + t.ticker = time.NewTicker(t.duration) return t.ticker.C } // Stop stops the underlying time.Ticker. func (t *BatchTicker) Stop() { - t.ticker.Stop() + if t.ticker != nil { + t.ticker.Stop() + t.ticker = nil + } } // ChannelLinkConfig defines the configuration for the channel link. ALL @@ -794,6 +801,7 @@ func (l *channelLink) fwdPkgGarbager() { // NOTE: This MUST be run as a goroutine. func (l *channelLink) htlcManager() { defer func() { + l.cfg.BatchTicker.Stop() l.wg.Done() log.Infof("ChannelLink(%v) has exited", l) }() @@ -843,9 +851,6 @@ func (l *channelLink) htlcManager() { l.wg.Add(1) go l.fwdPkgGarbager() - batchTick := l.cfg.BatchTicker.Start() - defer l.cfg.BatchTicker.Stop() - // We'll only need the batch ticker if we have outgoing updates that are // not covered by our last signature. This value will be nil unless a // downstream packet forces the batchCounter to be positive. After the @@ -938,6 +943,7 @@ out: // here. We also disable the batch ticker from waking up // the htlcManager while the batch is empty. if l.batchCounter == 0 { + l.cfg.BatchTicker.Stop() maybeBatchTick = nil continue } @@ -967,8 +973,8 @@ out: // If the downstream packet resulted in a non-empty // batch, reinstate the batch ticker so that it can be // cleared. - if l.batchCounter > 0 { - maybeBatchTick = batchTick + if l.batchCounter > 0 && maybeBatchTick == nil { + maybeBatchTick = l.cfg.BatchTicker.Start() } // A message from the switch was just received. This indicates @@ -996,8 +1002,8 @@ out: // If the downstream packet resulted in a non-empty // batch, reinstate the batch ticker so that it can be // cleared. - if l.batchCounter > 0 { - maybeBatchTick = batchTick + if l.batchCounter > 0 && maybeBatchTick == nil { + maybeBatchTick = l.cfg.BatchTicker.Start() } // A message from the connected peer was just received. This From 3ed2241a94a3f8d7666678b69d4b4eebfe30c56c Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Jul 2018 22:27:07 -0700 Subject: [PATCH 3/4] htlcswitch/link_test: only pass duration to NewBatchTicker --- htlcswitch/link_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index c518e78ce..99cb7ce1e 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -1497,7 +1497,7 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( Registry: invoiceRegistry, ChainEvents: &contractcourt.ChainEventSubscription{}, BatchTicker: ticker, - FwdPkgGCTicker: NewBatchTicker(time.NewTicker(5 * time.Second)), + FwdPkgGCTicker: NewBatchTicker(5 * time.Second), // Make the BatchSize and Min/MaxFeeUpdateTimeout large enough // to not trigger commit updates automatically during tests. BatchSize: 10000, @@ -3885,7 +3885,7 @@ func restartLink(aliceChannel *lnwallet.LightningChannel, aliceSwitch *Switch, Registry: invoiceRegistry, ChainEvents: &contractcourt.ChainEventSubscription{}, BatchTicker: ticker, - FwdPkgGCTicker: NewBatchTicker(time.NewTicker(5 * time.Second)), + FwdPkgGCTicker: NewBatchTicker(5 * time.Second), // Make the BatchSize and Min/MaxFeeUpdateTimeout large enough // to not trigger commit updates automatically during tests. BatchSize: 10000, From 0efe5ca49d2008940dbafbe60aca5c58392adbc0 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Jul 2018 22:28:37 -0700 Subject: [PATCH 4/4] peer: only pass duration to htlcswitch.NewBatchTicker --- peer.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/peer.go b/peer.go index 822425b39..86cfdfde1 100644 --- a/peer.go +++ b/peer.go @@ -542,12 +542,10 @@ func (p *peer) addLink(chanPoint *wire.OutPoint, *chanPoint, signals, ) }, - OnChannelFailure: onChannelFailure, - SyncStates: syncStates, - BatchTicker: htlcswitch.NewBatchTicker( - time.NewTicker(50 * time.Millisecond)), - FwdPkgGCTicker: htlcswitch.NewBatchTicker( - time.NewTicker(time.Minute)), + OnChannelFailure: onChannelFailure, + SyncStates: syncStates, + BatchTicker: htlcswitch.NewBatchTicker(50 * time.Millisecond), + FwdPkgGCTicker: htlcswitch.NewBatchTicker(time.Minute), BatchSize: 10, UnsafeReplay: cfg.UnsafeReplay, MinFeeUpdateTimeout: htlcswitch.DefaultMinLinkFeeUpdateTimeout,