-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>) and remove the use of unsafe
#3492
base: master
Are you sure you want to change the base?
Conversation
[fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>)
[fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>)[Fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>)
src/Neo.VM/Utility.cs
Outdated
for (; i < x.Length; i++) | ||
{ | ||
if (x[i] != 0) return true; | ||
} |
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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 ofx
.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 ofx
.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.
@@ -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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
@@ -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(); |
There was a problem hiding this comment.
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?
@@ -27,6 +27,23 @@ static Utility() | |||
StrictUTF8.EncoderFallback = EncoderFallback.ExceptionFallback; | |||
} | |||
|
|||
public static bool NotZero(this ReadOnlySpan<byte> x) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just that extension method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like this method to be moved to Neo.Extensions
library
src/Neo.VM/Utility.cs
Outdated
for (; i < x.Length; i++) | ||
{ | ||
if (x[i] != 0) return true; | ||
} |
There was a problem hiding this comment.
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;
src/Neo.VM/Utility.cs
Outdated
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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,
{ | ||
int len = x.Length; | ||
if (len == 0) return false; | ||
fixed (byte* xp = x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain more how this could be wrong in different platform? could you write a unit test for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align of long
is 8, and align of byte
is 1, so byte*
may not be aligned to 8.
[Fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>)[Fix]
unalignment memory load in Neo.VM Unsafe.NotZero(ReadOnlySpan<byte>) and remove the use of unsafe
netstandard2.1 problems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix to make compatible with netstandard2.1
. And Move NotZero
method to Neo.Extensions
library.
Error: /Users/runner/work/neo/neo/src/Neo.VM/Utility.cs(38,22): error CS1061: 'ReadOnlySpan<byte>' does not contain a definition for 'IndexOfAnyExcept' and no accessible extension method 'IndexOfAnyExcept' accepting a first argument of type 'ReadOnlySpan<byte>' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/neo/neo/src/Neo.VM/Neo.VM.csproj::TargetFramework=netstandard2.1]
🥲 |
Description
The
Unsafe.NotZero(ReadOnlySpan<byte>)
method inNeo.VM
uselong*
to load data fromReadOnlySpan<byte>
for better performance, but this approach may result in misaligned memory access. This approach is no problem on x86 CPUs, but it may cause bug on some RISC CPUs(for example, some ARM CPUs).Fixes # (issue)
Type of change
Test Configuration:
Checklist: