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

Enable reordering of methods ids in generated evm code. #4858

Open
rmeissner opened this issue Aug 24, 2018 · 8 comments

Comments

@rmeissner
Copy link

commented Aug 24, 2018

Abstract

Each method has an ID. When a call to a smart contract is made the first step is to find the correct method based on that ID. Currently solidity generates code that checks all method ids that are available in alphabetical order. To allow the development of gas optimised contracts it would be useful if this order could be adjusted.

Motivation

To optimise gas we were discussing in our project to rename a method from execTransaction to execTransaction32785586. The reason for this was that the later would cost ~400 gas less (in some other cases the differences were even higher). The differences in gas has 2 reasons:

  1. As described above all methods are checked alphabetically, so if there are a lot of methods with a smaller method id, more checks need to be performed.
  2. The data costs for the method id

In the described case the method id for execTransaction was 0x6a761202 and for execTransaction32785586 was 0x00000081. Thus we saved 192 bytes for the data and the rest because of the order of the methods.

As we know that nearly all transactions interacting with out smart contract will use this method we would love to be able to adjust the order in which the method id's are checked.

Specification

A method annotated with TBD should always be checked first when searching the correct function. Only one method in a contract should be annotated with this.

Optional

The annotation could also specify an order (uint). When generating the code to check the method id it would first check the annotated methods ordered by the provided order and afterwards the other methods in alphabetical order.

Backwards Compatibility

This should not break any current behaviour.

@rmeissner

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

Test contract:

contract MethodIdTest {
    function foo(uint256 test) public { }
    function foo(uint256 test, uint256 test2) public { }
    function bar(uint256 test, uint256 test2) public { }
    function bar(uint256 test) public { }
}

generates the following method jumps:

mstore(0x40, 0x80)
jumpi(tag_1, lt(calldatasize, 0x4))
calldataload(0x0)
0x100000000000000000000000000000000000000000000000000000000
swap1
div
0xffffffff
and
dup1
0x423a132 // bar(uint256)
eq
tag_2
jumpi
dup1
0x4bc52f8 // foo(uint256,uint256)
eq
tag_3
jumpi
dup1
0x2fbebd38 // foo(uint256)
eq
tag_4
jumpi
dup1
0xae42e951 // bar(uint256,uint256)
eq
tag_5
jumpi

if bar(uint256,uint256) should be our primarily used method it would be more efficient to have the following code:

mstore(0x40, 0x80)
jumpi(tag_1, lt(calldatasize, 0x4))
calldataload(0x0)
0x100000000000000000000000000000000000000000000000000000000
swap1
div
0xffffffff
and
dup1
0xae42e951 // bar(uint256,uint256)
eq
tag_2
jumpi
dup1
0x4bc52f8 // foo(uint256,uint256)
eq
tag_3
jumpi
dup1
0x2fbebd38 // foo(uint256)
eq
tag_4
jumpi
dup1
0x423a132 // bar(uint256)
eq
tag_5
jumpi
@axic

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

I think this is an optimiser level problem (well, code generator to be precise, but from the users' perspective it is optimiser).

Therefore I would suggest not to pollute the language syntax with such, rather offer a configuration field in the compilation process (similar to how settings are provided for the optimiser):

`optimiseDispatchList`: [ 'myimportantfunction, 'myotherimportantfunction' ]

Where any function name listed here would take precedence in order they are listed and anything not listed would be freely sorted after them.

Also note that currently it is sorted by the selector (hash of the function name).

Last note, #4034 could obsolete this.

@axic axic added the feature label Aug 24, 2018
@rmeissner

This comment has been minimized.

Copy link
Author

commented Aug 24, 2018

We are currently not using the optimiser, but I do agree that it might be better to put it there.

Also the binary search will affect all calls made. But for most contracts not all methods are called with the same frequency. If you as a developer know what methods are called the most you should be able to optimise for it. I think #4034 doesn't solve this ;)

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2018

I think this has a very bad complexity / usefulness ratio to be included as a feature for the code generator. I do not understand why you care about 400 gas but do not use the optimizer.

How many functions do you have? Why do you think binary search would not reduce the gas costs?

@rmeissner

This comment has been minimized.

Copy link
Author

commented Sep 4, 2018

It was more that this seemed like an easy way to save gas. I do agree that if the complexity is too high this makes no sense. (Adjusting it by hand was quite easy ;) )

To the point why we don't use an optimizer: at some point we wanted to minimize potential sources of error. So by not using the optimizer we say the chances that the generated code contains an error is lower.

My comment concerning binary search was that a contract where the method ids have been sorted by usage could be more gas efficient than a contract that uses binary search (depends on the usage). But binary search will be definitely be an improvement compared to a current (unoptimized) solution.

@axic

This comment has been minimized.

Copy link
Member

commented Sep 25, 2018

#4034 is implemented now (albeit not merged for 0.5.0).

I think @rmeissner you should have a test when that's done, see the cost change and take the discussion from there.

I am still kind of leaning towards that adding an optimiser feature as described here is useful. It is better to offer such a safe solution, than leave contract developers do homebrew solutions.

@rmeissner

This comment has been minimized.

Copy link
Author

commented Sep 26, 2018

I assume it would be 0.5.1 in that case? We are planning on always testing against the newest compiler, so I will check it out 🙂

@chriseth

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.