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

Optimized/Rebuild LevelDB Native for LevelDBStore #899

Closed

Conversation

cschuchardt88
Copy link
Member

Change Log

  • Optimized LevelDBStore
  • Code Cleanup
  • Rebuild

image

@cschuchardt88
Copy link
Member Author

@cschuchardt88 cschuchardt88 changed the title Optimized LevelDB Native for LevelDBStore Optimized/Rebuild LevelDB Native for LevelDBStore Apr 22, 2024
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.

@cschuchardt88 ,I would like to see a benchmark on this.
This is not the first time this part is being refactoring by you, right? Or is this continuation of previous commit?

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Apr 23, 2024

@vncoelho This is refactoring everything. Im sure there is performance benefits. But that isn't the point of this PR. This PR fixes dotnet management of the leveldb objects from marshal objects. Also you have access to leveldb classes for easy accessibly.

Example

var options = new Options { CreateIfMissing = true };
using (var db = new DB(options, path))
{
    db.Put("NA", "Na");

    using(var batch = new WriteBatch())
    {
        batch.Delete("NA")
             .Put("Tampa", "Green")
             .Put("London", "red")
             .Put("New York", "blue");
        db.Write(batch);
    }
}

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.

Reviewed 17/36

Comment on lines 43 to 52
if (disposing)
{
FreeManagedObjects();
}
if (this.Handle != IntPtr.Zero)
{
FreeUnManagedObjects();
this.Handle = IntPtr.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.

Move _disposed to beginning?

Suggested change
if (disposing)
{
FreeManagedObjects();
}
if (this.Handle != IntPtr.Zero)
{
FreeUnManagedObjects();
this.Handle = IntPtr.Zero;
}
_disposed = true;
_disposed = true;
if (disposing)
{
FreeManagedObjects();
}
if (this.Handle != IntPtr.Zero)
{
FreeUnManagedObjects();
this.Handle = IntPtr.Zero;
}

Copy link
Member Author

@cschuchardt88 cschuchardt88 Apr 23, 2024

Choose a reason for hiding this comment

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

_disposed is saying have already disposed objects. So happens after it completes.

Copy link
Member

@shargon shargon Apr 25, 2024

Choose a reason for hiding this comment

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

Prevents double dispose in multiple threads.

Copy link
Member Author

Choose a reason for hiding this comment

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

Moving above all that doesn't do anything either way. Only difference would be you set the _disposed variable early.

src/LevelDBStore/DB.cs Show resolved Hide resolved
src/LevelDBStore/ReadOptions.cs Outdated Show resolved Hide resolved
src/LevelDBStore/LeastRecentlyUsedSet.cs Outdated Show resolved Hide resolved
@cschuchardt88
Copy link
Member Author

@shargon @Jim8y review please

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.

@cschuchardt88 You never use this. and the code have a lot of this.. Is this code yours?

Copy link
Member Author

@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.

@shargon to answer your question I was keeping consistent with existing code in that library.

src/LevelDBStore/WriteOptions.cs Outdated Show resolved Hide resolved
src/LevelDBStore/ReadOptions.cs Outdated Show resolved Hide resolved
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

This PR contains a massive set of changes, it's hard to review and some mistake may be done occasionally. There should be a way to split this PR into multiple granular PRs, each PR for its own optimisation. It also would be nice to distinguish optimisations and code refactoring.

Also, every optimisation should be confirmed by benchmarks, otherwise it's controversial. I'm sure there is a way of C# code profiling. Take a look at nspcc-dev/neo-go@cd42b8b, nspcc-dev/neo-go@9712be7, nspcc-dev/neo-go@ffeb3b8 and other optimisation-related PRs in NeoGo.

There shouldn't be TODOs in the resulting code, since every TODO is an issue.

Also, have you tested the resulting code against the current mainnet/testnet? Is the state compatible?

src/LevelDBStore/Comparator.cs Show resolved Hide resolved
src/LevelDBStore/Comparator.cs Show resolved Hide resolved
@cschuchardt88
Copy link
Member Author

@AnnaShaleva

Also, have you tested the resulting code against the current mainnet/testnet? Is the state compatible?

Yes

This adds the full features of the leveldb library.

@shargon
Copy link
Member

shargon commented May 21, 2024

Repository moved to neo, please re-open there

@shargon shargon closed this May 21, 2024
@shargon shargon deleted the Fix-leveldbstore branch May 21, 2024 08:03
@cschuchardt88 cschuchardt88 restored the Fix-leveldbstore branch May 24, 2024 20:44
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