[`Change`] DataCache to Match LevelDbStore & RocksDbStore #3709

cschuchardt88 · 2025-02-03T06:07:05Z

Description

Mostly bug fixes and same data results as the Leveldb and Rocksdb.

BenchmarkDotNet v0.14.0, Windows 11 (10.0.26100.3037)
Intel Core i7-8700K CPU 3.70GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 9.0.102
  [Host]   : .NET 9.0.1 (9.0.124.61010), X64 RyuJIT AVX2
  .NET 9.0 : .NET 9.0.1 (9.0.124.61010), X64 RyuJIT AVX2

Job=.NET 9.0  OutlierMode=DontRemove  Runtime=.NET 9.0  
Concurrent=True  Force=False  Server=True

Method	Mean	Error	StdDev	Median	Min	Max
TestOldDataCacheAdd	0.0009 ns	0.0023 ns	0.0021 ns	0.0000 ns	0.0000 ns	0.0080 ns
TestDataCacheAdd	0.0002 ns	0.0008 ns	0.0008 ns	0.0000 ns	0.0000 ns	0.0030 ns
TestOldDataCacheUpdate	0.0030 ns	0.0085 ns	0.0080 ns	0.0000 ns	0.0000 ns	0.0310 ns
TestDataCacheUpdate	0.0141 ns	0.0143 ns	0.0134 ns	0.0102 ns	0.0000 ns	0.0406 ns
TestOldDataCacheRemove	35.8684 ns	0.2957 ns	0.2766 ns	35.8496 ns	35.3293 ns	36.2868 ns
TestDataCacheRemove	78.9549 ns	0.5450 ns	0.5098 ns	78.7148 ns	78.3838 ns	80.0923 ns
TestOldDataCacheGet	0.0127 ns	0.0122 ns	0.0115 ns	0.0107 ns	0.0000 ns	0.0321 ns
TestDataCacheGet	0.0107 ns	0.0102 ns	0.0095 ns	0.0094 ns	0.0000 ns	0.0325 ns

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?

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

vncoelho · 2025-02-03T06:51:21Z

src/Neo.Extensions/ByteArrayComparer.cs

-                return y.AsSpan().SequenceCompareTo(x.AsSpan());
-            return x.AsSpan().SequenceCompareTo(y.AsSpan());
+
+            return x.AsSpan().SequenceCompareTo(y.AsSpan()) * _direction;


What about submitting the changes in this first in another PR?
Perhaps this can be discussed faster and merged quicker.

Maybe something like

public int Compare(byte[]? x, byte[]? y) { if (ReferenceEquals(x, y)) return 0; if (x is null) return y is null ? 0 : -_direction; if (y is null) return _direction; return _direction * x.AsSpan().SequenceCompareTo(y.AsSpan()); }

Is the length multiplication really needed?

byte[] a = new byte[10]; byte[] b = new byte[20]; ByteArrayComparer comparer = ByteArrayComparer.Default; Console.WriteLine(comparer.Compare(null, a)); // Expected: -1, Original: -10 Console.WriteLine(comparer.Compare(null, b)); // Expected: -1, Original: -20

Im sorry, I mean MemorySnapshot was returning all results for null.

tests/Neo.UnitTests/Persistence/UT_MemoryStore.cs

shargon · 2025-02-03T08:46:49Z

tests/Neo.UnitTests/SmartContract/UT_InteropService.cs

@@ -547,7 +546,7 @@ public void TestBlockchain_ListContracts()
            Assert.AreEqual(state.Hash, NativeContract.ContractManagement.GetContract(engine.SnapshotCache, state.Hash).Hash);

            var list2 = NativeContract.ContractManagement.ListContracts(engine.SnapshotCache);
-            Assert.AreEqual(list.Count(), list2.Count());
+            Assert.AreEqual(list.Count(), list2.Count() - 1);


Why this changed?

If you look up a couple of lines there is a new contract that got added. So the test would be invalid.

10 != 11

The test didn't change, this line was before

This was an issue with DataCache being bugged. Let me find out what happening, so I can go into more detail about it.

