[Core Plugin] adopt more change from pr 3414 #3591

Jim8y · 2024-11-21T10:32:43Z

Description

3rd part of Pr #3414 and credits belong to @cschuchardt88

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
Update IntPtr to nint

Fixes # #3414

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

src/Plugins/LevelDBStore/Plugins/Storage/Store.cs

src/Plugins/LevelDBStore/IO/Data/LevelDB/DB.cs

shargon · 2024-11-21T19:07:43Z

src/Plugins/LevelDBStore/IO/Data/LevelDB/DB.cs

+            iterator.SeekToFirst();
+            while (iterator.Valid())
+            {
+                yield return new KeyValuePair<byte[], byte[]>(iterator.Key(), iterator.Value());


This behaviour maybe it's different than the previous one, because we are yield during a snapshot

I prefer to receive the snapshot from an argument, maybe we don't wan't to use it, and avoid IEnumerable<KeyValuePair<byte[], byte[]>>

It's helpful for debugging. Better than being in the dark.

It's helpful for debugging. Better than being in the dark.

Will this cause state difference or behavior difference? If the result is the same, i think it should be fine, this plugin can only be used here for the core.

We should receive the snapshot or null or we can have a big mistakes

Will this cause state difference or behavior difference? If the result is the same, i think it should be fine, this plugin can only be used here for the core.

We should receive the snapshot or null or we can have a big mistakes

All its doing is displaying the whole database, has nothing to do with state or snapshots. The reason it takes a snapshot, so the data doesn't get changed (When viewing it).

But imagine that we use this method in a loop, when we change the data, we would not notice this error

than don't use snapshot just use

using var iterator = NewIterator(ReadOptions.Default);

Easy to forget

We can discuss in a different PR if we create the IEnumerable<KeyValuePair<byte[], byte[]>> , this one is ready to merge

shargon · 2024-11-21T19:09:53Z

src/Plugins/LevelDBStore/Plugins/Storage/Store.cs

+            _db.GetEnumerator();
+
+        IEnumerator IEnumerable.GetEnumerator() =>
+            _db.GetEnumerator();


cschuchardt88 · 2024-11-26T22:39:37Z

src/Plugins/LevelDBStore/IO/Data/LevelDB/DB.cs

 {
    /// <summary>
    /// A DB is a persistent ordered map from keys to values.
    /// A DB is safe for concurrent access from multiple threads without any external synchronization.
    /// </summary>
-    public class DB : LevelDBHandle
+    public class DB : LevelDBHandle, IEnumerable<KeyValuePair<byte[], byte[]>>


Looks like you are missing a lot of methods for this class.

Missing Methods

Throw(IntPtr error)

Throw(IntPtr error, Func<string, Exception> exception)

DB(string name, Options options)

Get(byte[] key, ReadOptions options) - Code of function

SnapShot CreateSnapshot()

string PropertyValue(string name)

void Repair(Options options, string name)

void Destroy(Options options, string name)

These can come in different PR

Missing methods

shargon · 2024-11-27T08:36:41Z

src/Plugins/LevelDBStore/Plugins/Storage/Snapshot.cs

@@ -44,11 +44,12 @@ public void Delete(byte[] key)
        public void Dispose()
        {
            _snapshot.Dispose();
+            _options.Dispose();


This memory leak is present in master

shargon · 2024-11-27T08:38:24Z

src/Plugins/LevelDBStore/Plugins/Storage/Store.cs

+        public void Dispose()
+        {
+            _db.Dispose();
+            _options.Dispose();


This one also, but one per node

nan01ab · 2024-11-29T01:21:01Z

src/Plugins/LevelDBStore/IO/Data/LevelDB/DB.cs

+
+            using var iterator = NewIterator(options);
+            iterator.SeekToFirst();
+            while (iterator.Valid())


Why is this while, and below is for. Might it be better to keep a consistent style?

He copied mine. but for would be better.

will be fixed in another pr.

adapt more change from pr 3414

4cc9b87

Jim8y requested review from cschuchardt88, shargon, vncoelho, roman-khimov and AnnaShaleva November 21, 2024 10:34

Jim8y added the Waiting for Review label Nov 21, 2024

Jim8y changed the title ~~[Core Plugin] adapt more change from pr 3414~~ [Core Plugin] adopt more change from pr 3414 Nov 21, 2024

shargon reviewed Nov 21, 2024

View reviewed changes

src/Plugins/LevelDBStore/Plugins/Storage/Store.cs Outdated Show resolved Hide resolved

Update src/Plugins/LevelDBStore/Plugins/Storage/Store.cs

1692a38

shargon reviewed Nov 21, 2024

View reviewed changes

src/Plugins/LevelDBStore/IO/Data/LevelDB/DB.cs Show resolved Hide resolved

Merge branch 'master' into pr-3414-3

6a35a54

shargon reviewed Nov 21, 2024

View reviewed changes

Merge branch 'master' into pr-3414-3

ec77f11

cschuchardt88 previously approved these changes Nov 23, 2024

View reviewed changes

Merge branch 'master' into pr-3414-3

3155891

cschuchardt88 reviewed Nov 26, 2024

View reviewed changes

Remove two memory leaks and avoid iterate without specify the snapshot

9680ea0

shargon approved these changes Nov 27, 2024

View reviewed changes

Merge branch 'master' into pr-3414-3

582a9a8

shargon reviewed Nov 27, 2024

View reviewed changes

nan01ab reviewed Nov 29, 2024

View reviewed changes

Merge branch 'master' into pr-3414-3

776ad7c

Jim8y requested a review from cschuchardt88 November 30, 2024 08:01

Jim8y added the Ready to Merge label Nov 30, 2024

cschuchardt88 approved these changes Nov 30, 2024

View reviewed changes

shargon merged commit 116c219 into neo-project:master Nov 30, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core Plugin] adopt more change from pr 3414 #3591

[Core Plugin] adopt more change from pr 3414 #3591

Jim8y commented Nov 21, 2024 •

edited

Loading

shargon Nov 21, 2024

shargon Nov 21, 2024 •

edited

Loading

cschuchardt88 Nov 21, 2024

Jim8y Nov 22, 2024 •

edited

Loading

shargon Nov 22, 2024

cschuchardt88 Nov 22, 2024 •

edited

Loading

shargon Nov 22, 2024

cschuchardt88 Nov 22, 2024

shargon Nov 24, 2024

shargon Nov 27, 2024

shargon Nov 21, 2024

cschuchardt88 Nov 26, 2024

shargon Nov 27, 2024 •

edited

Loading

shargon Nov 27, 2024

shargon Nov 27, 2024

nan01ab Nov 29, 2024

cschuchardt88 Nov 29, 2024

Jim8y Nov 30, 2024

[Core Plugin] adopt more change from pr 3414 #3591

[Core Plugin] adopt more change from pr 3414 #3591

Conversation

Jim8y commented Nov 21, 2024 • edited Loading

Description

Type of change

Checklist:

Choose a reason for hiding this comment

shargon Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Nov 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shargon Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y commented Nov 21, 2024 •

edited

Loading

shargon Nov 21, 2024 •

edited

Loading

Jim8y Nov 22, 2024 •

edited

Loading

cschuchardt88 Nov 22, 2024 •

edited

Loading

shargon Nov 27, 2024 •

edited

Loading