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 exp2 with batching bit checks #77

Merged
merged 1 commit into from
Apr 17, 2022

Conversation

k06a
Copy link
Contributor

@k06a k06a commented Apr 4, 2022

I didn't find an easy way to build a gas report.

@PaulRBerg
Copy link
Owner

Ooh this is cool, thanks a lot for making this PR! I will review later.

Re gas reports - yeah, a discussed in #70, the current solution is to do what I did in the test/gas-estimations branch - add a bunch of console.log statements, run each function test set individually, and parse that manually in a Markdown file.

The crux of the problem is that hardhat-gas-reporter does not work for internal methods 😬

@k06a
Copy link
Contributor Author

k06a commented Apr 5, 2022

@PaulRBerg you could make transactions to view methods to make gas reporter work properly, but you will need to subtract calldata cost and 21000.

@PaulRBerg
Copy link
Owner

PaulRBerg commented Apr 16, 2022

I tested this with the following functions:

function measure_exp2(uint256 x) external pure returns (uint256 result) {
    result = x.exp2();
}

function measure_exp2_v2(uint256 x) external pure returns (uint256 result) {
    result = x.exp2_v2();
}

Where exp_v2 is the exp2 function as modified by this PR. Preliminary gas results:

input: 0x30000000000000000
exp2:       23196
exp2_v2: 21762

In this case, v2 was cheaper by 1434 gas.

input: 0x4e4acd9e83e425aee6
exp2:       23754
exp2_v2: 23973

In this case, v2 was more expensive by 219 gas.

So it looks like batching bit check lowers the gas costs only in some cases.

I think the best way to determine whether this PR should be merged or not is to rebase the test/gas-estimations branch to use the latest implementations from main, push the work on this PR onto that branch, and see how exp_v2 compares to exp across all test inputs.

Side note: I'm thinking to tentatively merge this on the staging branch and run the gas comparison at some later point .. I am planning on doing a major refactor soon and I would rather avoid having to deal with git conflicts.

@PaulRBerg
Copy link
Owner

PaulRBerg commented Apr 16, 2022

In the meantime, two quick questions for you @k06a:

  1. Could you briefly explain how and why batching bit checks like this works?
  2. Would it save even more gas if we went the extra mile and batched calls further down? I.e. wrap the ifs from 0x800000000000000 to 0x100000000000000 under an if that checks for if (x & 0xF00000000000000) > 0?

@k06a
Copy link
Contributor Author

k06a commented Apr 16, 2022

@PaulRBerg checking to & 0xFF00 is equal to 8 checks:

  • & 0x8000
  • & 0x4000
  • & 0x2000
  • & 0x1000
  • & 0x0800
  • & 0x0400
  • & 0x0200
  • & 0x0100

@k06a
Copy link
Contributor Author

k06a commented Apr 16, 2022

I suspect rewriting this in solidity assembly could win tens of percentages

@k06a
Copy link
Contributor Author

k06a commented Apr 16, 2022

BTW example 0x4e4acd9e83e425aee6 is the worst case for this optimization, since there are no zero bytes in this number representation.

@PaulRBerg
Copy link
Owner

PaulRBerg commented Apr 16, 2022

0x4e4acd9e83e425aee6 is the worst case for this optimization

This is a good point. In this case, it does seem like that this PR's optimization will lead to a gas reduction more often than it will lead to a gas increase. Also when the gas is reduced is reduced more than it increases when it is increased.

@PaulRBerg PaulRBerg changed the base branch from main to staging April 17, 2022 08:36
@PaulRBerg
Copy link
Owner

Going to merge this onto staging and after a few final gas comparisons, I will include it as part of the next release of PRBMath (which is probably going to be v3.0.0).

Thanks for your contribution, @k06a!

@PaulRBerg PaulRBerg merged commit 313741f into PaulRBerg:staging Apr 17, 2022
@PaulRBerg
Copy link
Owner

PaulRBerg commented Mar 27, 2023

Turns out that this PR introduced a bug in the Common.exp2 function: the bitmask 0xFF00000000 was being used twice, whereas 0xFF000000 should have been used the second time: #178

@andreivladbrg fixed this in #179.

@k06a
Copy link
Contributor Author

k06a commented Mar 28, 2023

@PaulRBerg I sincerely apologise for the bug, it was made unintentionally

@PaulRBerg
Copy link
Owner

No worries, @k06a! I never assumed otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants