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

[Fixes/Optimizations/Styles] KeyBuilder, StorageKey, StorageItem, MemorySnapshot, MemoryStore, ByteArrayComparer & ByteArrayEqualityComparer #3705

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions src/Neo.Extensions/ByteArrayEqualityComparer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,15 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;

namespace Neo.Extensions
{
public class ByteArrayEqualityComparer : IEqualityComparer<byte[]>
{
public static readonly ByteArrayEqualityComparer Default = new();
public static readonly ByteArrayEqualityComparer Instance = new();
Copy link
Member

Choose a reason for hiding this comment

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

Default should be marked as obsolete because it's public


/// <inheritdoc />
public bool Equals(byte[]? x, byte[]? y)
{
if (ReferenceEquals(x, y)) return true;
Expand All @@ -26,9 +28,12 @@ public bool Equals(byte[]? x, byte[]? y)
return x.AsSpan().SequenceEqual(y.AsSpan());
}

public int GetHashCode(byte[] obj)
{
return obj.XxHash3_32();
}
/// <inheritdoc />
public int GetHashCode([DisallowNull] byte[] obj) =>
obj.XxHash3_32();

/// <inheritdoc />
public int GetHashCode([DisallowNull] object obj) =>
obj is byte[] b ? GetHashCode(b) : 0;
}
}
2 changes: 1 addition & 1 deletion src/Neo/IO/Caching/ECPointCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Neo.IO.Caching
internal class ECPointCache : FIFOCache<byte[], ECPoint>
{
public ECPointCache(int max_capacity)
: base(max_capacity, ByteArrayEqualityComparer.Default)
: base(max_capacity, ByteArrayEqualityComparer.Instance)
{
}

Expand Down
11 changes: 8 additions & 3 deletions src/Neo/Persistence/MemorySnapshot.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ internal MemorySnapshot(MemoryStore store, ConcurrentDictionary<byte[], byte[]>
{
Store = store;
_innerData = innerData;
_immutableData = innerData.ToImmutableDictionary(ByteArrayEqualityComparer.Default);
_writeBatch = new ConcurrentDictionary<byte[], byte[]?>(ByteArrayEqualityComparer.Default);
_immutableData = innerData.ToImmutableDictionary(ByteArrayEqualityComparer.Instance);
_writeBatch = new ConcurrentDictionary<byte[], byte[]?>(ByteArrayEqualityComparer.Instance);
}

public void Commit()
Expand Down Expand Up @@ -64,16 +64,21 @@ public void Put(byte[] key, byte[] value)
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward)
{
keyOrPrefix ??= [];

if (direction == SeekDirection.Backward && keyOrPrefix.Length == 0) yield break;

var comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse;

IEnumerable<KeyValuePair<byte[], byte[]>> records = _immutableData;

if (keyOrPrefix.Length > 0)
records = records
.Where(p => comparer.Compare(p.Key, keyOrPrefix) >= 0);

records = records.OrderBy(p => p.Key, comparer);

foreach (var pair in records)
yield return (pair.Key[..], pair.Value[..]);
yield return new(pair.Key[..], pair.Value[..]);
}

public byte[]? TryGet(byte[] key)
Expand Down
9 changes: 6 additions & 3 deletions src/Neo/Persistence/MemoryStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace Neo.Persistence
/// </summary>
public class MemoryStore : IStore
{
private readonly ConcurrentDictionary<byte[], byte[]> _innerData = new(ByteArrayEqualityComparer.Default);
private readonly ConcurrentDictionary<byte[], byte[]> _innerData = new(ByteArrayEqualityComparer.Instance);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public void Delete(byte[] key)
Expand All @@ -51,16 +51,19 @@ public void Put(byte[] key, byte[] value)
public IEnumerable<(byte[] Key, byte[] Value)> Seek(byte[]? keyOrPrefix, SeekDirection direction = SeekDirection.Forward)
{
keyOrPrefix ??= [];
if (direction == SeekDirection.Backward && keyOrPrefix.Length == 0) yield break;

if (direction == SeekDirection.Backward && keyOrPrefix.Length == 0) yield break;
var comparer = direction == SeekDirection.Forward ? ByteArrayComparer.Default : ByteArrayComparer.Reverse;

IEnumerable<KeyValuePair<byte[], byte[]>> records = _innerData;

if (keyOrPrefix.Length > 0)
records = records
.Where(p => comparer.Compare(p.Key, keyOrPrefix) >= 0);
records = records.OrderBy(p => p.Key, comparer);

foreach (var pair in records)
yield return (pair.Key[..], pair.Value[..]);
yield return new(pair.Key[..], pair.Value[..]);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
Expand Down
36 changes: 17 additions & 19 deletions src/Neo/SmartContract/KeyBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@

#nullable enable

using Neo.Extensions;
using Neo.IO;
using System;
using System.Buffers.Binary;
using System.IO;
using System.Runtime.CompilerServices;

namespace Neo.SmartContract
Expand All @@ -24,7 +24,8 @@ namespace Neo.SmartContract
/// </summary>
public class KeyBuilder
{
private readonly MemoryStream _stream;
private readonly Memory<byte> _cachedData;
private int _keyLen = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

keySizeHint is a hint.
The actual length may exceed the keySizeHint.
So the KeyBuilder cannot use Memory<byte> instead of MemoryStream.

Copy link
Member

Choose a reason for hiding this comment

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

I think the same, but the true is that it will speed up the code a lot of, maybe we should check all the keyBuilds and force to be less than this length

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion this 3 classes should be improved in different PR's because I'm agree with some changes and not with others


/// <summary>
/// Initializes a new instance of the <see cref="KeyBuilder"/> class.
Expand All @@ -34,12 +35,11 @@ public class KeyBuilder
/// <param name="keySizeHint">The hint of the storage key size(including the id and prefix).</param>
public KeyBuilder(int id, byte prefix, int keySizeHint = ApplicationEngine.MaxStorageKeySize)
{
Span<byte> data = stackalloc byte[sizeof(int)];
BinaryPrimitives.WriteInt32LittleEndian(data, id);
_cachedData = new byte[keySizeHint];
BinaryPrimitives.WriteInt32LittleEndian(_cachedData.Span, id);

_stream = new(keySizeHint);
_stream.Write(data);
_stream.WriteByte(prefix);
_keyLen = sizeof(int);
_cachedData.Span[_keyLen++] = prefix;
}

/// <summary>
Expand All @@ -50,7 +50,7 @@ public KeyBuilder(int id, byte prefix, int keySizeHint = ApplicationEngine.MaxSt
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public KeyBuilder Add(byte key)
{
_stream.WriteByte(key);
_cachedData.Span[_keyLen++] = key;
return this;
}

Expand All @@ -62,7 +62,8 @@ public KeyBuilder Add(byte key)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public KeyBuilder Add(ReadOnlySpan<byte> key)
{
_stream.Write(key);
key.CopyTo(_cachedData.Span[_keyLen..]);
_keyLen += key.Length;
return this;
}

Expand Down Expand Up @@ -97,11 +98,11 @@ public KeyBuilder Add(ReadOnlySpan<byte> key)
/// <returns>A reference to this instance after the add operation has completed.</returns>
public KeyBuilder Add(ISerializable key)
{
using (BinaryWriter writer = new(_stream, Utility.StrictUTF8, true))
{
key.Serialize(writer);
writer.Flush();
}
var raw = key.ToArray();

raw.CopyTo(_cachedData[_keyLen..]);
_keyLen += raw.Length;

return this;
}

Expand Down Expand Up @@ -167,15 +168,12 @@ public KeyBuilder AddBigEndian(ulong key)
/// <returns>The storage key.</returns>
public byte[] ToArray()
{
using (_stream)
{
return _stream.ToArray();
}
return _cachedData[.._keyLen].ToArray();
}

public static implicit operator StorageKey(KeyBuilder builder)
{
return new StorageKey(builder.ToArray());
return new StorageKey(builder._cachedData[..builder._keyLen].ToArray());
}
}
}
26 changes: 14 additions & 12 deletions src/Neo/SmartContract/StorageItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ public ReadOnlyMemory<byte> Value
}
set
{
_value = value;
_value = value.ToArray(); // create new memory region
_cache = null;
}
}
Expand All @@ -63,7 +63,7 @@ public StorageItem() { }
/// <param name="value">The byte array value of the <see cref="StorageItem"/>.</param>
public StorageItem(byte[] value)
{
_value = value;
_value = value.AsMemory().ToArray(); // allocate new buffer
}

/// <summary>
Expand Down Expand Up @@ -99,11 +99,13 @@ public void Add(BigInteger integer)
/// <returns>The created <see cref="StorageItem"/>.</returns>
public StorageItem Clone()
{
return new()
var newItem = new StorageItem
{
_value = _value,
_cache = _cache is IInteroperable interoperable ? interoperable.Clone() : _cache
_value = _value.ToArray(), // allocate new buffer
_cache = _cache is IInteroperable interoperable ? interoperable.Clone() : _cache,
};

return newItem;
}

public void Deserialize(ref MemoryReader reader)
Expand All @@ -117,7 +119,7 @@ public void Deserialize(ref MemoryReader reader)
/// <param name="replica">The instance to be copied.</param>
public void FromReplica(StorageItem replica)
{
_value = replica._value;
_value = replica._value.ToArray(); // allocate new buffer. DONT USE INSTANCE
if (replica._cache is IInteroperable interoperable)
{
if (_cache?.GetType() == interoperable.GetType())
Expand All @@ -144,7 +146,7 @@ public void FromReplica(StorageItem replica)
interoperable.FromStackItem(BinarySerializer.Deserialize(_value, ExecutionEngineLimits.Default));
_cache = interoperable;
}
_value = null;
_value = ReadOnlyMemory<byte>.Empty; // garbage collect the previous allocated memory space
return (T)_cache;
}

Expand All @@ -162,7 +164,7 @@ public void FromReplica(StorageItem replica)
interoperable.FromStackItem(BinarySerializer.Deserialize(_value, ExecutionEngineLimits.Default), verify);
_cache = interoperable;
}
_value = null;
_value = ReadOnlyMemory<byte>.Empty; // garbage collect the previous allocated memory space
return (T)_cache;
}

