morpho-org / morpho-utils

Repository gathering useful libraries and contracts.
GNU Affero General Public License v3.0
64 stars 1 forks source link

Refactoring log2 and its testing #88

Closed QGarchery closed 1 year ago

QGarchery commented 1 year ago

Gas report seems off when you use the internal functions which should really be private.

See https://github.com/morpho-dao/morpho-utils/pull/89 that shows a 645 gas usage for the new log2 (vs 624 before)

github-actions[bot] commented 1 year ago

Changes to gas cost

Generated at commit: 9728cd25a6f1a075c7cdaa21b25da2a2d5109309, compared to commit: 70b8eec7a2c7a3f8bf07a2e99c459ca86263057e

๐Ÿงพ Summary (20% most significant diffs)

Contract Method Avg (+/-) %
MathMock contract log2 +95 โŒ +15.22%

Full diff report ๐Ÿ‘‡
| Contract | Deployment Cost (+/-) | Method | Min (+/-) | % | Avg (+/-) | % | Median (+/-) | % | Max (+/-) | % | # Calls (+/-) | |:-|-:|:-|-:|-:|-:|-:|-:|-:|-:|-:|-:| | **MathMock contract** | 207,651 (+43,843) | _divUp_
_log2_
_max_
_min_
_zeroFloorSub_ | 328 (-10)
719 (+95)
379 (+45)
412 (+45)
361 (-22) | **-2.96%**
**+15.22%**
**+13.47%**
**+12.26%**
**-5.74%** | 410 (-10)
719 (+95)
379 (+45)
412 (+45)
361 (-22) | **-2.38%**
**+15.22%**
**+13.47%**
**+12.26%**
**-5.74%** | 431 (-10)
719 (+95)
379 (+45)
412 (+45)
361 (-22) | **-2.27%**
**+15.22%**
**+13.47%**
**+12.26%**
**-5.74%** | 431 (-10)
719 (+95)
379 (+45)
412 (+45)
361 (-22) | **-2.27%**
**+15.22%**
**+13.47%**
**+12.26%**
**-5.74%** | 5 (0)
2 (0)
1 (0)
1 (0)
1 (0) | | **MathRef contract** | 222,263 (+22,018) | _divUp_
_log2Dichotomy_
_log2Naive_
_max_
_min_
_zeroFloorSub_ | 577 (-22)
4,811 (+23)
88,837 (-22)
336 (+23)
368 (+23)
420 (-22) | **-3.67%**
**+0.48%**
**-0.02%**
**+7.35%**
**+6.67%**
**-4.98%** | 577 (-22)
4,811 (+23)
88,837 (-22)
336 (+23)
368 (+23)
420 (-22) | **-3.67%**
**+0.48%**
**-0.02%**
**+7.35%**
**+6.67%**
**-4.98%** | 577 (-22)
4,811 (+23)
88,837 (-22)
336 (+23)
368 (+23)
420 (-22) | **-3.67%**
**+0.48%**
**-0.02%**
**+7.35%**
**+6.67%**
**-4.98%** | 577 (-22)
4,811 (+23)
88,837 (-22)
336 (+23)
368 (+23)
420 (-22) | **-3.67%**
**+0.48%**
**-0.02%**
**+7.35%**
**+6.67%**
**-4.98%** | 1 (0)
1 (0)
1 (0)
1 (0)
1 (0)
1 (0) |
QGarchery commented 1 year ago

I'm not a big fan tbh (always the same debate). I must admit that it simplifies the tests quite a bit, but the fact that we add a useless return in the code for this reason looks also very weird.

Agreed, it looks weird. Adding the return actually saves some gas somehow

QGarchery commented 1 year ago

It seems that the changes are motivated by tests, is it correct? What is the impact on gas? I'm not sure to understand your comment below. On this PR I'm seeing 734 as gas cost for the log2.

If you look at the PR linked you see that the impact on gas is 645 gas, whereas before it was 624. The linked PR does not change the library, but only how it is used. Even then, nothing is changed if we run only the CompareLog2 tests. So I'm assuming the compilation is different because the compiler sees that the functions _roundDownToPowerOf2 and _lookupDeBruijn are used and cannot be inlined. This is why I said they those 2 functions should really be private (but then we couldn't test them)

QGarchery commented 1 year ago

Seen with @MerlinEgalite: we are closing this for now as we feel it makes it less efficient while not providing enough clarity.