Optimizing VM

[Turalchik]

This pull request brings the asymptotic complexity of the opcodes closer to https://github.com/nspcc-dev/neo-go. Basically, this PR presents refcounting optimizations in opcodes, in which items are removed from the stack, which shortly after that will be there on the stack.

[codecov]

Codecov Report

❌ Patch coverage is 86.04651% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.47%. Comparing base (348476f) to head (5ab6f21).

Files with missing lines	Patch %	Lines
src/Neo.VM/JumpTable/JumpTable.Compound.cs	79.27%	19 Missing and 4 partials ⚠️
src/Neo.VM/Slot.cs	94.44%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #575      +/-   ##
==========================================
- Coverage   85.61%   85.47%   -0.14%     
==========================================
  Files          41       41              
  Lines        2606     2720     +114     
  Branches      338      345       +7     
==========================================
+ Hits         2231     2325      +94     
- Misses        287      304      +17     
- Partials       88       91       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Jim8y]

SETITEM now removes the new value with PopNoRef() before validating the array index. If the array branch then throws the catchable out-of-range exception, the value has already been removed from the evaluation stack but its stack reference is never removed. A script can catch that exception and continue with an inflated ReferenceCounter.Count, which can later trigger incorrect MaxStackSize failures.\n\nI reproduced this with a TRY/CATCH around an out-of-range array SETITEM: the same test passes on base with count 0 after the catch, but this PR leaves count 1. Please either keep the normal Pop() until all catchable validation is complete, or remove the value reference on every failure path before rethrowing.

[shargon]

It's better to use internal because these calls can desynchronize the reference counter

[Jim8y]

Thanks for updating the reference-counter helpers to internal and fixing the array out-of-range path. I found one remaining blocker in the Buffer SETITEM branch: the value is removed before validating the buffer index, then removed again on the out-of-range catchable path. A handled script such as TRY; PUSH1; NEWBUFFER; PUSH2; PUSH0; SETITEM; DROP; ENDTRY halts with ReferenceCounter.Count == -1 instead of 0. I reproduced it by adding a focused UT_ReferenceCounter test and running dotnet test tests/Neo.VM.Tests/Neo.VM.Tests.csproj --filter TestSetItemBufferCatchableIndexFailureBalancesReferences --verbosity minimal.

[Jim8y]

Rechecked the latest head. The Buffer SETITEM catchable out-of-range path now keeps the reference counter balanced, and the new JSON case covers it. I also ran Determining projects to restore...
All projects are up-to-date for restore.
Neo.VM -> /private/tmp/neo-vm-review-575/src/Neo.VM/bin/Debug/net10.0/Neo.VM.dll
Neo.VM.Tests -> /private/tmp/neo-vm-review-575/tests/Neo.VM.Tests/bin/Debug/net10.0/Neo.VM.Tests.dll
Test run for /private/tmp/neo-vm-review-575/tests/Neo.VM.Tests/bin/Debug/net10.0/Neo.VM.Tests.dll (.NETCoreApp,Version=v10.0)
A total of 1 test files matched the specified pattern.

Passed! - Failed: 0, Passed: 156, Skipped: 0, Total: 156, Duration: 647 ms - Neo.VM.Tests.dll (net10.0) successfully.

[Turalchik]

On neo-project/neo#4546 there was a question about optimizing the ROT opcode, I executed benchmarks for this opcode from this file https://github.com/neo-project/neo-vm/blob/ed0eb66359fd1d76ddb5be91cc5a703ddc714a18/benchmarks/Neo.VM.Benchmarks/OpCodes/Benchmarks_StackOps.cs and I got the following results

Old ROT with two swap:

Method	Mean	Error	StdDev	Median	Gen0	Gen1	Gen2	Allocated
ROT_Shallow_3x1000	293.0 us	9.73 us	25.65 us	286.9 us	-	-	-	139.64 KB
ROT_Deep_100x1000	305.2 us	6.08 us	11.87 us	303.0 us	-	-	-	152.3 KB
ROT_Multiple_50x1000	9,214.9 us	614.24 us	1,732.46 us	8,450.0 us	1000.0000	1000.0000	1000.0000	5931.26 KB

New ROT using roll:

Method	Mean	Error	StdDev	Gen0	Gen1	Gen2	Allocated
ROT_Shallow_3x1000	259.9 us	5.14 us	7.04 us	-	-	-	139.64 KB
ROT_Deep_100x1000	295.7 us	5.87 us	12.00 us	-	-	-	152.3 KB
ROT_Multiple_50x1000	8,342.5 us	279.26 us	764.47 us	1000.0000	1000.0000	1000.0000	5931.26 KB

[AnnaShaleva]

LGTM. The code is compatible with NeoGo. Let's merge this PR firstly, it contains optimisations only, and then we'll rebase the #571 onto fresh master.

@neo-project/core please, review.

[ajara87]

lgtm

[superboyiii]

I'm testing data compatibility...

[superboyiii]

@AnnaShaleva @Turalchik Data is compatible.

[shargon]

@AnnaShaleva is this required for 3.10.1?

[AnnaShaleva]

is this required for 3.10.1?

Not strictly required. But I'd suggest to include it to v3.10.1 to proceed with dynamic opcodes step-by step. This PR doesn't contain any incompatible changes.

[superboyiii]

Merge? Then we can begin to release vm.

[AnnaShaleva]

Merge? Then we can begin to release vm.

Yes.

[ajara87]

@shargon could you merge?

[shargon]

In my opinion this should wait to next release, it's not critical and it's related to dynamic prices

[vncoelho]

I believe this PR should be merged for N4 only, and battle tested for 3.11.0 together with dynamic opcodes price

[superboyiii]

VM v3.10.1 is released, it's good to merge now.