openzeppelin-contracts icon indicating copy to clipboard operation
openzeppelin-contracts copied to clipboard

gas optimize Checkpoints.sol

Open grGred opened this issue 2 years ago • 1 comments

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas. As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with: for (uint256 i; i < numIterations; ++i) { Lines: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Checkpoints.sol#L54

>= is cheaper than > in if with optimizer disabled

>= is cheaper than > in require. And it's also cheaper sometimes to use >= than > with the optimizer disabled. With optimizer enabled they have the same amount of gas used (checked only 0.8.17).

    // 0.8.17 | optimizer off
    uint256 public gas;

    event log(uint256 time);

    function checkStrict() external {
        gas = gasleft();
        if (999999999999999999 > 0) {
            emit log(block.timestamp);
        } // gas 23329
        gas -= gasleft();
    }

    function checkNonStrict() external {
        gas = gasleft();
        if (999999999999999999 >= 1) {
            emit log(block.timestamp);
        } // gas 23326
        gas -= gasleft();
    }

I changed > 0 to >= 1 where possible.

  • [ ] Tests
  • [ ] Documentation
  • [x] Changelog entry

grGred avatar Oct 18 '22 22:10 grGred

Hello @grGred


No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas. As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with: for (uint256 i; i < numIterations; ++i) {

I would disagree with you here. I really don't think being explicit its an anti-pattern. Also, the measurement I made (using solidity 0.8.7 in remix) show that it doesn't save gas to have an implicit initialization


>= is cheaper than > in if with optimizer disabled

>= is cheaper than > in require. And it's also cheaper sometimes to use >= than > with the optimizer disabled. With optimizer enabled they have the same amount of gas used (checked only 0.8.17).

This is strange. The EVM includes gt and lt opcodes, but not gte and lte. The consequence is that gte is translated to lt iszero (not lt) ... and the equivalent for lte

With 0.8.17, I get the following measurements:

    function doSomething(uint256 a) public { // 221 gas
        if (a > 0) revert();
    }
    function doSomething(uint256 a) public { // 224 gas
        if (a >= 1) revert();
    }

The 3 gas difference exactly matched the addition of the "not" operation that I described above.

I'm not sure what happened with older compiler, but IMO we should not optimize for older compilers if the result is to get worst result with the latest ones. I strongly feel that:

  • developers should use the latest compiler whenever possible
  • all these micro optimization should be the responsibility of the compiler, and that we shouldn't compromise on code readability for the sake of saving 3 gas (particularly when the compiler can take care of that anyway).

Finally, you apparently missed line 3 of the Checkpoints.sol file // This file was procedurally generated from scripts/generate/templates/Checkpoints.js.

The change should go into the template file, and go through the generation process. That is needed for the tests to pass, and for us to get automated gas measurement from our test suite

Amxx avatar Oct 19 '22 08:10 Amxx

I agree with @Amxx's comments and based on them it seems this PR should be closed.

frangio avatar Dec 28 '22 23:12 frangio