Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

Introduce CLONE and remove Struct stackitem #182

Closed
igormcoelho opened this issue Jun 28, 2019 · 20 comments
Closed

Introduce CLONE and remove Struct stackitem #182

igormcoelho opened this issue Jun 28, 2019 · 20 comments

Comments

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 28, 2019

I wonder if it's best to add CLONE opcode, as discussed before,and also remove Struct stackitem from neovm.

The reason is that SETITEM itself is meant to require a reference type, since it consumes the input (and generates no output). So, it's fine for arrays. For Struct, it's essentially more complicated, as you also need it as a reference type (it will be consumed by SETITEM), but its fields are currently set by an internal clone operation. In this sense, it would be better to simply explicitly clone the value before setitem.

Example array: V[0]=x
element x on stack
push0
push1 newarray
setitem

Example struct: S.field0=x
element x on stack
push0
push1 newstruct
setitem

Example "struct" wirh clone: S.field0=x
element x on stack
clone. <---- struct field clone behavior
push0
push1 newarray. <---- same effect on struct setitem
setitem

If it's possible to eliminate a stack item,logic will be much simpler on the vm side, in my opinion. From user perspective, many clone operations will be possibly needed, but the opcode effect willl be much simpler to track.

A better option would possibly provide CLONE (regular clone) and RCLONE (recursive clone), for shallow and deep copies.

@shargon
Copy link
Member

shargon commented Jun 28, 2019

I have never liked STRUCT, so i think is a good move, but clone a map, could be complicated (for limits)

@igormcoelho
Copy link
Contributor Author

Isnt it the same limit for map size and array size?

@erikzhang
Copy link
Member

@lightszero What do you think?

@lightszero
Copy link
Member

lightszero commented Jun 30, 2019

yes,I agree,work with a CLONE opcode will make things more clearly.

@erikzhang
Copy link
Member

erikzhang commented Jul 1, 2019

Maybe SETITEMWITHCLONE.

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Jul 2, 2019

Maybe SETITEMWITHCLONE

this could be useful if used in big proportions... it would be nice to see the requirements of typescript compiler on neo-one, to know in advance if it's used a lot

Two useful behaviors may be: (i) CLONE item before set it on Array, and keep it on stack; (ii) set element on array and clone the array.

[EDIT]: As usual, I think a lot, and in the end I fully agree with you Erik. Thats EXACTLY what we need.

@erikzhang
Copy link
Member

erikzhang commented Jul 3, 2019

struct A
{
    public int X, Y;
}
struct B
{
    public A[] Items;
}
void Main()
{
    B[] array = new B[100];
    B b = new B
    {
        Items = new[]
        {
            new A
            {
                X = 2,
                Y = 3
            }
        }
    };
    array[0] = b;
}

It is impossible for CLONE or SETITEMWITHCLONE to clone the struct B if there is not a Struct type in NeoVM.

@lightszero @igormcoelho

@igormcoelho
Copy link
Contributor Author

I have to think how this is supposed to compile Erik... 🤔

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 4, 2019

I thought for a while... no type should receive a pure reference, only clones. I'll create an example to explain why... so in fact we need a SETITEMWITHCLONE, instead of SETITEM.

It is impossible for CLONE or SETITEMWITHCLONE to clone the struct B if there is not a Struct type in NeoVM.

Wrong is Array type, not Struct, so I prefer to keep Struct behavior, and rename it to Array.

Array is harmful because of this simple example:

PUSH1
NEWARRAY
DUP
DUP
PUSH1
SETITEM

After this, you can never CLONE array, since it contains itself, and this would cause an infinite loop. No StackItem should reference another one internally.

Please let's change this on nvm3.

@igormcoelho
Copy link
Contributor Author

Ok, my proposal is:

  • ban Struct concept
  • change behavior of Array to Struct (never holding references)
  • change behavior of SETITEM to SETITEMWITHCLONE

do you agree @shargon @erikzhang @lightzero ?

@erikzhang
Copy link
Member

erikzhang commented Aug 18, 2019

No. We need this:

class TestClass
{
    public int A;
}

TestClass[] t = new TestClass[10];
TestClass e0 = new TestClass();
t[0] = e0;
t[0].A = 1;
Assert(e0.A == 1);

@shargon
Copy link
Member

shargon commented Aug 18, 2019

Agree with @erikzhang

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 18, 2019

@erikzhang all features on C# will be preserved, even if we do that. I think that perhaps current behavior of Array is excessively ref based, I dont even know how to do an example on C#... only c++.

Can you create an object array, where first element is array itself? @shargon

@vncoelho
Copy link
Member

vncoelho commented Aug 29, 2019

Example:

using Neo.SmartContract.Framework.Services.Neo;
using System.Diagnostics;

namespace Neo.SmartContract
{
    public    class TestClass
    {
        public int A;
    }

    public class HelloWorld : Framework.SmartContract
    {
        public static void Main()
        {
            TestClass[] t = new TestClass[10];
            TestClass e0 = new TestClass();
            t[0] = e0;
            t[0].A = 1;
            Debug.Assert(e0.A == 1);
        }
    }
}

