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.

Sign up for GitHub

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

Jump to bottom

perf: set MDB_NOLOCK for peer and blockchain database #3473

Merged
merged 10 commits into from
Dec 7, 2021
Merged

perf: set MDB_NOLOCK for peer and blockchain database #3473

merged 10 commits into from
Dec 7, 2021

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Oct 19, 2021

Description

  • Use MDB_NOLOCK flag for LMDB environment for peer db and blockchain db

Motivation and Context

In both cases we use RwLocks to manage concurrent access so any overhead on locking on LMDB is not required

From lmdb-zero:

NOLOCK - Don't do any locking. If concurrent access is anticipated, the caller must manage all concurrency itself. For proper operation the caller must enforce single-writer semantics, and must ensure that no readers are using old transactions while a writer is active. The simplest approach is to use an exclusive lock so that no readers may be active at all when a writer begins.

How Has This Been Tested?

Existing tests and manually

@sdbondi sdbondi force-pushed the core-remove-double-db-lock branch from 17e6d25 to e6b9759 Compare October 19, 2021 08:24
@sdbondi sdbondi changed the title fix: set MDB_NOLOCK for peer and blockchain database DNM - fix: set MDB_NOLOCK for peer and blockchain database Oct 19, 2021
@sdbondi
Copy link
Member Author

sdbondi commented Oct 19, 2021

Testing this, appears to work but DNM for now

@sdbondi sdbondi marked this pull request as draft October 19, 2021 08:25
@stringhandler
Copy link
Collaborator

Interesting idea

@sdbondi sdbondi force-pushed the core-remove-double-db-lock branch from e6b9759 to 3e80977 Compare October 19, 2021 09:24
@sdbondi
Copy link
Member Author

sdbondi commented Nov 9, 2021

Double checked that LMDB transaction rollbacks still work. I ran a single test (test_handle_possible_reorg_accum_difficulty_is_correct_case_1) a few times with LMDB locking on and off, and was not really able to see any meaningful difference in execution time. Of course, this is not a good way to measure performance. That said, I have not observed any issues with the base node / comms with locking turned off so perhaps we should give this a try if only to save on the cost of the reader table

No reader table is used if the database is on a read-only filesystem, or if MDB_NOLOCK is set.

@sdbondi sdbondi changed the title DNM - fix: set MDB_NOLOCK for peer and blockchain database perf: set MDB_NOLOCK for peer and blockchain database Nov 9, 2021
@sdbondi sdbondi marked this pull request as ready for review November 9, 2021 05:48
@stringhandler
Copy link
Collaborator

Can merge after #3460

@sdbondi
Merge branch 'development' into core-remove-double-db-lock
828cccf 
* development: (46 commits)
  refactor: remove tari_common dependency from tari_comms (tari-project#3580)
  feat: language detection for mnemonic seed words (tari-project#3590)
  chore: minor clippy fixes (tari-project#3576)
  fix: be more permissive of responses for the incorrect request_id (tari-project#3588)
  feat: track ping failures and disconnect (tari-project#3597)
  chore: upgrade tokio deps tari-project#3581 (tari-project#3595)
  feat: standardize output hash for unblinded output, transaction output and transaction input (tari-project#3592)
  fix: allow bullet proof value only rewinding off one-sided transaction (tari-project#3587)
  refactor: update miningcore repository links (tari-project#3593)
  refactor: clean up unwraps in wallet_ffi (tari-project#3585)
  fix: update daily test start times and seed phrase (tari-project#3584)
  fix: allow bullet proof value only rewinding in atomic swaps (tari-project#3586)
  v0.21.2
  feat: add atomic swap refund transaction handling (tari-project#3573)
  feat: improve wallet connectivity status for console wallet (tari-project#3577)
  v0.21.1
  feat: add error codes to LibWallet for CipherSeed errors (tari-project#3578)
  ci: split cucumber job into two (tari-project#3583)
  feat(wallet): import utxo’s as EncumberedToBeReceived rather than Unspent (tari-project#3575)
  docs: rfc 0250_Covenants (tari-project#3574)
  ...
@sdbondi
minor clippy
411c22d
@sdbondi
Merge branch 'development' into core-remove-double-db-lock
b5d5a5b 
* 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)
  ...
@sdbondi
fix compile error after merge
9a1f419
@aviator-app aviator-app bot merged commit 26f66d1 into tari-project:development Dec 7, 2021
@sdbondi sdbondi deleted the core-remove-double-db-lock branch December 7, 2021 18:33
sdbondi added a commit to sdbondi/tari that referenced this pull request Dec 8, 2021
@sdbondi
Merge branch 'development' into comms-peer-manager-signed-peers
4241ddd 
* development:
  chore: add eslint to explorer (tari-project#3648)
  v0.22.0
  perf: set MDB_NOLOCK for peer and blockchain database (tari-project#3473)
  feat: prevent banning of connected base node in wallet (tari-project#3642)
sdbondi added a commit to sdbondi/tari that referenced this pull request Dec 8, 2021
@sdbondi
Merge branch 'development' into comms-peer-manager-signed-peers
481cade 
* development:
  chore: add eslint to explorer (tari-project#3648)
  v0.22.0
  perf: set MDB_NOLOCK for peer and blockchain database (tari-project#3473)
  feat: prevent banning of connected base node in wallet (tari-project#3642)
@Cifko Cifko mentioned this pull request Dec 27, 2021
Merged
stringhandler pushed a commit that referenced this pull request Jan 7, 2022
@Cifko
chore: remove moving lock.mdb (#3674)
3ad9a90 
Description
---
The lock.mdb was disabled in [3473](#3473)

How Has This Been Tested?
---
`npm test -- --name "Blockchain database recovery"`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stringhandler stringhandler stringhandler approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sdbondi @stringhandler