Reference tracing

[erikzhang]

No description provided.

[shargon]

At simple view the speed is very close to my solution. So according to the speed both solution are a great improve!

TestWithArray
#232 - 00:00:01.2240960
#230 - 00:00:01.0538888

TestWithPUSHPOP
#232 - 00:00:00.0169211
#230 - 00:00:00.0165896

But the unique think that I don't like is that we lose the possibility of adding or removing items in CompoundType outside neo-vm.

I think that this solution is harder to follow and I think that we lose the possibility of alter outside the library the Stacks.

[lock9]

I have to confess: I think #230 is more clear about the memory limits and this code is harder to read and maintain.
Also, I noticed some new classes but I didn't see new tests. There is no code coverage enabled, I can't verify if the new code is being tested or not.

[erikzhang]

I can add a new method:

bool PushNew(CompoundType compound);

Then you can create CompoundType from outside and push it to the stack. The method will recompute the references for it.

[erikzhang]

I don't like to create arrays with ReservedMemory. This looks strange. In my scenario, ReferenceTracing is internal and transparent to the user.

[erikzhang]

We need some UT.

[erikzhang]

I added UT, and tested it with circular-reference and self-reference. It works perfectly.

[lock9]

Hi Erik,
Can we have a quick discussion on wechat? Me and Shargon don’t like this PR very much. Not sure if we are wrong or there is something missing. I don’t agree with this Pr, sorry 😅

[erikzhang]

The discussion should be transparent. So the best place is on Github. You can express your views here, without having to use WeChat.

[erikzhang]

In my opinion, this PR performance is the same as #230. And the use method is the same as before. The entire mechanism is transparent to the user. So why don't you like it?

[lock9]

I don’t like it in comparison to the other PR. I can’t see this Pr solving the memory limit issue(current method is “unfair”). I believe there are performance improvements, but aren’t the objects immutable with this PR? Can we add new items to the array or do we need to create a new one?
The biggest issue is that not only this PR will be very hard to maintain and debug, but it also doesn’t solve the “explicit memory usage” problem

[lock9]

The other PR is counting bytes precisely and in the same way for all objects. This one seems to have some tricks that only specialists can understand the reason

[shargon]

I don't like it for some reasons:

• We loose a lot of public methods like for example Add , Insert in Array .. most of the logic inside the StackItems.
• We loose the availability to insert items into certain ExecutionContext, we must use the centralized Push Method.
• We need to use AppendItem for adding something to an Array I prefer to construct the item with the reference and use it as before.
• The code is too much hard to follow, we lose readability.
• Hard to change the logic to other mechanism (maybe we can study the limit by memory instead of counting).

[igormcoelho]

I'm joining this discussion with a little delay, couldnt properly check all changes... to be honest,my perspective is very positive on this change,because for the first time it clarifies the reference management nature of neovm. Looks like this can heavily simplify my C++ code, since C# will need similar ref count techniques as I do. And since no global linear prealloc mem is required,this is also different from wasm,in a positive manner. Will try to verify more and confirm some thoughts. Looks like an ellegant and definitive solution to refcount problems.

[erikzhang]

I can’t see this Pr solving the memory limit issue

It doesn't limit the memory, but limit the item count. It is compatible with the current mechanism.

We loose a lot of public methods like for example Add , Insert in Array .. most of the logic inside the StackItems.

NeoVM is designed for executing smart contract. In a contract, you can use the APPEND and SETITEM to edit the array as usual. Only when you are writing SYSCALL will you need to construct the arrays directly and push them onto the running VM stack. In this case, you can use the new function in this PR. I don't think it's a big problem.

We loose the availability to insert items into certain ExecutionContext, we must use the centralized Push Method.

What does this matter?

[erikzhang]

@shargon I can add new constructor to Array.

public Array(ExecutionEngine engine = null)

If you need to add a new created array to a running VM stack, you need to create it with this constructor. I think it is the same as your method, but without an additional ReservedMemory.

[shargon]

Do you think that is possible to adapt this solution without lose the public methods?

[erikzhang]

I think it is possible. I will work on it tomorrow.

[lock9]

It doesn't limit the memory, but limit the item count. It is compatible with the current mechanism.

That is exactly the issue that I see with this PR. It doesn't seem to fix #202. Mabe this PR fixes something else that I'm not aware (explicit reference counting?).
A VM should have "free space" based on memory, not object count. I would expect reference counting to be used as memory management only, but not as a consumption boundary.

[erikzhang]

@lock9 You are right. It doesn't close #202. But we currently don't have a perfect solution for memory limit. Changing the current mechanism to a memory limit mechanism is a very big change. We must propose the plan first.

For the memory limit mechanism, I don't think we have a complete solution. Every items and every structures have different size on different platforms and different languages. We must first have a complete standard for it.

My PR is not to limit the memory usage. It doesn't change the current mechanism. It limit the items count on the stack, but with great performance improvement.

[erikzhang]

@shargon @lock9 I reorganized the code. I think it looks more clear and easy to understand.

[erikzhang]

You can't assume that neo runs on x64 only. And also, a map is not only a pointer.

[shargon]

You can't assume neo is run on x64 only

If they run on x32, also they can sum 8 bytes for references, although in his system is 4.

[shargon]

Little optimizations here #233

[shargon]

00:00:00.8549087 Very good improvement for the VM. Neo3 will fly!

[shargon]

