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.

Sign up for GitHub

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

Jump to bottom

Opt opcodes #16939

Merged
merged 4 commits into from
Jun 14, 2018
Merged

Opt opcodes #16939

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f9871ad
vm/test: add tests+benchmarks for mstore
holiman Jun 10, 2018
4f26fab
core/vm: less alloc and copying for mstore
holiman Jun 10, 2018
5ef43e0
core/vm: less allocs in sload
holiman Jun 10, 2018
f178d4d
vm: check for errors more correctly
holiman Jun 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/vm/instructions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ func opMload(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *St
func opMstore(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) {
// pop value of the stack
mStart, val := stack.pop(), stack.pop()
memory.Set(mStart.Uint64(), 32, math.PaddedBigBytes(val, 32))
memory.Set32(mStart.Uint64(), val)

evm.interpreter.intPool.put(mStart, val)
return nil, nil
Expand All @@ -570,9 +570,9 @@ func opMstore8(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *
}

func opSload(pc *uint64, evm *EVM, contract *Contract, memory *Memory, stack *Stack) ([]byte, error) {
loc := common.BigToHash(stack.pop())
val := evm.StateDB.GetState(contract.Address(), loc).Big()
stack.push(val)
loc := stack.peek()
val := evm.StateDB.GetState(contract.Address(), common.BigToHash(loc))
loc.SetBytes(val.Bytes())
return nil, nil
}

Expand Down
39 changes: 39 additions & 0 deletions core/vm/instructions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,3 +425,42 @@ func BenchmarkOpIsZero(b *testing.B) {
x := "FBCDEF090807060504030201ffffffffFBCDEF090807060504030201ffffffff"
opBenchmark(b, opIszero, x)
}

func TestOpMstore(t *testing.T) {
var (
env = NewEVM(Context{}, nil, params.TestChainConfig, Config{})
stack = newstack()
mem = NewMemory()
)
mem.Resize(64)
pc := uint64(0)
v := "abcdef00000000000000abba000000000deaf000000c0de00100000000133700"
stack.pushN(new(big.Int).SetBytes(common.Hex2Bytes(v)), big.NewInt(0))
opMstore(&pc, env, nil, mem, stack)
if got := common.Bytes2Hex(mem.Get(0, 32)); got != v {
t.Fatalf("Mstore fail, got %v, expected %v", got, v)
}
stack.pushN(big.NewInt(0x1), big.NewInt(0))
opMstore(&pc, env, nil, mem, stack)
if common.Bytes2Hex(mem.Get(0, 32)) != "0000000000000000000000000000000000000000000000000000000000000001" {
t.Fatalf("Mstore failed to overwrite previous value")
}
}

func BenchmarkOpMstore(bench *testing.B) {
var (
env = NewEVM(Context{}, nil, params.TestChainConfig, Config{})
stack = newstack()
mem = NewMemory()
)
mem.Resize(64)
pc := uint64(0)
memStart := big.NewInt(0)
value := big.NewInt(0x1337)

bench.ResetTimer()
for i := 0; i < bench.N; i++ {
stack.pushN(value, memStart)
opMstore(&pc, env, nil, mem, stack)
}
}
21 changes: 20 additions & 1 deletion core/vm/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@

package vm

import "fmt"
import (
"fmt"
"math/big"

"github.com/ethereum/go-ethereum/common/math"
)

// Memory implements a simple memory model for the ethereum virtual machine.
type Memory struct {
Expand All @@ -43,6 +48,20 @@ func (m *Memory) Set(offset, size uint64, value []byte) {
}
}

// Set32 sets the 32 bytes starting at offset to the value of val, left-padded with zeroes to
// 32 bytes.
func (m *Memory) Set32(offset uint64, val *big.Int) {
// length of store may never be less than offset + size.
// The store should be resized PRIOR to setting the memory
if 32 > uint64(len(m.store)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're validating only the size, not the offset + size. Shouldn't you have offset + 32 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you expand the code snippet a bit, you'll see that I reused the code above as much as possible, for easier review. I also thought it should be offset+32, but it's not in the code above... (?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the snippet above, I believe that if size is zero, the offset can be arbitrarily high. So checking size+offset for that would be wrong. So whereas in this instance such a check would be ok, this code is no more wrong than the code above. Both depends on resize already having taken place.

panic("INVALID memory: store empty")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fact you're panicing gets the message across already, no need for the caps :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

}
// Zero the memory area
copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps use make([]byte, 32) instead of the literal? Seems less error prone, or perhaps if you don't want to alloc at all, separate this out into a global constant initialized with make?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is me being n00b. I initially attempted const, but that didn't work so I figured this way to put it would make the compiler replace this with something truly constant. Could you give an example of how to do a const slice?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For zeroing memory, the compiler recognises loops of the form

for i := range slice {
    slice[i] = 0
}

Such loops are replaced with a target-specific idiom for clearing memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tried that:

[user@work vm]$ go test -run - -bench BenchmarkOpMs -benchmem 
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/core/vm
BenchmarkOpMstore-2   	50000000	        32.8 ns/op	       0 B/op	       0 allocs/op
PASS

replace with your loop

diff --git a/core/vm/memory.go b/core/vm/memory.go
index a3530419d..9cc7cde40 100644
--- a/core/vm/memory.go
+++ b/core/vm/memory.go
@@ -57,7 +57,10 @@ func (m *Memory) Set32(offset uint64, val *big.Int) {
                panic("INVALID memory: store empty")
        }
        // Zero the memory area
-       copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
+       for i := range m.store[offset:offset+32] {
+               m.store[offset:offset+32][i] = 0
+       }
+       //copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
        // Fill in relevant bits
        math.ReadBits(val, m.store[offset:offset+32])
 }
ok  	github.com/ethereum/go-ethereum/core/vm	1.694s
[user@work vm]$ go test -run - -bench BenchmarkOpMs -benchmem 
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/core/vm
BenchmarkOpMstore-2   	20000000	        66.9 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/ethereum/go-ethereum/core/vm	1.528s
````diff --git a/core/vm/memory.go b/core/vm/memory.go
index a3530419d..d278f34ae 100644
--- a/core/vm/memory.go
+++ b/core/vm/memory.go
@@ -57,7 +57,10 @@ func (m *Memory) Set32(offset uint64, val *big.Int) {
                panic("INVALID memory: store empty")
        }
        // Zero the memory area
-       copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
+       for i := range m.store[offset:offset+32] {
+               m.store[i] = 0
+       }
+       //copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
        // Fill in relevant bits
        math.ReadBits(val, m.store[offset:offset+32])
 }

Another variant

diff --git a/core/vm/memory.go b/core/vm/memory.go
index a3530419d..d278f34ae 100644
--- a/core/vm/memory.go
+++ b/core/vm/memory.go
@@ -57,7 +57,10 @@ func (m *Memory) Set32(offset uint64, val *big.Int) {
                panic("INVALID memory: store empty")
        }
        // Zero the memory area
-       copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
+       for i := range m.store[offset:offset+32] {
+               m.store[i] = 0
+       }
+       //copy(m.store[offset:offset+32], []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0})
        // Fill in relevant bits
        math.ReadBits(val, m.store[offset:offset+32])
 }
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/core/vm
BenchmarkOpMstore-2   	30000000	        45.4 ns/op	       0 B/op	       0 allocs/op
PASS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a global var zero32 = make([]byte,32) :

BenchmarkOpMstore-2   	30000000	        48.0 ns/op	       0 B/op	       0 allocs/op

// Fill in relevant bits
math.ReadBits(val, m.store[offset:offset+32])
}

// Resize resizes the memory to size
func (m *Memory) Resize(size uint64) {
if uint64(m.Len()) < size {
Expand Down