Expand All @@ -178,7 +180,7 @@ public void Serialize(BinaryWriter writer)
public void Set(BigInteger integer)
{
_cache = integer;
_value = null;
_value = ReadOnlyMemory<byte>.Empty; // garbage collect the previous allocated memory space
}

/// <summary>
Expand All @@ -188,7 +190,7 @@ public void Set(BigInteger integer)
public void Set(IInteroperable interoperable)
{
_cache = interoperable;
_value = null;
_value = ReadOnlyMemory<byte>.Empty; // garbage collect the previous allocated memory space
}

public static implicit operator BigInteger(StorageItem item)
Expand All @@ -199,12 +201,12 @@ public static implicit operator BigInteger(StorageItem item)

public static implicit operator StorageItem(BigInteger value)
{
return new StorageItem(value);
return new(value);
}

public static implicit operator StorageItem(byte[] value)
{
return new StorageItem(value);
return new(value);
}
}
}
28 changes: 14 additions & 14 deletions src/Neo/SmartContract/StorageKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public sealed record StorageKey
/// </summary>
public ReadOnlyMemory<byte> Key { get; init; }

private byte[]? _cache;
private Memory<byte> _cache;

// NOTE: StorageKey is readonly, so we can cache the hash code.
private int _hashCode = 0;
Expand All @@ -49,9 +49,9 @@ public StorageKey()
/// <param name="cache">The cached byte array. NOTE: It must be read-only and can be modified by the caller.</param>
internal StorageKey(byte[] cache)
{
_cache = cache;
Id = BinaryPrimitives.ReadInt32LittleEndian(cache);
Key = cache.AsMemory(sizeof(int));
_cache = cache.AsMemory().ToArray(); // allocate new buffer
Id = BinaryPrimitives.ReadInt32LittleEndian(_cache.Span);
Key = _cache[sizeof(int)..].ToArray(); // allocate new buffer. NOTE: DONT USE POINTERS HERE
}

/// <summary>
Expand All @@ -68,10 +68,10 @@ internal StorageKey(ReadOnlySpan<byte> cache) : this(cache.ToArray()) { }
/// <returns>The created search prefix.</returns>
public static byte[] CreateSearchPrefix(int id, ReadOnlySpan<byte> prefix)
{
var buffer = new byte[sizeof(int) + prefix.Length];
Span<byte> buffer = stackalloc byte[sizeof(int) + prefix.Length];
BinaryPrimitives.WriteInt32LittleEndian(buffer, id);
prefix.CopyTo(buffer.AsSpan(sizeof(int)));
return buffer;
prefix.CopyTo(buffer[sizeof(int)..]);
return buffer.ToArray();
}

public bool Equals(StorageKey? other)
Expand All @@ -92,22 +92,22 @@ public override int GetHashCode()

public byte[] ToArray()
{
if (_cache is null)
if (_cache is { IsEmpty: true })
{
_cache = new byte[sizeof(int) + Key.Length];
BinaryPrimitives.WriteInt32LittleEndian(_cache, Id);
Key.CopyTo(_cache.AsMemory(sizeof(int)));
_cache = new byte[sizeof(int) + Key.Length]; // allocate new buffer
BinaryPrimitives.WriteInt32LittleEndian(_cache.Span, Id);
Key.CopyTo(_cache[sizeof(int)..]);
}
return _cache;
return _cache.ToArray(); // allocate new buffer
Copy link
Member

Choose a reason for hiding this comment

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

Every time that is invoked a new buffer is created, so the "cache" is not complete, and slower

Copy link
Member Author

Choose a reason for hiding this comment

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

How is it slower? No memory is moved. Its like creating a Span on byte[] nothing moves.

Copy link
Member Author

@cschuchardt88 cschuchardt88 Feb 6, 2025

Choose a reason for hiding this comment

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

So my way is faster

NEW

Method Mean Error StdDev Median Min Max
TestDataCacheAdd 0.0042 ns 0.0087 ns 0.0081 ns 0.0000 ns 0.0000 ns 0.0283 ns
TestDataCacheUpdate 0.0062 ns 0.0080 ns 0.0075 ns 0.0016 ns 0.0000 ns 0.0217 ns
TestDataCacheDelete 36.0960 ns 0.2631 ns 0.2461 ns 36.1001 ns 35.6243 ns 36.5847 ns
TestDataCacheGet 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns 0.0000 ns

OLD

Method Mean Error StdDev Median Min Max
TestDataCacheAdd 0.0105 ns 0.0162 ns 0.0152 ns 0.0062 ns 0.0000 ns 0.0533 ns
TestDataCacheUpdate 0.0011 ns 0.0044 ns 0.0041 ns 0.0000 ns 0.0000 ns 0.0159 ns
TestDataCacheDelete 36.5685 ns 0.5138 ns 0.4807 ns 36.4161 ns 35.9531 ns 37.9400 ns
TestDataCacheGet 0.0099 ns 0.0187 ns 0.0175 ns 0.0009 ns 0.0000 ns 0.0582 ns

}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator StorageKey(byte[] value) => new(value);

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator StorageKey(ReadOnlyMemory<byte> value) => new(value.Span);
public static implicit operator StorageKey(ReadOnlyMemory<byte> value) => new(value.ToArray());

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static implicit operator StorageKey(ReadOnlySpan<byte> value) => new(value);
public static implicit operator StorageKey(ReadOnlySpan<byte> value) => new(value.ToArray());
}
}
2 changes: 1 addition & 1 deletion src/Plugins/MPTTrie/Cryptography/MPTTrie/Trie.Proof.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ public bool TryGetProof(byte[] key, out HashSet<byte[]> proof)
throw new ArgumentException("could not be empty", nameof(key));
if (path.Length > Node.MaxKeyLength)
throw new ArgumentException("exceeds limit", nameof(key));
proof = new HashSet<byte[]>(ByteArrayEqualityComparer.Default);
proof = new HashSet<byte[]>(ByteArrayEqualityComparer.Instance);
return GetProof(ref root, path, proof);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ namespace Neo.Cryptography.MPTTrie.Tests
{
class TestSnapshot : IStoreSnapshot
{
public Dictionary<byte[], byte[]> store = new Dictionary<byte[], byte[]>(ByteArrayEqualityComparer.Default);
public Dictionary<byte[], byte[]> store = new Dictionary<byte[], byte[]>(ByteArrayEqualityComparer.Instance);

private byte[] StoreKey(byte[] key)
{
Expand Down
4 changes: 2 additions & 2 deletions tests/Neo.Extensions.Tests/UT_ByteArrayEqualityComparer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public void TestEqual()
{
var a = new byte[] { 1, 2, 3, 4, 1, 2, 3, 4, 5 };
var b = new byte[] { 1, 2, 3, 4, 1, 2, 3, 4, 5 };
var check = ByteArrayEqualityComparer.Default;
var check = ByteArrayEqualityComparer.Instance;

Assert.IsTrue(check.Equals(a, a));
Assert.IsTrue(check.Equals(a, b));
Expand All @@ -45,7 +45,7 @@ public void TestGetHashCode()
{
var a = new byte[] { 1, 2, 3, 4, 1, 2, 3, 4, 5 };
var b = new byte[] { 1, 2, 3, 4, 1, 2, 3, 4, 5 };
var check = ByteArrayEqualityComparer.Default;
var check = ByteArrayEqualityComparer.Instance;

Assert.AreEqual(check.GetHashCode(a), check.GetHashCode(b));
Assert.AreNotEqual(check.GetHashCode(a), check.GetHashCode(b.Take(8).ToArray()));
Expand Down