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

Fix SHA3 performance degradation #472

Merged
merged 1 commit into from
May 5, 2023

Conversation

aewag
Copy link
Contributor

@aewag aewag commented May 2, 2023

Related issue #468

@Slixe Could you please give this branchof the SHA3 crate a try and see if it fixes the performance degradation?

If it does, I would finalize it and mark it as ready.

@Slixe
Copy link

Slixe commented May 2, 2023

LGTM.

v1.0.6:
~ 10 MH/s

v1.0.7 (bug introduced):
~ 4 MH/s

This pull request seems to fix it because I'm at ~ 10 MH/s with it again.

Thanks for your work 🙂

@tarcieri
Copy link
Member

tarcieri commented May 2, 2023

@aewag if you can remove WIP/draft we can get this merged

@aewag
Copy link
Contributor Author

aewag commented May 3, 2023

@aewag if you can remove WIP/draft we can get this merged

will do. just one question: shall the sha3 crate take the keccak from git or do you plan to release a patch?

@tarcieri
Copy link
Member

tarcieri commented May 4, 2023

@aewag keccak v0.1.4 has been released

@aewag aewag force-pushed the fix-sha3-performance-degradation branch from f619f23 to f695ac2 Compare May 5, 2023 08:09
@aewag aewag marked this pull request as ready for review May 5, 2023 08:25
@aewag aewag changed the title WIP: Fix SHA3 performance degradation Fix SHA3 performance degradation May 5, 2023
@aewag
Copy link
Contributor Author

aewag commented May 5, 2023

@tarcieri nice, just updated the PR and flagged it as ready

@tarcieri tarcieri merged commit 4d13ef1 into RustCrypto:master May 5, 2023
@aewag aewag deleted the fix-sha3-performance-degradation branch May 5, 2023 12:59
@tarcieri tarcieri mentioned this pull request May 5, 2023
aleksuss added a commit to aurora-is-near/aurora-engine that referenced this pull request May 8, 2023
<!--
Thanks for submitting a pull request! Here are some helpful tips:

* Always create branches on and target the `develop` branch.
* Run all the tests locally and ensure that they are passing.
* Run `make format` to ensure that the code is formatted.
* Run `make check` to ensure that all checks passed successfully.
* Small commits and contributions that attempt one single goal is
preferable.
* If the idea changes or adds anything functional which will affect
users, an
AIP discussion is required first on the Aurora forum: 

https://forum.aurora.dev/discussions/AIPs%20(Aurora%20Improvement%20Proposals).
* Avoid breaking the public API (namely in engine/src/lib.rs) unless
required.
* If your PR is a WIP, ensure that you enable "draft" mode.
* Your first PRs won't use the CI automatically unless a maintainer
starts.
If this is not your first PR, please do NOT abuse the CI resources.

Checklist:
- [ ] I have performed a self-review of my code
- [ ] I have documented my code, particularly in the hard-to-understand
areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests to prove my fix or new feature is effective and
works
- [ ] Any dependent changes have been merged
- [ ] The PR is targeting the `develop` branch and not `master`
- [ ] I have pre-squashed my commits into a single commit and rebased.
-->

## Description

The PR updates the transitive dependency `sha3` to 0.10.8, which fixes
the regression in gas costs (in the tests) regarding this
[fix](RustCrypto/hashes#472).

## Performance / NEAR gas cost considerations

A tiny gas costs improvement.

## Testing

<!--
Please describe the tests that you ran to verify your changes.
-->

## How should this be reviewed

<!--
Include any recommendations of areas to be careful of to ensure that the
reviewers use extra attention.
-->

## Additional information

<!--
Include any additional information which you think should be in this PR,
such
as prior arts, future extensions, unresolved problems, or a TODO list
which
should be followed up.
-->
birchmd pushed a commit to aurora-is-near/aurora-engine that referenced this pull request May 11, 2023
<!--
Thanks for submitting a pull request! Here are some helpful tips:

* Always create branches on and target the `develop` branch.
* Run all the tests locally and ensure that they are passing.
* Run `make format` to ensure that the code is formatted.
* Run `make check` to ensure that all checks passed successfully.
* Small commits and contributions that attempt one single goal is
preferable.
* If the idea changes or adds anything functional which will affect
users, an
AIP discussion is required first on the Aurora forum: 

https://forum.aurora.dev/discussions/AIPs%20(Aurora%20Improvement%20Proposals).
* Avoid breaking the public API (namely in engine/src/lib.rs) unless
required.
* If your PR is a WIP, ensure that you enable "draft" mode.
* Your first PRs won't use the CI automatically unless a maintainer
starts.
If this is not your first PR, please do NOT abuse the CI resources.

Checklist:
- [ ] I have performed a self-review of my code
- [ ] I have documented my code, particularly in the hard-to-understand
areas
- [ ] I have made corresponding changes to the documentation
- [ ] I have added tests to prove my fix or new feature is effective and
works
- [ ] Any dependent changes have been merged
- [ ] The PR is targeting the `develop` branch and not `master`
- [ ] I have pre-squashed my commits into a single commit and rebased.
-->

## Description

The PR updates the transitive dependency `sha3` to 0.10.8, which fixes
the regression in gas costs (in the tests) regarding this
[fix](RustCrypto/hashes#472).

## Performance / NEAR gas cost considerations

A tiny gas costs improvement.

## Testing

<!--
Please describe the tests that you ran to verify your changes.
-->

## How should this be reviewed

<!--
Include any recommendations of areas to be careful of to ensure that the
reviewers use extra attention.
-->

## Additional information

<!--
Include any additional information which you think should be in this PR,
such
as prior arts, future extensions, unresolved problems, or a TODO list
which
should be followed up.
-->
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.

3 participants