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.

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

core/vm: first version of the intpool pool #17070

Merged
merged 3 commits into from
Jul 3, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
6 changes: 6 additions & 0 deletions core/vm/instructions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ func testTwoOperandOp(t *testing.T, tests []twoOperandTest, opFn func(pc *uint64
stack = newstack()
pc = uint64(0)
)
env.interpreter.intPool = poolOfIntPools.get()
for i, test := range tests {
x := new(big.Int).SetBytes(common.Hex2Bytes(test.x))
shift := new(big.Int).SetBytes(common.Hex2Bytes(test.y))
Expand Down Expand Up @@ -64,13 +65,15 @@ func testTwoOperandOp(t *testing.T, tests []twoOperandTest, opFn func(pc *uint64
}
}
}
poolOfIntPools.put(env.interpreter.intPool)
}

func TestByteOp(t *testing.T) {
var (
env = NewEVM(Context{}, nil, params.TestChainConfig, Config{})
stack = newstack()
)
env.interpreter.intPool = poolOfIntPools.get()
tests := []struct {
v string
th uint64
Expand All @@ -97,6 +100,7 @@ func TestByteOp(t *testing.T) {
t.Fatalf("Expected [%v] %v:th byte to be %v, was %v.", test.v, test.th, test.expected, actual)
}
}
poolOfIntPools.put(env.interpreter.intPool)
}

func TestSHL(t *testing.T) {
Expand Down Expand Up @@ -432,6 +436,7 @@ func TestOpMstore(t *testing.T) {
stack = newstack()
mem = NewMemory()
)
env.interpreter.intPool = poolOfIntPools.get()
mem.Resize(64)
pc := uint64(0)
v := "abcdef00000000000000abba000000000deaf000000c0de00100000000133700"
Expand All @@ -445,6 +450,7 @@ func TestOpMstore(t *testing.T) {
if common.Bytes2Hex(mem.Get(0, 32)) != "0000000000000000000000000000000000000000000000000000000000000001" {
t.Fatalf("Mstore failed to overwrite previous value")
}
poolOfIntPools.put(env.interpreter.intPool)
}

func BenchmarkOpMstore(bench *testing.B) {
Expand Down
12 changes: 11 additions & 1 deletion core/vm/interpreter.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ func NewInterpreter(evm *EVM, cfg Config) *Interpreter {
evm: evm,
cfg: cfg,
gasTable: evm.ChainConfig().GasTable(evm.BlockNumber),
intPool: newIntPool(),
}
}

Expand All @@ -104,6 +103,14 @@ func (in *Interpreter) enforceRestrictions(op OpCode, operation operation, stack
// considered a revert-and-consume-all-gas operation except for
// errExecutionReverted which means revert-and-keep-gas-left.
func (in *Interpreter) Run(contract *Contract, input []byte) (ret []byte, err error) {
if in.intPool == nil {
in.intPool = poolOfIntPools.get()
defer func() {
poolOfIntPools.put(in.intPool)
in.intPool = nil
}()
}
Copy link
Member

Choose a reason for hiding this comment

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

Please move this code up to the top. It should be an initialization code, currently it mixes in with the actual execution of the interpreter already.

Copy link
Member Author

Choose a reason for hiding this comment

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

couldn't do that since it now depends on the declaration of stack. See my upcoming change.

Copy link
Member

Choose a reason for hiding this comment

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

As we agreed, pls move this up.

Copy link
Member

@karalabe karalabe Jun 26, 2018

Choose a reason for hiding this comment

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

    if in.intPool == nil {
        in.intPool = poolOfIntPools.get()
        defer func() {
            poolOfIntPools.put(in.intPool)
            in.intPool = nil
        }()
    }


// Increment the call depth which is restricted to 1024
in.evm.depth++
defer func() { in.evm.depth-- }()
Expand Down Expand Up @@ -133,6 +140,9 @@ func (in *Interpreter) Run(contract *Contract, input []byte) (ret []byte, err er
)
contract.Input = input

// Reclaim the stack as an int pool when the execution stops
defer in.intPool.put(stack.data...)
Copy link
Member

Choose a reason for hiding this comment

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

Ah, this is not good. The issue is that the parameters are evaluated here already (i.e. stack.data... evaluates into an empty list). https://play.golang.org/p/A5WL8nCKrIs

We need to defer the parameter evaluation too.

defer func() { in.intPool.put(stack.data...) }()

https://play.golang.org/p/4ayoTNxgwsY


if in.cfg.Debug {
defer func() {
if err != nil {
Expand Down
41 changes: 40 additions & 1 deletion core/vm/intpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@

package vm

import "math/big"
import (
"math/big"
"sync"
)

var checkVal = big.NewInt(-42)

Expand Down Expand Up @@ -65,3 +68,39 @@ func (p *intPool) put(is ...*big.Int) {
p.pool.push(i)
}
}

// The intPool pool's default capacity
const poolDefaultCap = 25
Copy link
Member

Choose a reason for hiding this comment

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

Pls add a doc.


// intPoolPool manages a pool of intPools.
type intPoolPool struct {
pools []*intPool
lock sync.Mutex
}

var poolOfIntPools = &intPoolPool{
pools: make([]*intPool, 0, poolDefaultCap),
}

// get is looking for an available pool to return.
func (ipp *intPoolPool) get() *intPool {
ipp.lock.Lock()
defer ipp.lock.Unlock()

if len(poolOfIntPools.pools) > 0 {
ip := ipp.pools[len(ipp.pools)-1]
ipp.pools = ipp.pools[:len(ipp.pools)-1]
return ip
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't get why you're doing this boolean flag thing. Just keep the unused pools in here. When someone requests it, give it to them and remove it from the slice. When they return it, add it back to the slice (as long as it doesn't overflow).

return newIntPool()
}

// put a pool that has been allocated with get.
func (ipp *intPoolPool) put(ip *intPool) {
ipp.lock.Lock()
defer ipp.lock.Unlock()

if len(ipp.pools) < cap(ipp.pools) {
ipp.pools = append(ipp.pools, ip)
}
}
55 changes: 55 additions & 0 deletions core/vm/intpool_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright 2018 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

package vm

import (
"testing"
)

func TestIntPoolPoolGet(t *testing.T) {
poolOfIntPools.pools = make([]*intPool, 0, poolDefaultCap)

nip := poolOfIntPools.get()
if nip == nil {
t.Fatalf("Invalid pool allocation")
}
}

func TestIntPoolPoolPut(t *testing.T) {
poolOfIntPools.pools = make([]*intPool, 0, poolDefaultCap)

nip := poolOfIntPools.get()
if len(poolOfIntPools.pools) != 0 {
t.Fatalf("Pool got added to list when none should have been")
}

poolOfIntPools.put(nip)
if len(poolOfIntPools.pools) == 0 {
t.Fatalf("Pool did not get added to list when one should have been")
}
}

func TestIntPoolPoolReUse(t *testing.T) {
poolOfIntPools.pools = make([]*intPool, 0, poolDefaultCap)
nip := poolOfIntPools.get()
poolOfIntPools.put(nip)
poolOfIntPools.get()

if len(poolOfIntPools.pools) != 0 {
t.Fatalf("Invalid number of pools. Got %d, expected %d", len(poolOfIntPools.pools), 0)
}
}