shargon · 2025-02-03T08:47:48Z

tests/Neo.UnitTests/Persistence/UT_DataCache.cs

@@ -290,7 +290,7 @@ public void TestFindRange()
            Assert.IsTrue(items[0].Value.EqualsTo(value5));
            Assert.AreEqual(key4, items[1].Key);
            Assert.IsTrue(items[1].Value.EqualsTo(value4));
-            Assert.AreEqual(2, items.Length);
+            Assert.AreEqual(3, items.Length);


Why this changed, and not in the other PR?

Because it from starting key to ending end. It's not in between. We don't skip the 1st one.

// OLD FindRange([0x00, 0x00, 0x04], [0x00, 0x00, 0x03], Backwards); // Results: Empty // NEW FindRange([0x00, 0x00, 0x04], [0x00, 0x00, 0x03], Backwards); // Results: // [0x00, 0x00, 0x04] // [0x00, 0x00, 0x03]

Could you move this fix into the other pr, and here only the DataCache refactor?

Isn't this part of the DataCache?

I rewrote all the code. So it would be hard for me to put back the bug.

…8/neo into fix/datacache-store

src/Neo/Persistence/StorageKeyEqualityComparer.cs

src/Neo/Persistence/MemoryStore.cs

…8/neo into fix/datacache-store

shargon · 2025-02-06T08:45:09Z

