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

Opt opcodes #16939

Merged
merged 4 commits into from
Jun 14, 2018
Merged

Opt opcodes #16939

Changes from 1 commit
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
Prev Previous commit
vm: check for errors more correctly
  • Loading branch information
@holiman
holiman committed Jun 11, 2018
commit f178d4d0f0b13e23f37b503ffa51b1f76c64f8b3
15 changes: 7 additions & 8 deletions core/vm/memory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,14 @@ func NewMemory() *Memory {

// Set sets offset + size to value
func (m *Memory) Set(offset, size uint64, value []byte) {
// length of store may never be less than offset + size.
// The store should be resized PRIOR to setting the memory
if size > uint64(len(m.store)) {
panic("INVALID memory: store empty")
}

// It's possible the offset is greater than 0 and size equals 0. This is because
// the calcMemSize (common.go) could potentially return 0 when size is zero (NO-OP)
if size > 0 {
// length of store may never be less than offset + size.
// The store should be resized PRIOR to setting the memory
if offset+size > uint64(len(m.store)) {
panic("invalid memory: store empty")
}
copy(m.store[offset:offset+size], value)
}
}
Expand All @@ -53,8 +52,8 @@ func (m *Memory) Set(offset, size uint64, value []byte) {
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)) {
panic("INVALID memory: store empty")
if offset+32 > uint64(len(m.store)) {
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})
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

Expand Down