OPCODES:

51c56b5ac56151c56a00527ac476006a00c3c400c351007cc4616c7566

#29 bytes
51 0: PUSH1  # The number 1 is pushed onto the stack.
c5 1: NEWARRAY  #
6b 2: TOALTSTACK  # Puts the input onto the top of the alt stack. Removes it from the main stack.
5a 3: PUSH10  # The number 10 is pushed onto the stack.
c5 4: NEWARRAY  #
61 5: NOP  # Does nothing.
51 6: PUSH1  # The number 1 is pushed onto the stack.
c5 7: NEWARRAY  #
6a 8: DUPFROMALTSTACK  #
00 9: PUSH0  #An empty array of bytes is pushed onto the stack
52 10: PUSH2  # The number 2 is pushed onto the stack.
7a 11: ROLL  # The item n back in the stack is moved to the top.
c4 12: SETITEM  #
76 13: DUP  # Duplicates the top stack item.
00 14: PUSH0  #An empty array of bytes is pushed onto the stack
6a 15: DUPFROMALTSTACK  #
00 16: PUSH0  #An empty array of bytes is pushed onto the stack
c3 17: PICKITEM  #
c4 18: SETITEM  #
00 19: PUSH0  #An empty array of bytes is pushed onto the stack
c3 20: PICKITEM  #
51 21: PUSH1  # The number 1 is pushed onto the stack.
00 22: PUSH0  #An empty array of bytes is pushed onto the stack
7c 23: SWAP  # The top two items on the stack are swapped.
c4 24: SETITEM  #
61 25: NOP  # Does nothing.
6c 26: FROMALTSTACK  # Puts the input onto the top of the main stack. Removes it from the alt stack.
75 27: DROP  # Removes the top stack item.
66 28: RET  #

@vncoelho
Copy link
Member

vncoelho commented Aug 29, 2019

Example:

using Neo.SmartContract.Framework.Services.Neo;
using System.Diagnostics;

namespace Neo.SmartContract
{
    struct A
    {
        public int X, Y;
    }
    struct B
    {
        public A[] Items;
    }

    public class HelloWorld : Framework.SmartContract
    {
        public static void Main()
        {
                B[] array = new B[100];
                B b = new B
                {
                    Items = new[]
                    {
                        new A
                        {
                            X = 2,
                            Y = 3
                        }
                    }
                };
                array[0] = b;
        }
    }
}

OpCodes: 54c56b0164c56a00527ac4526151c66a527a527ac46a52c351c576005361
52c66a527a527ac46a53c352007cc46a53c353517cc46a53c3c4007cc46a
52c36a51527ac46a00c3006a51c3c4616c7566

#79 bytes
54 0: PUSH4  # The number 4 is pushed onto the stack.
c5 1: NEWARRAY  #
6b 2: TOALTSTACK  # Puts the input onto the top of the alt stack. Removes it from the main stack.
01 3: PUSHBYTES1 64 # d
c5 5: NEWARRAY  #
6a 6: DUPFROMALTSTACK  #
00 7: PUSH0  #An empty array of bytes is pushed onto the stack
52 8: PUSH2  # The number 2 is pushed onto the stack.
7a 9: ROLL  # The item n back in the stack is moved to the top.
c4 10: SETITEM  #
52 11: PUSH2  # The number 2 is pushed onto the stack.
61 12: NOP  # Does nothing.
51 13: PUSH1  # The number 1 is pushed onto the stack.
c6 14: NEWSTRUCT  #
6a 15: DUPFROMALTSTACK  #
52 16: PUSH2  # The number 2 is pushed onto the stack.
7a 17: ROLL  # The item n back in the stack is moved to the top.
52 18: PUSH2  # The number 2 is pushed onto the stack.
7a 19: ROLL  # The item n back in the stack is moved to the top.
c4 20: SETITEM  #
6a 21: DUPFROMALTSTACK  #
52 22: PUSH2  # The number 2 is pushed onto the stack.
c3 23: PICKITEM  #
51 24: PUSH1  # The number 1 is pushed onto the stack.
c5 25: NEWARRAY  #
76 26: DUP  # Duplicates the top stack item.
00 27: PUSH0  #An empty array of bytes is pushed onto the stack
53 28: PUSH3  # The number 3 is pushed onto the stack.
61 29: NOP  # Does nothing.
52 30: PUSH2  # The number 2 is pushed onto the stack.
c6 31: NEWSTRUCT  #
6a 32: DUPFROMALTSTACK  #
52 33: PUSH2  # The number 2 is pushed onto the stack.
7a 34: ROLL  # The item n back in the stack is moved to the top.
52 35: PUSH2  # The number 2 is pushed onto the stack.
7a 36: ROLL  # The item n back in the stack is moved to the top.
c4 37: SETITEM  #
6a 38: DUPFROMALTSTACK  #
53 39: PUSH3  # The number 3 is pushed onto the stack.
c3 40: PICKITEM  #
52 41: PUSH2  # The number 2 is pushed onto the stack.
00 42: PUSH0  #An empty array of bytes is pushed onto the stack
7c 43: SWAP  # The top two items on the stack are swapped.
c4 44: SETITEM  #
6a 45: DUPFROMALTSTACK  #
53 46: PUSH3  # The number 3 is pushed onto the stack.
c3 47: PICKITEM  #
53 48: PUSH3  # The number 3 is pushed onto the stack.
51 49: PUSH1  # The number 1 is pushed onto the stack.
7c 50: SWAP  # The top two items on the stack are swapped.
c4 51: SETITEM  #
6a 52: DUPFROMALTSTACK  #
53 53: PUSH3  # The number 3 is pushed onto the stack.
c3 54: PICKITEM  #
c4 55: SETITEM  #
00 56: PUSH0  #An empty array of bytes is pushed onto the stack
7c 57: SWAP  # The top two items on the stack are swapped.
c4 58: SETITEM  #
6a 59: DUPFROMALTSTACK  #
52 60: PUSH2  # The number 2 is pushed onto the stack.
c3 61: PICKITEM  #
6a 62: DUPFROMALTSTACK  #
51 63: PUSH1  # The number 1 is pushed onto the stack.
52 64: PUSH2  # The number 2 is pushed onto the stack.
7a 65: ROLL  # The item n back in the stack is moved to the top.
c4 66: SETITEM  #
6a 67: DUPFROMALTSTACK  #
00 68: PUSH0  #An empty array of bytes is pushed onto the stack
c3 69: PICKITEM  #
00 70: PUSH0  #An empty array of bytes is pushed onto the stack
6a 71: DUPFROMALTSTACK  #
51 72: PUSH1  # The number 1 is pushed onto the stack.
c3 73: PICKITEM  #
c4 74: SETITEM  #
61 75: NOP  # Does nothing.
6c 76: FROMALTSTACK  # Puts the input onto the top of the main stack. Removes it from the alt stack.
75 77: DROP  # Removes the top stack item.
66 78: RET  #

