-
Notifications
You must be signed in to change notification settings - Fork 199
Reduce asm in Proxy.sol #1487
base: master
Are you sure you want to change the base?
Reduce asm in Proxy.sol #1487
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great suggestion, thank you @k06a! Sorry for taking a while to review.
What about a new version? Or you think performance will be different and in this place it is more matter? |
I'm not sure I understod the question. A new version of what? Do you mean upgrading to Solidity 0.6? |
@frangio I mean I added new commit to handle this 😁 |
I really like this! I would like someone else to look at it because it's quite critical. |
case 0 { revert(add(data, 32), returndatasize) } | ||
default { return(add(data, 32), returndatasize) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small optimization, although the compiler may catch this too.
case 0 { revert(add(data, 32), returndatasize) } | |
default { return(add(data, 32), returndatasize) } | |
let data_start := add(data, 32) | |
case 0 { revert(data_start, returndatasize) } | |
default { return(data_start, returndatasize) } |
I also considered using mload(data)
instead of returndatasize
but it seems to be more expensive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried mload(data)
, but tests failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's interesting. Did you understand why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frangio not yet, will check
No description provided.