I need one day more for review the changes in ExecutionEngine deeper, but all looks good to me :)

[lock9]

Hi Erik, I'm impressed with the changes. This refactor is very good. I really liked the way you made the reference counting structures.
I'm waiting for the code to be "deployable" so we can test this, but in any case, really good PR.

[shargon]

The changes are good to me, this is an awesome improve for the NeoVM

[Tommo-L]

Benchmark:

type	neo2.x	neo3
5 invoc. stack	565000	1406000
1 invoc. stack	1125000	1409000

Increased 25% ~ 250% on ops

Neo2 test case:

[TestMethod]
public void TestBenchmark()
{
using (var script = new ScriptBuilder())
{
	script.Emit(OpCode.PUSH0);
	script.Emit(OpCode.PUSH1);
	script.Emit(OpCode.PUSH2);
	script.Emit(OpCode.PUSH3);
	script.Emit(OpCode.PUSH4);
	script.Emit(OpCode.PUSH5);
	script.Emit(OpCode.PUSH6);
	script.Emit(OpCode.PUSH7);
	script.Emit(OpCode.PUSH8);
	script.Emit(OpCode.PUSH9);
	script.Emit(OpCode.PUSH10);
	script.Emit(OpCode.PUSH11);
	script.Emit(OpCode.PUSH12);
	script.Emit(OpCode.PUSH13);
	script.Emit(OpCode.PUSH15);
	script.Emit(OpCode.PACK);
	script.Emit(OpCode.CALL, new byte[] { 0x04, 0x00 });  // <-----------------
	script.Emit(OpCode.RET);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.CALL, new byte[] { 0x04, 0x00 });
	script.Emit(OpCode.RET);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.CALL, new byte[] { 0x04, 0x00 });
	script.Emit(OpCode.RET);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.CALL, new byte[] { 0x04, 0x00 });
	script.Emit(OpCode.RET);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.NOP);
	script.Emit(OpCode.CALL, new byte[] { 0x04, 0x00 });
        script.Emit(OpCode.NOP);
        script.Emit(OpCode.NOP);                             // <-------------------

	for(int i = 0; i < 2000000; i++)
	{
		script.Emit(OpCode.PUSH0);
		script.Emit(OpCode.PUSH1);
		script.Emit(OpCode.PUSH2);
		script.Emit(OpCode.DROP);
		script.Emit(OpCode.DROP);
		script.Emit(OpCode.DROP);
	}
	script.Emit(OpCode.RET);
	using (var engine = new ExecutionEngine(null, Crypto.Default, null, null))
	{
		engine.LoadScript(script.ToArray());
		Stopwatch sw = new Stopwatch();

		sw.Start();
		engine.Execute();

		Console.WriteLine("ops: " + (2000000 * 6 + 37) / sw.ElapsedMilliseconds * 1000);
		sw.Reset();
	}
}

Neo3 test case

[TestMethod]
public void TestBenchmark()
{
	using (var script = new ScriptBuilder())
	{
		script.Emit(OpCode.PUSH0);
		script.Emit(OpCode.PUSH1);
		script.Emit(OpCode.PUSH2);
		script.Emit(OpCode.PUSH3);
		script.Emit(OpCode.PUSH4);
		script.Emit(OpCode.PUSH5);
		script.Emit(OpCode.PUSH6);
		script.Emit(OpCode.PUSH7);
		script.Emit(OpCode.PUSH8);
		script.Emit(OpCode.PUSH9);
		script.Emit(OpCode.PUSH10);
		script.Emit(OpCode.PUSH11);
		script.Emit(OpCode.PUSH12);
		script.Emit(OpCode.PUSH13);
		script.Emit(OpCode.PUSH14);
		script.Emit(OpCode.PUSH15);
		script.Emit(OpCode.PACK);
		script.Emit(OpCode.CALL, new byte[] { 0x03 });  // <-----------------
		script.Emit(OpCode.RET);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.CALL, new byte[] { 0x03 });
		script.Emit(OpCode.RET);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.CALL, new byte[] { 0x03 });
		script.Emit(OpCode.RET);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.CALL, new byte[] { 0x03 });
		script.Emit(OpCode.RET);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.CALL, new byte[] { 0x03 });
		script.Emit(OpCode.RET);
		script.Emit(OpCode.NOP);
		script.Emit(OpCode.NOP);                     // <-----------------

		for (int i = 0; i < 2000000; i++)
		{
			script.Emit(OpCode.PUSH0);
			script.Emit(OpCode.PUSH1);
			script.Emit(OpCode.PUSH2);
			script.Emit(OpCode.DROP);
			script.Emit(OpCode.DROP);
			script.Emit(OpCode.DROP);
		}
		script.Emit(OpCode.RET);
		
		using (var engine = new TestEngine())
		{
			engine.LoadScript(script.ToArray());
			Stopwatch sw = new Stopwatch();

			sw.Start();
			engine.Execute();

			Console.WriteLine("ops: " + (2000000 * 6 + 38) / sw.ElapsedMilliseconds * 1000);
			sw.Reset();
		}
	}
}

[erikzhang]

This test case is not designed to fully reflect the advantages of this PR. A large number of nested array insertion and deletion operations should be added.

[Tommo-L]

This test case is not designed to fully reflect the advantages of this PR.

Yes, in some special case, this pr increased more.

I mainly tested for simulateing a normal case.

[shargon]

I think that it depend of the case, try it with a regular SC, our previous test was great.