@shargon
Copy link
Member

shargon commented Aug 29, 2019

Can you create an object array, where first element is array itself? @shargon

@igormcoelho if you serialize the object, will fail, and in the case of struct is cloned

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Sep 19, 2019

@vncoelho I finally took time to understand struct/class on C#. It's simple in fact.

using System;
					
public class Program
{
	public struct Point
	{
		public int x;
		public int y;
	}
	
	public class CPoint
	{
		public int x;
		public int y;
	}
	
	public static void func(Point p1)
	{
		p1.x = 20;
	}
	
	public static void cfunc(CPoint p1)
	{
		p1.x = 20;
	}

	public static void vfunc(Point[] vp1)
	{
		vp1[0].x = 20;
	}
	
	public static void Main()
	{
		Point p = new Point();
		p.x = 10;
		p.y = 20;
		func(p);
		Console.WriteLine($"x: {p.x} y: {p.y}"); // x: 10 y: 20
		
		CPoint cp = new CPoint();
		cp.x = 10;
		cp.y = 20;
		cfunc(cp);
		Console.WriteLine($"x: {cp.x} y: {cp.y}"); // x: 20 y: 20

		Point[] vp = new Point[2];
		vp[0].x = 10;
		vp[0].y = 20;
		vfunc(vp);
		Console.WriteLine($"x: {vp[0].x} y: {vp[0].y}"); // x: 20 y: 20
	}
}

On C++, it's like doing func(Point p) vs cfunc(Point& cp). vfunc(Point[] vp) is the same on C++ (ref-based). Now I can come back to re-understand this discussion :)

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Sep 29, 2019

@erikzhang you are right, if we want this, we need references:

class TestClass
{
    public int A;
}

TestClass[] t = new TestClass[10];
TestClass e0 = new TestClass();
t[0] = e0;
t[0].A = 1;
Assert(e0.A == 1);

The problem is, if we want this, we can build any kind of self-references too... we can build tree-like objects and cycle objects using references, and this will make memory management very hard on languages like C++, to implement NeoVM.
We can implement in C++, using smart/shared pointers, but it's like having a global memory management, for a double stack computing machine.

In this case, I think CLONE will also be very challenging to implement... I hope it's possible.

@igormcoelho
Copy link
Contributor Author

@erikzhang what do you think of a solution like this?

TestClass[] t = new TestClass[10];
TestClass e0 = new TestClass();
// t[0] = e0; // this will raise an error (must check which CIL ops happen here)
e0 = t[0]; // this can be converted to DUP (I hope!)
t[0].A = 1;
Assert(e0.A == 1);

This way, we can adopt SETITEMWITHCLONE, and get indirections from the opposite direction...
From user perspective, I don't think this will be an issue, and we would make NeoVM much more solid (no outside memory management, besides stacks). Limitations will always exist on the devpack (right now we have many already), and some are hard to solve because we don't have a CLONE... so I think we can try to simplify the VM, perhaps lose some operations, but gain others (and simplicity).

@igormcoelho
Copy link
Contributor Author

I'll try to think on something better... right now it seems hard to remove either Struct or Array types.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants