Some VM optimizations #2128
Some VM optimizations #2128
Conversation
It is always copied. Signed-off-by: Evgeniy Stratonikov <[email protected]>
Remove one indirection step. `` name old time/op new time/op delta MakeInt-8 79.7ns ± 8% 56.2ns ± 8% -29.44% (p=0.000 n=10+10) name old alloc/op new alloc/op delta MakeInt-8 48.0B ± 0% 40.0B ± 0% -16.67% (p=0.000 n=10+10) name old allocs/op new allocs/op delta MakeInt-8 3.00 ± 0% 2.00 ± 0% -33.33% (p=0.000 n=10+10) ``` Signed-off-by: Evgeniy Stratonikov <[email protected]>
| @@ -171,8 +171,7 @@ func (c *Context) Next() (opcode.Opcode, []byte, error) { | |||
| if err != nil { | |||
| return instr, nil, err | |||
| } | |||
| parameter := make([]byte, numtoread) | |||
| copy(parameter, c.prog[c.nextip:c.nextip+numtoread]) | |||
| parameter := c.prog[c.nextip : c.nextip+numtoread] | |||
There was a problem hiding this comment.
This reminds me of golang/go#27975, but until we have it we need to trace what exactly is happening to this parameter.
We have it being used in compiler/test code, but it's not dangerous there.
ParseSignatureContract and ParseMultiSigContract return it, but all current users are safe, they only read these values.
PrintOps also reads things.
And then we have v.execute with a hell a lot of options. It's used for GetPrice, but it doesn't care. It's used to push integers onto the stack, but that inevitably leads to bigint.FromBytes, so not a problem. It's used for PUSHDATA, but we'll get to it later. Then PUSHA, ISTYPE, CONVERT, slot-related opcodes, NEW*, JMP*, CALL*, and *TRY* all read their parameters from it and it's all fine.
The most problematic case is PUSHDATA, because that's where a ByteArray item is created containing reference to this data. By design ByteArray is an immutable type (kinda like string), unlike Buffer, but we still need to check it.
We can use it in MEMCPY, but only as a source that we copy data from. CAT can take it, but it also copies data from both of its parameters. SUBSTR, LEFT and RIGHT copy too. PICKITEM reads one byte and SIZE takes a len(). It can also be used in exception-handling code, but it's converted to string there immediately.
We have conversions though. Conversions to ByteArray don't copy data, but it's ByteArray, so it should be safe, Buffer does slice.Copy and Integer and Bool return new respective values. Equals doesn't change anything.
So, we're safe?
There was a problem hiding this comment.
Anyway, I think it's worth mentioning in Next documentation that this parameter points to script data and user is responsible for copying it if needed.
Signed-off-by: Evgeniy Stratonikov <[email protected]>
`Next` doesn't longer copy parameter. Signed-off-by: Evgeniy Stratonikov <[email protected]>
|
@roman-khimov updated for |
Codecov Report
@@ Coverage Diff @@
## master #2128 +/- ##
==========================================
+ Coverage 84.07% 84.44% +0.37%
==========================================
Files 290 291 +1
Lines 27356 27966 +610
==========================================
+ Hits 22999 23616 +617
+ Misses 3085 3069 -16
- Partials 1272 1281 +9
Continue to review full report at Codecov.
|
Meh, I wanted to tell about |
|
@roman-khimov Go disallows declaring methods on aliases to interfaces. |
| return &ByteArray{ | ||
| value: b, | ||
| } | ||
| return (*ByteArray)(&b) | ||
| } | ||
|
|
||
| // Value implements Item interface. | ||
| func (i *ByteArray) Value() interface{} { |
There was a problem hiding this comment.
Can pointer be avoided here?
There was a problem hiding this comment.
It certainly can, though I am not sure this will speed-up things: while there is 1 less indirection step, we will copy 2.5 as much data for every method.
There was a problem hiding this comment.
OK, let it be the way it is for now and we can always get back to it later.
There was a problem hiding this comment.
I'd expect it to be somewhat faster, but it just needs some testing.
``` name old time/op new time/op delta RefCounter_Add-8 44.8ns ± 4% 11.7ns ± 3% -73.94% (p=0.000 n=10+10) ``` Signed-off-by: Evgeniy Stratonikov <[email protected]>
Signed-off-by: Evgeniy Stratonikov <[email protected]>
Signed-off-by: Evgeniy Stratonikov <[email protected]>
BigIntegerandBoolitems.