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

Optimize gas for _getFraction in AmountDeriver #384

Merged
merged 9 commits into from
Jun 8, 2022
14 changes: 7 additions & 7 deletions contracts/lib/AmountDeriver.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
AmountDerivationErrors
} from "../interfaces/AmountDerivationErrors.sol";

import "./AmountDeriverConstants.sol";

/**
* @title AmountDeriver
* @author 0age
Expand Down Expand Up @@ -98,16 +100,14 @@ contract AmountDeriver is AmountDerivationErrors {

// Ensure fraction can be applied to the value with no remainder. Note
// that the denominator cannot be zero.
bool exact;
assembly {
// Ensure new value contains no remainder via mulmod operator.
// Credit to @hrkrshnn + @axic for proposing this optimal solution.
exact := iszero(mulmod(value, numerator, denominator))
}

// Ensure that division gave a final result with no remainder.
if (!exact) {
revert InexactFraction();
// Further optimization by @Chomtana and @0age
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove the credit here :)

if mulmod(value, numerator, denominator) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this does anything? The double iszero(iszero(...)) should be optimised out.

Copy link
Contributor Author

@Chomtana Chomtana Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just mulmod(value, numerator, denominator) should be worked as @0age said since we need to test whether mulmod(value, numerator, denominator) is not zero and it is unsigned which means mulmod(value, numerator, denominator) is truthy

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if the change does anything gas wise. The double iszero should be removed by the optimiser.

Copy link
Contributor Author

@Chomtana Chomtana Jun 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does by removing unnecessary exact variable.

exact := iszero(mulmod(value, numerator, denominator))
if iszero(exact) {
...
}

Don't sure whether optimiser can remove double iszero that is stored into intermediate variable first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are many optimiser stages, including one which operates on opcode level, where known stack movement are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just tested gas on remix using 2 separate contracts with same function name getFraction for both case and adding
a global variable assignment to the end

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.13;

error InexactFraction();

// error InexactFraction() @ AmountDerivationErrors.sol
uint256 constant InexactFraction_error_signature = (
    0xc63cf08900000000000000000000000000000000000000000000000000000000
);
uint256 constant InexactFraction_error_len = 0x20;

contract TestMulMod {
    uint256 public x;

    /**
     * @dev Internal pure function to return a fraction of a given value and to
     *      ensure the resultant value does not have any fractional component.
     *      Note that this function assumes that zero will never be supplied as
     *      the denominator parameter; invalid / undefined behavior will result
     *      should a denominator of zero be provided.
     *
     * @param numerator   A value indicating the portion of the order that
     *                    should be filled.
     * @param denominator A value indicating the total size of the order. Note
     *                    that this value cannot be equal to zero.
     * @param value       The value for which to compute the fraction.
     *
     * @return newValue The value after applying the fraction.
     */
    function getFraction(
        uint256 numerator,
        uint256 denominator,
        uint256 value
    ) public returns (uint256 newValue) {
        // Return value early in cases where the fraction resolves to 1.
        if (numerator == denominator) {
            return value;
        }

        // Ensure fraction can be applied to the value with no remainder. Note
        // that the denominator cannot be zero.
        assembly {
            // Ensure new value contains no remainder via mulmod operator.
            // Credit to @hrkrshnn + @axic for proposing this optimal solution.
            // Further optimization by @Chomtana and @0age
            if mulmod(value, numerator, denominator) {
                mstore(0, InexactFraction_error_signature)
                revert(0, InexactFraction_error_len)
            }
        }

        // Multiply the numerator by the value and ensure no overflow occurs.
        uint256 valueTimesNumerator = value * numerator;

        // Divide and check for remainder. Note that denominator cannot be zero.
        assembly {
            // Perform division without zero check.
            newValue := div(valueTimesNumerator, denominator)
        }

        x = newValue;
    }

    /**
     * @dev Internal pure function to return a fraction of a given value and to
     *      ensure the resultant value does not have any fractional component.
     *      Note that this function assumes that zero will never be supplied as
     *      the denominator parameter; invalid / undefined behavior will result
     *      should a denominator of zero be provided.
     *
     * @param numerator   A value indicating the portion of the order that
     *                    should be filled.
     * @param denominator A value indicating the total size of the order. Note
     *                    that this value cannot be equal to zero.
     * @param value       The value for which to compute the fraction.
     *
     * @return newValue The value after applying the fraction.
     */
    // function getFraction(
    //     uint256 numerator,
    //     uint256 denominator,
    //     uint256 value
    // ) public returns (uint256 newValue) {
    //     // Return value early in cases where the fraction resolves to 1.
    //     if (numerator == denominator) {
    //         return value;
    //     }

    //     // Ensure fraction can be applied to the value with no remainder. Note
    //     // that the denominator cannot be zero.
    //     bool exact;
    //     assembly {
    //         // Ensure new value contains no remainder via mulmod operator.
    //         // Credit to @hrkrshnn + @axic for proposing this optimal solution.
    //         exact := iszero(mulmod(value, numerator, denominator))
    //     }

    //     // Ensure that division gave a final result with no remainder.
    //     if (!exact) {
    //         revert InexactFraction();
    //     }

    //     // Multiply the numerator by the value and ensure no overflow occurs.
    //     uint256 valueTimesNumerator = value * numerator;

    //     // Divide and check for remainder. Note that denominator cannot be zero.
    //     assembly {
    //         // Perform division without zero check.
    //         newValue := div(valueTimesNumerator, denominator)
    //     }

    //     x = newValue;
    // }
}

