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

[Add]: LevelDB Benchmarks #3667

Merged
merged 12 commits into from
Jan 21, 2025

Conversation

nan01ab
Copy link
Contributor

@nan01ab nan01ab commented Jan 12, 2025

Description

In PR #3606, LevelDbStore.Snapshot introduces a lock to protect concurrent operations on the WriteBatch.
But:

  1. LevelDbStore.Snapshot has this synchronization mechanism, but RocksDbStore.Snapshotdoesn't.
  2. WriteBatchs (LevelDB and RocksDB`) are not for concurrent operations. If there are concurrent operations, each thread should use its own WriteBatch.
  3. On-chain operations must be deterministic, but concurrent operations do not guarantee determinism. So there shouldn't be any concurrent operations when writing to the store.

In summary, write operations on Snapshot should not be concurrent, so these locks are unnecessary.

Fixes # (issue)

Type of change

  • Optimization (the change is only an optimization)
  • Style (the change is only a code style for better maintenance or standard purpose)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

For me it's ok, but I think that @Jim8y think the opposite

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 13, 2025

For me it's ok, but I think that @Jim8y think the opposite

How is everything always ok? Let me refresh your mind! The reason for this is, core does or any other program that called this library, plugins included. This protects us from concurrent operations. Because WriteBatch can't be called concurrently. Its not supported, by leveldb!

Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

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

This is needed, Core and other programs/plugins use concurrent.

@Jim8y
Copy link
Contributor

Jim8y commented Jan 13, 2025

The core issue here is 3, we should not have concurrent operations, indeed, but how could we detect or avoid issue if concurrent operation is mistakenly involved, maybe by third party lib, maybe by some other modules? Say few years later it's no longer us to maintain this repo, how could 'they' avoid making such mistakes unintentionally. Many hidden design I faced while maintaining the core, 'oh you should not do it this way, it's wrong, but you won't know it until the project crashes.

@nan01ab
Copy link
Contributor Author

nan01ab commented Jan 13, 2025

The core issue here is 3, we should not have concurrent operations, indeed, but how could we detect or avoid issue if concurrent operation is mistakenly involved, maybe by third party lib, maybe by some other modules? Say few years later it's no longer us to maintain this repo, how could 'they' avoid making such mistakes unintentionally. Many hidden design I faced while maintaining the core, 'oh you should not do it this way, it's wrong, but you won't know it until the project crashes.

Using a WriteBatch in a concurrent context is a misuse(therefore, the WriteBatch in LevelDB and RocksDB does not need to be concurrency safe).

Adding lock here is to tell others that this can be used for concurrency, but it should not.
We can't provide a wrong way to use it.

how could 'they' avoid making such mistakes unintentionally.

That's the user's bug, and we can't help them hide the bug.

@nan01ab
Copy link
Contributor Author

nan01ab commented Jan 13, 2025

This is needed, Core and other programs/plugins use concurrent.

  1. No concurrent use in Core;
  2. WriteBatch is used for a single thread to commit a transaction with multi Put or Delete operations. We shouldn't support a wrong way of using it.

@Jim8y
Copy link
Contributor

Jim8y commented Jan 14, 2025

Yes, using it in concurrent is wrong and misuse, exactly and very clear, but the problem is how could you detect misuse and prevent it being misused. Require solid understanding with the core code/structure and thirdparty code/structure right? Say we have 100 details need to care about that will allow you to do it, but is actually wrong, what's worse it will casue problem that is hard to debug and trace, is the project still maintainable?

@cschuchardt88
Copy link
Member

cschuchardt88 commented Jan 15, 2025

Using a WriteBatch in a concurrent context is a misuse(therefore, the WriteBatch in LevelDB and RocksDB does not need to be concurrency safe).

WRONG
However other objects (like Iterator and WriteBatch) may require external synchronization. If two threads share such an object, they must protect access to it using their own locking protocol. More details are available in the public header files.

Reference: https://github.com/google/leveldb/blob/main/doc/index.md#concurrency


Plus this is your safe way to prevent such use. Nothing stops a SDK user or 3rd-party plugin or plugins in general from using different threads.

@nan01ab nan01ab force-pushed the improve.remove-lock-in-Snapshot branch from 5e9d5bf to 685d19a Compare January 15, 2025 16:43
@nan01ab nan01ab changed the title [Optimization]: remove unnecessary locks in Snapshot implementations [Optimization]: lock in LevelDB.Snapshot Jan 15, 2025
@nan01ab
Copy link
Contributor Author

nan01ab commented Jan 15, 2025

Fine.
Now this PR is to improve the lock operations, and add comments and benchmarks.

cschuchardt88
cschuchardt88 previously approved these changes Jan 17, 2025
@cschuchardt88 cschuchardt88 changed the title [Optimization]: lock in LevelDB.Snapshot [Add]: LevelDB Benchmarks Jan 17, 2025
shargon
shargon previously approved these changes Jan 18, 2025
@cschuchardt88 cschuchardt88 dismissed stale reviews from shargon and themself via 01adefe January 18, 2025 12:06
cschuchardt88
cschuchardt88 previously approved these changes Jan 18, 2025
@cschuchardt88 cschuchardt88 requested a review from shargon January 18, 2025 12:08
@cschuchardt88 cschuchardt88 dismissed their stale review January 20, 2025 05:06

Needs to fix format of functions

@shargon shargon merged commit 865c578 into neo-project:master Jan 21, 2025
6 of 7 checks passed
Jim8y added a commit to Jim8y/neo that referenced this pull request Jan 31, 2025
* master: (43 commits)
  Fix `GetAndChange` warnings (neo-project#3702)
  `Murmur3` should not be cryptographic hash algorithm (neo-project#3668)
  Test: add tests for native contract id (neo-project#3697)
  Update nugets (neo-project#3692)
  [Core P2P] fix the bug (neo-project#3695)
  Add hardfork HF_Echidna (neo-project#3454)
  Fix: add lock for RocksDbStore.Snapshot to keep same behavior as MemoryStore and LevelDbStore (neo-project#3689)
  Nullable rocks db (neo-project#3686)
  Nullable leveldb (neo-project#3685)
  Enforcement Compiler Warnings (neo-project#3687)
  [`Update`] Dotnet & Compiler Version (neo-project#3684)
  [`Add`]: LevelDB Benchmarks (neo-project#3667)
  [`Fix`]: Behavior when `keyPrefix` is null in different `IStore.Seek` impls. (neo-project#3682)
  Improve calculatenetworkfee (neo-project#3674)
  more 2025 (neo-project#3678)
  Nullable in Storage classes (neo-project#3670)
  readonly (neo-project#3676)
  [Fix] Set max entries for `VerifyProof` in `statePlugin` (neo-project#3675)
  Neo.json.benchmarks (neo-project#3673)
  Happy new year 2025 (neo-project#3677)
  ...

# Conflicts:
#	src/Neo/Neo.csproj
#	src/Neo/ProtocolSettings.cs
#	src/Neo/SmartContract/ApplicationEngine.cs
#	src/Neo/SmartContract/Native/NeoToken.cs
#	src/Neo/SmartContract/Native/RoleManagement.cs
#	tests/Neo.UnitTests/SmartContract/Native/UT_NativeContract.cs
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