refactor(wallet): simpler debounce mechanism #3375
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Hi @mikethetike, this is nicely generalized, but I feel we will be missing the purpose of the debouncer for the balance calculation specifically. I have two major comments as seen with system-level testing:
- We need the very last trigger to also execute the required function because the next trigger can be received after the update occurred but before the end of the debounce time thus leaving the last trigger request stale; the wallet could wait for a long time for the next trigger displaying the wrong information. (This was seen with the balance specifically.)
- Function
trigger_balance_refreshspecifically needs a configurable debounce time. With the system-level testing I have performed thedefault_debounce_timeis too quick for this function for a large busy wallet; monitoring a couple of hundred transactions in various states will trigger this function continuously under stressed conditions as the balance calculation time quickly escalates to >> 1s. For that case, I was running with a 60 s delay between calculations to prevent continuous database queries.
|
You are solving the wrong problem then. If the calculation takes >1s you should make the calculation not take more than 1s as a first step |
aviator-app bot
pushed a commit
that referenced
this pull request
Sep 22, 2021
Description --- Fixed a bug in the console wallet's debouncer where the tokio delay did not perform as expected. This was replaced with a tokio interval with missed tick behaviour set to `MissedTickBehavior::Delay`. _**Note:** I know this is totally different from the approach in #3375, but it fixes a bug to present a working alternative._ Motivation and Context --- The debouncing cooldown period was not honoured by the tokio delay in the select loop. Compounded to this the event fired consecutively if the system got busy. Sampled time measurements of the change this PR brought measuring events `_ = interval.tick() => {` below with a user-configured debouncing cooldown period of 60s. The lags represent balance update requests that were ignored from the beginning of the time interval with the balance update only occurring at the end of the interval. The wallet in question started receiving 250 transactions during this period, therefore the high balance update request rate. ``` rust 2021-09-21 17:36:06.556792200 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (60008 ms): Err(Lagged(2)) 2021-09-21 17:36:59.404245700 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (15075 ms): Ok(()) 2021-09-21 17:37:44.332807100 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (43347 ms): Err(Lagged(5)) 2021-09-21 17:38:44.335269600 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (59129 ms): Ok(()) 2021-09-21 17:40:10.003133300 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (84774 ms): Ok(()) 2021-09-21 17:43:10.060240200 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (25690 ms): Ok(()) 2021-09-21 17:45:12.709112500 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (28316 ms): Ok(()) 2021-09-21 17:45:44.393584900 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (30605 ms): Err(Lagged(65)) 2021-09-21 17:46:44.395335600 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (58222 ms): Err(Lagged(260)) 2021-09-21 17:47:44.392690700 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (55383 ms): Err(Lagged(347)) 2021-09-21 17:48:44.393535800 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (47611 ms): Err(Lagged(485)) 2021-09-21 17:49:44.396478100 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (53356 ms): Err(Lagged(348)) 2021-09-21 17:50:44.403330700 [wallet::console_wallet::debouncer] TRACE tokio::select! delay (53906 ms): Err(Lagged(1)) ``` The balance refresh query time from before the 250 transactions was sent till after is shown below:  How Has This Been Tested? --- System-level testing
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
A simpler debounce mechanism for the wallet
Motivation and Context
@hansieodendaal in #3304 added a debounce service that potentially will queue up requests for a long time in the receive channel. This is a simpler implementation that achieves the same purpose. I have hard coded in the debounce time, since this is a visual timing that will feel smooth to the eye and configuring it per method and call is overkill.
How Has This Been Tested?
Manually