ReferenceCounterV2
#3526
ReferenceCounterV2
#3526
Conversation
|
its not yet being used right, no wander the bench has no change. |
No, if you like it, i can start with the selection according to the height |
I could not really understand the reason of this pr, but after i replace RC with this version, optimization for some pocs are obvious, but for the rest is not that clear: V1 // | Method | Mean | Error | StdDev | Median | V2 // | Method | Mean | Error | StdDev | |
|
Is not possible to be slower than the previous case in any of them xD |
src/Neo.VM/ReferenceCounterV2.cs
Outdated
| /// <param name="count">Number of similar entries</param> | ||
| public void AddStackReference(StackItem item, int count = 1) | ||
| { | ||
| Count += count; |
There was a problem hiding this comment.
As @roman-khimov suggested, take a look at compound types handling in https://github.com/nspcc-dev/neo-go/blob/b8a65d3c37cfa429b8a5e135422bb1fa78762056/pkg/vm/ref_counter.go#L15.
There was a problem hiding this comment.
We've got some tests as well: https://github.com/nspcc-dev/neo-go/blob/b8a65d3c37cfa429b8a5e135422bb1fa78762056/pkg/vm/ref_counter_test.go
src/Neo.VM/ReferenceCounterV2.cs
Outdated
| throw new InvalidOperationException("Reference was not added before"); | ||
| } | ||
|
|
||
| Count--; |
There was a problem hiding this comment.
Hi, what if you are removing a compound type? for instance, you have an array with 2047 sub items, then you remove the array, the rc will only remove one reference, and the vm will still be full.
There was a problem hiding this comment.
[TestMethod]
public void TestNewArrayThenDrop()
{
using ScriptBuilder sb = new();
sb.EmitPush(2040);
sb.Emit(OpCode.NEWARRAY);
sb.Emit(OpCode.DROP);
using ExecutionEngine engine = new();
engine.LoadScript(sb.ToArray());
Assert.AreEqual(0, engine.ReferenceCounter.Count);
Assert.AreEqual(VMState.HALT, engine.Execute());
Assert.AreEqual(0, engine.ReferenceCounter.Count);
}Assert.AreEqual failed. Expected:<0>. Actual:<2040>.
at Neo.Test.UT_ReferenceCounter.TestNewArrayThenDrop() in /Users/jinghuiliao/git/neo/tests/Neo.VM.Tests/UT_ReferenceCounter.cs:line 257
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
op:[0000]PUSHINT16 []
op:[0003]NEWARRAY [Integer(2040)]
op:[0004]DROP [Array(2040)]
op:[0005] []
There was a problem hiding this comment.
[TestMethod]
public void TestNewArrayThenDrop()
{
using ScriptBuilder sb = new();
sb.EmitPush(2040);
sb.Emit(OpCode.NEWARRAY);
sb.Emit(OpCode.DROP);
sb.EmitPush(10);
sb.Emit(OpCode.NEWARRAY);
using ExecutionEngine engine = new();
engine.LoadScript(sb.ToArray());
Assert.AreEqual(0, engine.ReferenceCounter.Count);
var res = engine.Execute();
Assert.AreEqual(0, engine.ReferenceCounter.Count);
Assert.AreEqual(VMState.HALT, res);
}Assert.AreEqual failed. Expected:<0>. Actual:<2051>.
at Neo.Test.UT_ReferenceCounter.TestNewArrayThenDrop() in /Users/jinghuiliao/git/neo/tests/Neo.VM.Tests/UT_ReferenceCounter.cs:line 259
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
op:[0000]PUSHINT16 []
op:[0003]NEWARRAY [Integer(2040)]
op:[0004]DROP [Array(2040)]
op:[0005]PUSH10 []
op:[0006]NEWARRAY [Integer(10)]
There was a problem hiding this comment.
Hi, what if you are removing a compound type? for instance, you have an array with 2047 sub items, then you remove the array, the rc will only remove one reference, and the vm will still be full.
yes, we need to propagate this change
There was a problem hiding this comment.
Assert.AreEqual(0, engine.ReferenceCounter.Count);
Assert.AreEqual(VMState.HALT, res);it's 11 in the previous version
There was a problem hiding this comment.
[TestMethod] public void TestNewArrayThenDrop() { using ScriptBuilder sb = new(); sb.EmitPush(2040); sb.Emit(OpCode.NEWARRAY); sb.Emit(OpCode.DROP); using ExecutionEngine engine = new(); engine.LoadScript(sb.ToArray()); Assert.AreEqual(0, engine.ReferenceCounter.Count); Assert.AreEqual(VMState.HALT, engine.Execute()); Assert.AreEqual(0, engine.ReferenceCounter.Count); }Assert.AreEqual failed. Expected:<0>. Actual:<2040>. at Neo.Test.UT_ReferenceCounter.TestNewArrayThenDrop() in /Users/jinghuiliao/git/neo/tests/Neo.VM.Tests/UT_ReferenceCounter.cs:line 257 at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor) at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
op:[0000]PUSHINT16 [] op:[0003]NEWARRAY [Integer(2040)] op:[0004]DROP [Array(2040)] op:[0005] []
this use the previous version also
|
@shargon, as far as I understood in our last call, there will be nothing like |
no, twice is two now, faster, but same reference is two items |
[TestMethod]
public void TestMultiNewArrayThenDrop()
{
using ScriptBuilder sb = new();
sb.EmitPush(1000);
sb.Emit(OpCode.NEWARRAY);
sb.Emit(OpCode.DROP);
sb.EmitPush(1000);
sb.Emit(OpCode.NEWARRAY);
sb.Emit(OpCode.DROP);
sb.EmitPush(1000);
sb.Emit(OpCode.NEWARRAY);
sb.Emit(OpCode.DROP);
using ExecutionEngine engine = new();
engine.LoadScript(sb.ToArray());
Assert.AreEqual(0, engine.ReferenceCounter.Count);
var res = engine.Execute();
Assert.AreEqual(0, engine.ReferenceCounter.Count);
Assert.AreEqual(VMState.HALT, res);
}Assert.AreEqual failed. Expected:<0>. Actual:<3001>. Your code will keep growing the RC for compound type, it never clears it. This will make contract impossible to use compoundtype, since there is < 2048 subitems in total can be ever generated during the whole execution. |
Waiting for #3549 |
|
@shargon please follow up with this work. |
Jim8y
added
Need Update
and removed
Blocked
| { | ||
| if (!_stack.Remove(item)) | ||
| { | ||
| throw new InvalidOperationException("Reference was not added before"); |
There was a problem hiding this comment.
This version is not compatible with circular references, but at the same it works like a protection
|
@shargon test fails |
Yes, i know, is because we don't allow circulsr reference with this reference counter, i will fix it if the logic seems good to you |
|
Wait I will fix somethings |
…into remove-tarjan-v2
|
Still having some issues with some UTs |
|------- |---------:|--------:|--------:| | V2 | 216.6 ms | 1.28 ms | 1.14 ms | | V1 | 235.8 ms | 3.18 ms | 2.48 ms |
| /// <param name="count">Number of similar entries</param> | ||
| public void AddStackReference(StackItem item, int count = 1) | ||
| { | ||
| for (var x = 0; x < count; x++) |
There was a problem hiding this comment.
this actually is not necessary, currently we process his when we create the compound type. For instance, new array will pass the rc and add all its subitems.
There was a problem hiding this comment.
I know it, but it's hard to explain, there are cases where the items are removed without exists.
- create a map with items without push the map to stack (unit test)
- Push the map to stack
- Add the map to static variables
- RET
maybe is not this case that i remember, but when the stack was clean, I received "item not found" error
| } | ||
| } | ||
|
|
||
| private int ReferenceEqualsIndexOf(StackItem item, int index = 0) |
There was a problem hiding this comment.
It's complexity is O(n), may not fast in some cases.
There was a problem hiding this comment.
Yes, this version is slower than I expected, I tried with different ways, and still slower than expected
Description
Close #3517
Type of change
How Has This Been Tested?
Test Configuration:
Checklist: