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

Some of VM benchmark scripts are improperly constructed #3523

Open
AnnaShaleva opened this issue Oct 10, 2024 · 6 comments
Open

Some of VM benchmark scripts are improperly constructed #3523

AnnaShaleva opened this issue Oct 10, 2024 · 6 comments
Labels
bug Used to tag confirmed bugs

Comments

@AnnaShaleva
Copy link
Member

Describe the bug
Some of benchmarks added in #3512 result in FAULTed VM. It happens because these benchmarks are old, they use NEWBUFFER with too large argument:

// PUSHINT32 1048575
// NEWBUFFER

Whereas since neo-project/neo-vm#514 VM limit for stackitem size is 2*maxUint16. The results corresponding with faulty benchmarks posted in #3512 (comment) are invalid.

The following benchmarks must be rewritten and fixed:

  • NeoIssue2723
  • PoC_NewBuffer
  • PoC_Cat
  • PoC_Left
  • PoC_Right
  • PoC_Substr
  • PoC_MemCpy

To Reproduce

  1. Take the following code that runs benchmarks:

    private static void Run(string poc)
    {
    byte[] script = Convert.FromBase64String(poc);
    using ExecutionEngine engine = new();
    engine.LoadScript(script);
    engine.Execute();
    Debug.Assert(engine.State == VMState.HALT);
    }

  2. Replace Debug.Assert(engine.State == VMState.HALT); statement with the following:

            if (engine.State != VMState.HALT)
            {
                throw new Exception(engine.State.ToString());
            }
  1. See failing benchmarks, for example:
// Found 1 benchmarks:
//   Benchmarks_Correct.PoC_Cat: DefaultJob

// **************************
// Benchmark: Benchmarks_Correct.PoC_Cat: DefaultJob
// *** Execute ***
// Launch: 1 / 1
// Execute: dotnet 7443c395-9aef-4a0e-806f-9e725e575501.dll --anonymousPipes 109 110 --benchmarkName Neo.VM.Benchmark.Benchmarks_Correct.PoC_Cat --job Default --benchmarkId 0 in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/bin/Release/net8.0/7443c395-9aef-4a0e-806f-9e725e575501/bin/Release/net8.0
// Failed to set up high priority (Permission denied). In order to run benchmarks with high priority, make sure you have the right permissions.
// BeforeAnythingElse

// Benchmark Process Environment Information:
// BenchmarkDotNet v0.13.12
// Runtime=.NET 8.0.3 (8.0.324.11423), X64 RyuJIT AVX2
// GC=Concurrent Workstation
// HardwareIntrinsics=AVX2,AES,BMI1,BMI2,FMA,LZCNT,PCLMUL,POPCNT VectorSize=256
// Job: DefaultJob

OverheadJitting  1: 1 op, 255355.00 ns, 255.3550 us/op

System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.Exception: FAULT
   at Neo.VM.Benchmark.Benchmarks_Correct.Run(String poc) in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/Benchmarks.Correct.cs:line 52
   at Neo.VM.Benchmark.Benchmarks_Correct.PoC_Cat() in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/Benchmarks.Correct.cs:line 40
   at BenchmarkDotNet.Autogenerated.Runnable_0.WorkloadActionNoUnroll(Int64 invokeCount) in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/bin/Release/net8.0/7443c395-9aef-4a0e-806f-9e725e575501/7443c395-9aef-4a0e-806f-9e725e575501.notcs:line 311
   at BenchmarkDotNet.Engines.Engine.RunIteration(IterationData data)
   at BenchmarkDotNet.Engines.EngineFactory.Jit(Engine engine, Int32 jitIndex, Int32 invokeCount, Int32 unrollFactor)
   at BenchmarkDotNet.Engines.EngineFactory.CreateReadyToRun(EngineParameters engineParameters)
   at BenchmarkDotNet.Autogenerated.Runnable_0.Run(IHost host, String benchmarkName) in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/bin/Release/net8.0/7443c395-9aef-4a0e-806f-9e725e575501/7443c395-9aef-4a0e-806f-9e725e575501.notcs:line 176
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at BenchmarkDotNet.Autogenerated.UniqueProgramName.AfterAssemblyLoadingAttached(String[] args) in /home/anna/Documents/GitProjects/neo-project/neo/benchmarks/Neo.VM.Benchmarks/bin/Release/net8.0/7443c395-9aef-4a0e-806f-9e725e575501/7443c395-9aef-4a0e-806f-9e725e575501.notcs:line 57

Expected behavior
All benchmarks must not use NEWBUFFER with invalid (too large) argument. All benchmarks should end in HALT VM state except those benchmarks that use endless cycles. Benchmarks with endless cycles must be checked against resulting execution error: the error must be caused by the end of GAS.

Platform:

@AnnaShaleva AnnaShaleva added the bug Used to tag confirmed bugs label Oct 10, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Oct 10, 2024

All benchmarks should end in HALT VM state

is not necessary, its benchmark, we just need to know how long it could run, even if it faults, actually worse case fault is normal cause it runs until it exhaust the gas, instead of reaching a halt states.

@AnnaShaleva
Copy link
Member Author

is not necessary

It is necessary for those benchmarks that are expected to end in HALT state.

it runs until it exhaust the gas,

As I said in the issue, for these benchmarks we need to check the exact exception message, i.e. to check that the failure reason is exactly run out of GAS. Otherwise, this benchmark is invalid.

instead of reaching a halt states.

Well, half of benchmarks from #3512 fail on something like third instruction execution, e.g.:

        [Benchmark]
        public void PoC_Cat()
        {
            // INITSLOT 0100
            // PUSHINT32 1048575
            // NEWBUFFER            <--- FAULTs VM execution here.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 10, 2024

we added some limits to the vm.since those pocs. any way, you do it if you think its the best, these pocs are no.longer that importsnd to me, they helped me to rise the attention of this issue already, we can focus on benchmarking opcodes.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 11, 2024

BTW, those are not benchmark for opcodes, but PoCs, opcode benchmarks are different.

@roman-khimov
Copy link
Contributor

Yeah. And they're broken. They are not proving anything. If you like them this way --- well, ok.

@Jim8y
Copy link
Contributor

Jim8y commented Oct 14, 2024

Yeah. And they're broken. They are not proving anything. If you like them this way --- well, ok.

Its not about i like it or not, its that all of those PoCs are meaningless for me now, what is important for me is the precise benchmark for opcodes, so, you can say those PoC as a bug, or you can also ignore them, i am ok with either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to tag confirmed bugs
Projects
None yet
Development

No branches or pull requests

3 participants