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

[Fix] Part-1 - LevelDbStore #3414

Open
wants to merge 32 commits into
base: master
Choose a base branch
from
Open

[Fix] Part-1 - LevelDbStore #3414

wants to merge 32 commits into from

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jul 8, 2024 •
edited
Loading

Description

This fixes the store's dotnet memory management for GC, a long with adding new feature options, easier Exception catching.

Also exposes the store.
image

Change Log

  • Added more option features to ReadOptions, WriteOptions and Options.
  • Added LevelDBHandle.cs for GC and easy management.
  • Added IEnumerable<KeyValuePair<byte[], byte[]>> to Store.cs to Iterate over the whole store.
  • Changed namespace from Neo.IO.Data.LevelDB to Neo.IO.Storage.LevelDB
  • Added MaxOpenFiles=4096, SnappyCompression and FilterPolicy=10 option to Store.
  • Added code comments
  • Added thread-safety to batching since leveldb doesn't support multi-thread in batching.

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

How Has This Been Tested?

Unit tests

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

@cschuchardt88 cschuchardt88 mentioned this pull request Jul 8, 2024
Closed
14 tasks
@cschuchardt88 cschuchardt88 added the blocker Issues that are blocking other issues. Check issues details to see what it is blocking. label Jul 9, 2024
vncoelho
vncoelho reviewed Jul 15, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

can this PR be tested without PART 2?

@cschuchardt88
Copy link
Member Author

can this PR be tested without PART 2?

Yes

@Jim8y
Copy link
Contributor

Jim8y commented Jul 15, 2024

Hey Chris, can you please add a few UTs to demonstrate how your pr improves the leveldb? I am reviewing your code, but dont really clear how and where the improve works.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jul 16, 2024 •
edited
Loading

Hey Chris, can you please add a few UTs to demonstrate how your pr improves the leveldb? I am reviewing your code, but dont really clear how and where the improve works.

Part-2 are the tests

@Jim8y
Copy link
Contributor

Jim8y commented Jul 16, 2024

Will check part 2 as well then, but please avoid doing this, cause it makes a single pr not reviewable.

@cschuchardt88
Copy link
Member Author

Will check part 2 as well then, but please avoid doing this, cause it makes a single pr not reviewable.

Just trying to make it small.

@cschuchardt88
Copy link
Member Author

Added thread-safety to batching since leveldb doesn't support multi-thread in batching.

@Jim8y
Copy link
Contributor

Jim8y commented Jul 30, 2024

@superboyiii can you please rake a look at this pr and do some tests on it?

@superboyiii
Copy link
Member

@superboyiii can you please rake a look at this pr and do some tests on it?

Sure

@superboyiii
Copy link
Member

@cschuchardt88 Seems not an issue from this PR because it occurs on master in block 2724677, I will open an issue.
1722997794683

@cschuchardt88 cschuchardt88 mentioned this pull request Sep 13, 2024
Draft
17 tasks
shargon
shargon reviewed Oct 11, 2024
View reviewed changes
src/Plugins/LevelDBStore/IO/Data/LevelDB/LevelDBHandle.cs
Comment on lines +40 to +49
if (disposing)
{
FreeManagedObjects();
}
if (Handle != nint.Zero)
{
FreeUnManagedObjects();
Handle = nint.Zero;
}
_disposed = true;
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use Interlocked for thread safe flag use

Copy link
Member Author

Choose a reason for hiding this comment

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

That's reasonable I'll add it in when I get a chance

Copy link
Member Author

@cschuchardt88 cschuchardt88 Oct 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like you can't Interlocked. its a non-ref returning type. Reason for locking it?

@shargon
Copy link
Member

shargon commented Oct 11, 2024 •
edited
Loading

@superboyiii Require testing in x86 and x64 machines

@cschuchardt88
Copy link
Member Author

@superboyiii Require testing in x86 and x64 machines

@shargon I added the nuints. I think I got them all.

@superboyiii
Copy link
Member

@superboyiii Require testing in x86 and x64 machines

OK

superboyiii
superboyiii approved these changes Oct 28, 2024
View reviewed changes
Copy link
Member

@superboyiii superboyiii left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Retsted with Part2, it works well and stable. No data imcompatible. I don't have a way to get a x86 but it works well on x64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jim8y Jim8y Awaiting requested review from Jim8y

@vncoelho vncoelho Awaiting requested review from vncoelho

@superboyiii superboyiii Awaiting requested review from superboyiii

@shargon shargon Awaiting requested review from shargon

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
blocker Issues that are blocking other issues. Check issues details to see what it is blocking. Plugins Tested waiting for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cschuchardt88 @Jim8y @superboyiii @shargon @vncoelho @NGDAdmin