core/vm: Add method Stack.popptr and apply to a bunch of instructions

[qianbin]

It's safe if the popped ptr not escape the body of OP function.

benchstat before.txt after.txt 
name            old time/op  new time/op  delta
OpAdd64-8       13.2ns ± 1%  11.5ns ± 0%  -12.70%  (p=0.016 n=5+4)
OpAdd128-8      13.3ns ± 4%  11.5ns ± 2%  -13.43%  (p=0.008 n=5+5)
OpAdd256-8      13.2ns ± 1%  12.0ns ±18%     ~     (p=0.143 n=5+5)
OpSub64-8       13.0ns ± 1%  11.4ns ± 2%  -11.95%  (p=0.008 n=5+5)
OpSub128-8      13.0ns ± 2%  11.9ns ± 9%     ~     (p=0.056 n=5+5)
OpSub256-8      12.9ns ± 1%  11.5ns ± 3%  -11.17%  (p=0.008 n=5+5)
OpMul-8         17.7ns ± 4%  15.6ns ± 1%  -11.54%  (p=0.008 n=5+5)
OpDiv256-8      83.4ns ± 0%  82.3ns ± 0%   -1.33%  (p=0.016 n=4+5)
OpDiv128-8      52.8ns ± 1%  51.7ns ± 1%   -2.17%  (p=0.008 n=5+5)
OpDiv64-8       18.3ns ± 1%  16.4ns ± 0%  -10.50%  (p=0.008 n=5+5)
OpSdiv-8        89.7ns ± 0%  89.3ns ± 0%     ~     (p=0.063 n=5+5)
OpMod-8         15.0ns ± 9%  13.4ns ± 8%  -10.29%  (p=0.008 n=5+5)
OpSmod-8        22.9ns ± 1%  21.5ns ± 1%   -6.17%  (p=0.008 n=5+5)
OpExp-8         2.28µs ± 0%  2.28µs ± 0%     ~     (p=0.143 n=5+5)
OpSignExtend-8  12.6ns ±11%  10.8ns ± 3%  -14.19%  (p=0.008 n=5+5)
OpLt-8          12.7ns ± 1%  11.5ns ± 3%   -9.85%  (p=0.008 n=5+5)
OpGt-8          12.1ns ± 1%  11.9ns ± 8%     ~     (p=0.135 n=5+5)
OpSlt-8         14.4ns ± 4%  12.3ns ± 0%  -14.92%  (p=0.008 n=5+5)
OpSgt-8         13.9ns ± 2%  12.4ns ± 1%  -11.00%  (p=0.008 n=5+5)
OpEq-8          12.7ns ± 1%  11.6ns ± 3%   -9.25%  (p=0.008 n=5+5)
OpEq2-8         11.4ns ± 2%  10.6ns ±12%     ~     (p=0.151 n=5+5)
OpAnd-8         12.8ns ± 2%  11.3ns ± 3%  -11.15%  (p=0.008 n=5+5)
OpOr-8          12.7ns ± 1%  11.4ns ± 3%  -10.15%  (p=0.008 n=5+5)
OpXor-8         12.7ns ± 1%  11.4ns ± 2%  -10.27%  (p=0.008 n=5+5)
OpByte-8        12.9ns ± 5%  10.7ns ± 5%  -17.26%  (p=0.008 n=5+5)
OpAddmod-8      98.3ns ± 0%  94.4ns ± 0%   -4.04%  (p=0.008 n=5+5)
OpMulmod-8       160ns ± 0%   157ns ± 0%   -2.03%  (p=0.008 n=5+5)
OpSHL-8         13.1ns ± 3%  11.7ns ± 6%  -10.62%  (p=0.008 n=5+5)
OpSHR-8         13.0ns ± 1%  11.8ns ± 5%   -9.07%  (p=0.008 n=5+5)
OpSAR-8         13.9ns ± 3%  12.3ns ± 1%  -11.13%  (p=0.008 n=5+5)
OpIsZero-8      5.70ns ± 0%  5.61ns ± 2%     ~     (p=0.143 n=5+5)
OpMstore-8      19.1ns ± 0%  14.5ns ± 0%  -23.90%  (p=0.008 n=5+5)
OpKeccak256-8    488ns ± 0%   488ns ± 0%     ~     (p=0.230 n=5+5)

[fjl]

This is a nice optimization idea, but very dangerous. popptr returns a pointer to the 'inaccessible' area of the stack. The validity of this pointer is complicated. The pointer can be used safely until the stack grows again and reaches the height at which the pointer was taken. However, it is not guaranteed that the pointer always refers to a live stack slot, because a reallocation of the stack's backing array may occur.

[fjl]

I still think we should pursue this optimization idea, just need to find an abstraction that makes it less likely to mess things up.

[qianbin]

popptr is somewhat equivalent to peek & pop. Yes, it's dangerous especially for the stack with a slice backend. Agree that we need a better abstraction, I'll try.

[holiman]

It's actually not terribly dangerous (see #24860 (review)). When something is pushed to the stack, the item is copied, so the stack owns it.
The peek operation already returns the address.

returns a pointer to the 'inaccessible' area of the stack.

As long as the caller only uses it in a very small scope, it is safe, just like the peek operation. However, for peek it's kind of obvious that this is a borrowed item which we cannot hold on to, which is not as apparent with popptr. So I agree that it needs to be made more obvious that this operation must not be used for anything longer than a single-op duration.

[holiman]

Hm, this can actually already be achieved, kind of, without popptr, but using Back. For me it was actually even a bit faster

$ benchstat poptr.txt poptr2.txt 
name       old time/op    new time/op    delta
OpAdd64-6    17.4ns ± 6%    16.0ns ± 3%  -8.22%  (p=0.000 n=9+9)

name       old alloc/op   new alloc/op   delta
OpAdd64-6     0.00B          0.00B         ~     (all equal)

name       old allocs/op  new allocs/op  delta
OpAdd64-6      0.00           0.00         ~     (all equal)

Using this diff

diff --git a/core/vm/instructions.go b/core/vm/instructions.go
index 32b7e2ddb6..81d076d89d 100644
--- a/core/vm/instructions.go
+++ b/core/vm/instructions.go
@@ -27,8 +27,9 @@ import (
 )
 
 func opAdd(pc *uint64, interpreter *EVMInterpreter, scope *ScopeContext) ([]byte, error) {
-	x, y := scope.Stack.popptr(), scope.Stack.peek()
+	x, y := scope.Stack.Back(0), scope.Stack.Back(1)
 	y.Add(x, y)
+	scope.Stack.Discard()
 	return nil, nil
 }
 
diff --git a/core/vm/stack.go b/core/vm/stack.go
index d56c6a6cdf..0e00c2a612 100644
--- a/core/vm/stack.go
+++ b/core/vm/stack.go
@@ -64,6 +64,10 @@ func (st *Stack) popptr() (ret *uint256.Int) {
 	return
 }
 
+func (st *Stack) Discard() {
+	st.data = st.data[:len(st.data)-1]
+}
+
 func (st *Stack) len() int {
 	return len(st.data)
 }

But I don't know, it's hard to get really stable benchmarks.

[qianbin]

Back + Discard has one more bounds check, so won't be faster. How about call popptr popUnsafe 😅?

[holiman]

I don't think we want to merge this, since it doesn't markedly improve speed, but does make the stack interaction less simple