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

New opcode 'PACKSTRUCT' #51

Closed
dicarlo2 opened this issue Aug 28, 2018 · 6 comments
Closed

New opcode 'PACKSTRUCT' #51

dicarlo2 opened this issue Aug 28, 2018 · 6 comments

Comments

@dicarlo2
Copy link

Like PACK but for structs.

@erikzhang
Copy link
Member

Is there a specific scenario that you need this for?

@dicarlo2
Copy link
Author

dicarlo2 commented Aug 30, 2018

Without going into too much detail, we effectively store every value as an array of [<number>, <value>] in case we need to know the type at runtime[1]. Currently we store this as an array because it's relatively efficient to create:

// assume <value> is on stack
PUSH<number> // free
PUSH2 // free
PACK // 1 droplet

An equivalent struct is much more expensive to create, to the point that it's not feasible to use structs at all.

If we were able to use structs, then we could make other parts more efficient, for example, === becomes a simple EQUAL op, rather than needing to unwrap the value:

// assume two wrapped values are on the stack
PUSH1 // free
PICKITEM // 1 droplet
SWAP // 1 droplet
PUSH1 // free
PICKITEM // 1 droplet
EQUAL // 1 droplet

4x more efficient if we can use a struct and a single EQUAL. Another example scenario is using these wrapped values as keys in maps, this would preserve the semantics of JavaScript along with #50.

[1] Note, later iterations of neo-one will optimize this away when possible, but still in some cases we will need to continue wrapping.

@erikzhang
Copy link
Member

erikzhang commented Aug 31, 2018

Creating struct with PACKSTRUCT

PUSH value3
PUSH value2
PUSH value1
PUSH 3
PACKSTRUCT

Creating struct without PACKSTRUCT

PUSH 0
NEWSTRUCT
DUP
PUSH value1
APPEND
DUP
PUSH value2
APPEND
DUP
PUSH value3
APPEND

@anthdm
Copy link

anthdm commented Oct 2, 2018

@dicarlo2 Not sure why you should need that? Structs are treated the same as arrays. Their values are just an ordered union on the stack. The compiler should track all additional information about the underlying type and index of a field.

@dicarlo2
Copy link
Author

@anthdm Structs do not work the same as arrays - I invite you to take a look at the source code to see the differences.

@dicarlo2
Copy link
Author

@erikzhang do you have objections to adding PACKSTRUCT? This is blocking for NEO-ONE TypeScript smart contracts.

bors bot added a commit to neo-one-suite/neo-one that referenced this issue Mar 28, 2019
1256: refactor(PACK+NEWSTRUCT): Replace PACKSTRUCT with PACK+NEWSTRUCT r=dicarlo2 a=afragapane

### Description of the Change
Removes the PACKSTRUCT Opcode and modifies NEWSTRUCT/NEWARRAY to be able to convert an existing array into a struct and struct into array.  This allows the use of a PACK+NEWSTRUCT pair of opcodes in place of PACKSTRUCT.

### Test Plan
Added tests for the conversions in NEWSTRUCT and NEWARRAY.  Updated existing tests to reflect the slightly higher GAS costs of neo-one compiled contracts.

### Alternate Designs
Previously the plan was to implement our PACKSTRUCT in the neo-vm as per neo-project/neo-vm#51 and neo-project/neo-vm#88.  This new solution was suggested and implemented here: neo-project/neo-vm#91.
### Benefits

### Possible Drawbacks
Very slightly higher GAS cost for neo-one compiled smart contracts.  

### Applicable Issues
#1254

Co-authored-by: afragapane <ahfragapane@gmail.com>
bors bot added a commit to neo-one-suite/neo-one that referenced this issue Mar 28, 2019
1256: refactor(PACK+NEWSTRUCT): Replace PACKSTRUCT with PACK+NEWSTRUCT r=dicarlo2 a=afragapane

### Description of the Change
Removes the PACKSTRUCT Opcode and modifies NEWSTRUCT/NEWARRAY to be able to convert an existing array into a struct and struct into array.  This allows the use of a PACK+NEWSTRUCT pair of opcodes in place of PACKSTRUCT.

### Test Plan
Added tests for the conversions in NEWSTRUCT and NEWARRAY.  Updated existing tests to reflect the slightly higher GAS costs of neo-one compiled contracts.

### Alternate Designs
Previously the plan was to implement our PACKSTRUCT in the neo-vm as per neo-project/neo-vm#51 and neo-project/neo-vm#88.  This new solution was suggested and implemented here: neo-project/neo-vm#91.
### Benefits

### Possible Drawbacks
Very slightly higher GAS cost for neo-one compiled smart contracts.  

### Applicable Issues
#1254

Co-authored-by: afragapane <ahfragapane@gmail.com>
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

3 participants