Normal case (2, 5, 10)
Before: 24189 gas
After: 24176 gas

Revert case (2, 5, 8)
Before: 21845 gas
After: 21804 gas

mstore(0, InexactFraction_error_signature)
revert(0, InexactFraction_error_len)
}
}

// Multiply the numerator by the value and ensure no overflow occurs.
Expand Down
42 changes: 42 additions & 0 deletions contracts/lib/AmountDeriverConstants.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// SPDX-License-Identifier: MIT
pragma solidity >=0.8.7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use 0.8.7 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is copied from ConsiderationConstants.sol let ask @0age should we change to >=0.8.13 or are there any specific cases? In my opinion, it should be >=0.8.13.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
There are too many >=0.8.7 so, separate PR should be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/*
* -------------------------- Disambiguation & Other Notes ---------------------
* - The term "head" is used as it is in the documentation for ABI encoding,
* but only in reference to dynamic types, i.e. it always refers to the
* offset or pointer to the body of a dynamic type. In calldata, the head
* is always an offset (relative to the parent object), while in memory,
* the head is always the pointer to the body. More information found here:
* https://docs.soliditylang.org/en/v0.8.14/abi-spec.html#argument-encoding
* - Note that the length of an array is separate from and precedes the
* head of the array.
*
* - The term "body" is used in place of the term "head" used in the ABI
* documentation. It refers to the start of the data for a dynamic type,
* e.g. the first word of a struct or the first word of the first element
* in an array.
*
* - The term "pointer" is used to describe the absolute position of a value
* and never an offset relative to another value.
* - The suffix "_ptr" refers to a memory pointer.
* - The suffix "_cdPtr" refers to a calldata pointer.
*
* - The term "offset" is used to describe the position of a value relative
* to some parent value. For example, OrderParameters_conduit_offset is the
* offset to the "conduit" value in the OrderParameters struct relative to
* the start of the body.
* - Note: Offsets are used to derive pointers.
*
* - Some structs have pointers defined for all of their fields in this file.
* Lines which are commented out are fields that are not used in the
* codebase but have been left in for readability.
*/

// Declare constant for errors related to amount derivation.

// error InexactFraction() @ AmountDerivationErrors.sol
uint256 constant InexactFraction_error_signature = (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should have the error signature as a comment, i.e. error InexactFraction().

Also a reference to the definition AmountDerivationErrors.sol may be useful. Note: do not remove the Solidity definition otherwise it will not be included in the output JSONs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More comments has been added

0xc63cf08900000000000000000000000000000000000000000000000000000000
);
uint256 constant InexactFraction_error_len = 0x20;