core/vm/memory: fix Memory.Set32 use make fill with 0 to []byte is better#24776
core/vm/memory: fix Memory.Set32 use make fill with 0 to []byte is better#24776xavierzho wants to merge 1 commit intoethereum:masterfrom xavierzho:master
Conversation
…tter than literal and Memory.GetCopy add "GetCopy" prefix to comment
xavierzho
requested review from
holiman,
karalabe and
rjl493456442
as code owners
holiman
left a comment
holiman
left a comment
There was a problem hiding this comment.
I don't think this is an improvement, other than the fix for the docstring on GetCopy
| } | ||
| // 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(m.store[offset:offset+bit32], empty) |
There was a problem hiding this comment.
I don't see any reason why this would be faster. As it was, the compiler can replace the []byte{...} with a constant, which it might not do with a dynamically allocated empty.
I have a recollection that we added this ugly hack a long time ago, when we detected that it was indeed faster to initalize it like this.
There was a problem hiding this comment.
I think this performance gap is acceptable.
| "github.com/holiman/uint256" | ||
| ) | ||
|
|
||
| const bit32 = 32 |
There was a problem hiding this comment.
I don't see why a constant bit32 is easier to read than 32 ?
There was a problem hiding this comment.
oh, This is my code cleanliness.
|
Oh, see #16939 (comment) for more context |
The performance of the evm is very important, and we're usually very conservative in making changes. Making changes only to somewhat increase legibility but incur performance penalty is not worth it for us. Still. thanks for the effort! |
2 participants
I think using make function fill []byte is obvious than literal,
Memory.GetCopy add "GetCopy" prefix to comment.