Replace the name, and change the logic, makes harder the review, I reverted the name replacement, you can do it in the next pr (i'm not against to) but in the same PR, makes this review harder and slower.

…8/neo into fix/datacache-store

src/Neo/Persistence/DataCache.CacheEntry.cs

src/Neo/Persistence/DataCache.cs

shargon · 2025-02-10T14:04:50Z

src/Neo/SmartContract/ApplicationEngine.cs

@@ -502,7 +502,7 @@ public ExecutionContext LoadScript(Script script, int rvcount = -1, int initialP
            // Create and configure context
            ExecutionContext context = CreateContext(script, rvcount, initialPosition);
            ExecutionContextState state = context.GetState<ExecutionContextState>();
-            state.SnapshotCache = SnapshotCache?.CloneCache();
+            state.SnapshotCache = SnapshotCache;


You can't remove this clone, it affects the try/catch safety

All CloneCache doesn't is the same as SnapshotCache. No data buffers are moved, neither any caches. CloneCache is just a wrapper class. I see the point of it. And if you are right why all tests work still?

@neo-project/core I tried to remove the CloneCache before, but it's not an easy task.

StorageItem is a class, and if is not cloned it will be shared between both DataCache, if in the second one is replace calling FromReplica (for example), the content of this instance will be changed in both DataCache, and if the second will be discarded, it will change the main one..

Maybe we don't have this unit tests, but it's how it work.

DataCache a; CloneCache b=new(a) var before= a.Get(1); var change=b.Get(1) change.FromReplica(xxxx) // before was changed and also a.Get(1)

Well I fixed the issue with StorageItem in another PR. Because your not at using a IComparer interface, you are just using the object as the key. It ends up throwing in dupes keys.

cschuchardt88 added 4 commits February 2, 2025 04:01

Added StorageCache

b301776

Fixes

7e2b43b

Merge branch 'master' into fix/datacache-store

36a795f

Fix cacheEntry

85c6d49

cschuchardt88 changed the title ~~['Change'] DataCache to Match LevelDbStore & RocksDbStore~~ [Change] DataCache to Match LevelDbStore & RocksDbStore Feb 3, 2025

cschuchardt88 added Discussion Initial issue state - proposed but not yet accepted Waiting for Review labels Feb 3, 2025

vncoelho reviewed Feb 3, 2025

View reviewed changes

cschuchardt88 added 2 commits February 3, 2025 03:18

Typos

81eeb44

Fixes

4630f41

shargon reviewed Feb 3, 2025

View reviewed changes

shargon added 3 commits February 3, 2025 09:48

change to var

81a0682

Merge branch 'fix/datacache-store' of https://github.com/cschuchardt8…

adb2878

…8/neo into fix/datacache-store

more vars

f34fae0

shargon reviewed Feb 3, 2025

View reviewed changes

src/Neo/Persistence/StorageKeyEqualityComparer.cs Outdated Show resolved Hide resolved

src/Neo/Persistence/MemoryStore.cs Outdated Show resolved Hide resolved

cschuchardt88 and others added 6 commits February 3, 2025 04:23

changes for StorageKeyEqualityComparer

3d8d8cd

Merge branch 'fix/datacache-store' of https://github.com/cschuchardt8…

034612f

…8/neo into fix/datacache-store

Fix pointer issues and unit tests

1acad8c

Merge branch 'master' into fix/datacache-store

d99c842

Merge branch 'master' into fix/datacache-store

ae5873a

Avoid remplace name

204c77f

cschuchardt88 and others added 8 commits February 6, 2025 03:46

Merge branch 'master' into fix/datacache-store

81eeed7

nullable enable for cacheEntry

48dc859

Merge branch 'fix/datacache-store' of https://github.com/cschuchardt8…

6160b21

…8/neo into fix/datacache-store

Merge branch 'master' into fix/datacache-store

4f78a50

Merge branch 'fix/datacache-store' of https://github.com/cschuchardt8…

c374320

…8/neo into fix/datacache-store

Change name back to DataCache for the file.

d68f8f7

File name change

7bacee0

revert changes

c1e92a5

cschuchardt88 commented Feb 6, 2025

View reviewed changes

src/Neo/Persistence/DataCache.CacheEntry.cs Outdated Show resolved Hide resolved

src/Neo/Persistence/DataCache.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

30bfd49

cschuchardt88 closed this Feb 6, 2025

cschuchardt88 reopened this Feb 7, 2025

cschuchardt88 added 3 commits February 6, 2025 20:51

Add benchmark

097e654

Merge branch 'master' into fix/datacache-store

b675911

Fixed typo

0827ae6

shargon reviewed Feb 10, 2025

View reviewed changes

cschuchardt88 closed this Feb 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`Change`] DataCache to Match LevelDbStore & RocksDbStore #3709

[`Change`] DataCache to Match LevelDbStore & RocksDbStore #3709

cschuchardt88 commented Feb 3, 2025 •

edited

Loading

vncoelho Feb 3, 2025

vncoelho Feb 3, 2025

cschuchardt88 Feb 3, 2025 •

edited

Loading

shargon Feb 3, 2025

cschuchardt88 Feb 3, 2025 •

edited

Loading

shargon Feb 6, 2025

cschuchardt88 Feb 6, 2025

shargon Feb 3, 2025

cschuchardt88 Feb 3, 2025 •

edited

Loading

shargon Feb 3, 2025

cschuchardt88 Feb 3, 2025

cschuchardt88 Feb 5, 2025

shargon commented Feb 6, 2025

shargon Feb 10, 2025

cschuchardt88 Feb 10, 2025

cschuchardt88 Feb 10, 2025

shargon Feb 11, 2025

cschuchardt88 Feb 11, 2025 •

edited

Loading

[Change] DataCache to Match LevelDbStore & RocksDbStore #3709

[Change] DataCache to Match LevelDbStore & RocksDbStore #3709

Conversation

cschuchardt88 commented Feb 3, 2025 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cschuchardt88 Feb 3, 2025 • 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

cschuchardt88 Feb 3, 2025 • 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

shargon commented Feb 6, 2025

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

cschuchardt88 Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

[`Change`] DataCache to Match LevelDbStore & RocksDbStore #3709

[`Change`] DataCache to Match LevelDbStore & RocksDbStore #3709

cschuchardt88 commented Feb 3, 2025 •

edited

Loading

cschuchardt88 Feb 3, 2025 •

edited

Loading

cschuchardt88 Feb 3, 2025 •

edited

Loading

cschuchardt88 Feb 3, 2025 •

edited

Loading

cschuchardt88 Feb 11, 2025 •

edited

Loading