Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement dht pooled db connection #3596

Conversation

hansieodendaal
Copy link
Contributor

@hansieodendaal hansieodendaal commented Nov 19, 2021

Description

Implemented the pooled SQLite db connection for the DHT storage in favour of the Rust based async read-write lock, which makes use of the much faster SQLite3 Write-ahead Log (WAL) mode.

Motivation and Context

Recent changes to the common implementation for SQLite pooled connections managed in the background via Diesel and SQLite3 enabled this improvement.

How Has This Been Tested?

  • Unit tests
  • Cucumber tests
  • System-level tests [Edit: Running a twice more intensive stress test than before (2x sender wallets with 3x 1000 transactions on one computer to 3x receiver wallets on another computer vs. 1x sender wallet with 3x 1000 transactions to 3x receiver wallets) the disk resource utilization was 90+% down from previous levels - ~100% disk utilization on both systems during all phases of the transactions life cycle down to <~5% with occasional spikes up to ~80%. All transactions reached mined confirmed in all 5 participating wallets.)

Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, A little concerned about blocking tokio task scheduling with synchronous calls, but understand that may be tricky with pooled connections. Let's see if we end up with any perf issues - it's in the wallet, so will probably be fine.

comms/dht/src/dedup/dedup_cache.rs Outdated Show resolved Hide resolved
comms/dht/src/storage/error.rs Outdated Show resolved Hide resolved
comms/dht/src/store_forward/database/mod.rs Outdated Show resolved Hide resolved
@hansieodendaal
Copy link
Contributor Author

Looks good, A little concerned about blocking tokio task scheduling with synchronous calls, but understand that may be tricky with pooled connections. Let's see if we end up with any perf issues - it's in the wallet, so will probably be fine.

@sdbondi see edit about stress test results monitoring disk utilization. The change actually affects both the base node's and the wallet's DHT dedup databases.

@sdbondi
Copy link
Member

sdbondi commented Nov 23, 2021

@hansieodendaal I'm happy about using a WAL - was talking specifically about tokio scheduler blocking, but if the calls are quick enough it shouldn't have a negative impact. I expect it will be fine.

EDIT: oh just saw the updated PR description - LGTM

@sdbondi
Copy link
Member

sdbondi commented Nov 23, 2021

@hansieodendaal Just need to address the infinite recursion in PartialEq

sdbondi
sdbondi previously approved these changes Nov 23, 2021
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice looks good

philipr-za
philipr-za previously approved these changes Nov 23, 2021
StriderDM and others added 5 commits November 24, 2021 08:48
Implemented the pooled sqlite db connection for the dht storage in
favour of the Rust based async read-write lock.
@aviator-app aviator-app bot merged commit 2ac0757 into tari-project:development Nov 24, 2021
@hansieodendaal hansieodendaal deleted the ho_dht_pooled_db_connection branch November 24, 2021 12:01
sdbondi added a commit to sdbondi/tari that referenced this pull request Nov 25, 2021
* development:
  feat: implement dht pooled db connection (tari-project#3596)
  feat: add page for detailed mempool in explorer (tari-project#3613)
  chore: add pub key in the dailes notify (tari-project#3612)
  feat: display network for console wallet (tari-project#3611)
  docs: update covenants links (tari-project#3614)
sdbondi added a commit to sdbondi/tari that referenced this pull request Dec 6, 2021
* development: (29 commits)
  fix(pruned mode)!: prune inputs, allow horizon sync resume and other fixes (tari-project#3521)
  feat!: sending one-sided transactions in wallet_ffi (tari-project#3634)
  fix: use json 5 for tor identity (regression) (tari-project#3624)
  test: add operation_id to log messages (tari-project#3633)
  fix!: multiple monerod addresses in tari merge mining proxy (tari-project#3628)
  fix: get-peer command works with public key again (tari-project#3636)
  fix!: separate peer seeds to common.network (tari-project#3635)
  test: removed stress test log target (tari-project#3631)
  feat: removed transaction validation redundant events (tari-project#3630)
  feat: improve wallet responsiveness (tari-project#3625)
  feat: add bulletproof rewind profiling (tari-project#3618)
  fix!: console wallet grpc_console_wallet_addresss config (tari-project#3619)
  test: increase timeout in cucumber (tari-project#3621)
  chore: change status line (tari-project#3610)
  feat!: add tcp bypass settings for tor in wallet_ffi (tari-project#3615)
  feat: only trigger UTXO scanning when a new block event is received (tari-project#3620)
  feat: implement dht pooled db connection (tari-project#3596)
  feat: add page for detailed mempool in explorer (tari-project#3613)
  chore: add pub key in the dailes notify (tari-project#3612)
  feat: display network for console wallet (tari-project#3611)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants