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

Slightly gas saving in avg function #89

Closed
wants to merge 0 commits into from

Conversation

atarpara
Copy link
Contributor

@atarpara atarpara commented May 4, 2022

I remove the unnecessary operation in avg function.

@PaulRBerg
Copy link
Owner

Hi @atarpara! Thanks for your contribution. Unfortunately, I cannot merge this. In fact, I cannot even review it:

  • The code does not compile, it fails with the error "ParserError: Expected ';' but got '}'"
  • The linter job does not pass (see CI logs) (make sure to install the Prettier extension for VSCode)
  • This PR does not include an explanation for why the proposed change brings about a reduction in gas costs while simultaneously keeping the behavior of the avg function the same across all possible inputs and outputs

@atarpara
Copy link
Contributor Author

@PaulRBerg I solved the compiler error.

@PaulRBerg
Copy link
Owner

Hi @atarpara, thanks for that. Would you mind explaining how your gas saving works?

@atarpara
Copy link
Contributor Author

atarpara commented May 20, 2022

Reduce the Number of Operation

Old Way: (x >> 1) + (y >> 1) + (x & y & 1) - Need 6 Operation

New Way : (x & y) + ((x^y) >> 1) - Need 4 Operation

More Info About is Here.

@PaulRBerg
Copy link
Owner

Hello @atarpara, I'm sorry for the delayed response.

I ran a few tests and indeed you are right - your approach is faster than mine.

Happy to accept this PR, but could I kindly ask you to re-apply your changes onto the tip of the staging branch, git push with force to your branch, and then make staging the base of this PR?

Thank you!

@PaulRBerg
Copy link
Owner

Hi @atarpara, I'm sorry, I accidentally closed your PR.

But I managed to implement your proposal, and credit you as a git author in commit a490c08.

Thanks again for your contribution.

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