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] unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>) and remove the use of unsafe #3492

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

[Fix] unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>) and remove the use of unsafe #3492

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7cf2e7f
fix: unalignment load in ReadOnlySpan<byte>.NotZero
nan01ab Sep 18, 2024
37f78c8
Update .gitignore
shargon Sep 18, 2024
2031460
use SequenceEqual to compare bytes for better performace
nan01ab Sep 19, 2024
a4ef07d
use ContainsAnyExcept to get better performace and simply code
nan01ab Sep 19, 2024
6315b28
use ContainsAnyExcept to get better performance and simply code
nan01ab Sep 20, 2024
cf362b7
use ContainsAnyExcept to get better performance and simply code
nan01ab Sep 20, 2024
d3fc7d2
fix comment
nan01ab Sep 20, 2024
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,3 +257,5 @@ paket-files/
PublishProfiles
/.vscode
launchSettings.json
/coverages
**/.DS_Store
2 changes: 1 addition & 1 deletion src/Neo.VM/Types/ByteString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ internal bool Equals(StackItem? other, ref uint limits)
public override bool GetBoolean()
{
if (Size > Integer.MaxSize) throw new InvalidCastException();
return Unsafe.NotZero(GetSpan());
return GetSpan().NotZero();
Copy link
Member

Choose a reason for hiding this comment

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

The implementation is quite straightforward, but GetBoolean is frequently-used in VM, thus we have to check mainnet/testnet networks for compatibility. There should be no issues, but let's do it just in case.

Copy link
Member

Choose a reason for hiding this comment

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

@superboyiii could you check it?

}

public override int GetHashCode()
Expand Down
42 changes: 0 additions & 42 deletions src/Neo.VM/Unsafe.cs
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions src/Neo.VM/Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,23 @@ static Utility()
StrictUTF8.EncoderFallback = EncoderFallback.ExceptionFallback;
}

public static bool NotZero(this ReadOnlySpan<byte> x)
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be moved to the extensions library. For ReadOnlySpans.cs

Copy link
Author

Choose a reason for hiding this comment

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

This needs to be moved to the extensions library. For ReadOnlySpans.cs

Move it with the rest of the content together?

Copy link
Member

Choose a reason for hiding this comment

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

Just that extension method

{
int i;
for (i = 0; i + 7 < x.Length; i += 8)
{
if ((x[i] | x[i + 1] | x[i + 2] | x[i + 3] | x[i + 4] | x[i + 5] | x[i + 6] | x[i + 7]) != 0)
Copy link
Member

Choose a reason for hiding this comment

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

This won't be faster as previous version

Copy link
Author

Choose a reason for hiding this comment

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

This won't be faster as previous version

This won't be faster as previous version

The performance has been optimized,

return true;
}

for (; i < x.Length; i++)
{
if (x[i] != 0) return true;
}
Copy link
Member

@AnnaShaleva AnnaShaleva Sep 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a difference in the existing approach comparing to the old code: new code starts comparison from the head of x, and the old code started comparison from the tail of x. GetBoolean is kind of a hot-path code in the VM, thus it would be interesting to see how this bugfix affects the performance (time) of DB restore from dump for mainnet/testnet.

Copy link
Author

Choose a reason for hiding this comment

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

The old code started comparison from the head of x too.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this work?

foreach (var b in Span)
    if (b != 0) return true;

Copy link
Author

@nan01ab nan01ab Sep 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There's a difference in the existing approach comparing to the old code: new code starts comparison from the head of x, and the old code started comparison from the tail of x. GetBoolean is kind of a hot-path code in the VM, thus it would be interesting to see how this bugfix affects the performance (time) of DB restore from dump for mainnet/testnet.

There's a difference in the existing approach comparing to the old code: new code starts comparison from the head of x, and the old code started comparison from the tail of x. GetBoolean is kind of a hot-path code in the VM, thus it would be interesting to see how this bugfix affects the performance (time) of DB restore from dump for mainnet/testnet.

Now the performance has been optimized(by ContainsAnyExcept because it can use SIMD).

Intel Core i7-8750H CPU 2.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK 8.0.203
  [Host]     : .NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2


| Method            | length | Mean     | Error     | StdDev    |
|------------------ |------- |---------:|----------:|----------:|
| LongPointer       | 10     | 4.221 ns | 0.0285 ns | 0.0222 ns |
| ContainsAnyExcept | 10     | 6.324 ns | 0.0619 ns | 0.0548 ns |
| LongPointer       | 20     | 5.567 ns | 0.0756 ns | 0.0670 ns |
| ContainsAnyExcept | 20     | 6.820 ns | 0.0935 ns | 0.0829 ns |
| LongPointer       | 30     | 7.218 ns | 0.0847 ns | 0.0751 ns |
| ContainsAnyExcept | 30     | 6.545 ns | 0.0514 ns | 0.0429 ns |

The new implementation performs better in some cases, especially if the bytes is relatively long.


return false;
}

public static bool TryGetString(this ReadOnlySpan<byte> bytes, out string? value)
{
try
Expand Down
33 changes: 0 additions & 33 deletions tests/Neo.VM.Tests/UT_Unsafe.cs
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions tests/Neo.VM.Tests/UT_Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,22 @@ public void TestModInverseTest()

Assert.AreEqual(new BigInteger(52), new BigInteger(19).ModInverse(141));
}

[TestMethod]
public void TestNotZero()
{
Assert.IsFalse(new ReadOnlySpan<byte>(System.Array.Empty<byte>()).NotZero());
Assert.IsFalse(new ReadOnlySpan<byte>(new byte[4]).NotZero());
Assert.IsFalse(new ReadOnlySpan<byte>(new byte[7]).NotZero());
Assert.IsFalse(new ReadOnlySpan<byte>(new byte[8]).NotZero());
Assert.IsFalse(new ReadOnlySpan<byte>(new byte[9]).NotZero());
Assert.IsFalse(new ReadOnlySpan<byte>(new byte[11]).NotZero());

Assert.IsTrue(new ReadOnlySpan<byte>(new byte[4] { 0x00, 0x00, 0x00, 0x01 }).NotZero());
Assert.IsTrue(new ReadOnlySpan<byte>(new byte[7] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 }).NotZero());
Assert.IsTrue(new ReadOnlySpan<byte>(new byte[8] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 }).NotZero());
Assert.IsTrue(new ReadOnlySpan<byte>(new byte[9] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00 }).NotZero());
Assert.IsTrue(new ReadOnlySpan<byte>(new byte[11] { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 }).NotZero());
}
}
}
Loading