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

Switch to .NET built-in MD5, it's 1.5X faster than your implementation #2598

Closed
1 of 2 tasks
alex-jitbit opened this issue May 1, 2023 · 4 comments
Closed
1 of 2 tasks
Labels
feature-request A feature should be added or improved. module/sdk-core p3 This is a minor priority issue perf queued

Comments

@alex-jitbit
Copy link

alex-jitbit commented May 1, 2023

Describe the feature

Method Mean Error StdDev Gen0 Allocated
NetCore_MD5_HashData 371.0 ns 9.32 ns 0.51 ns 0.0062 40 B
AWS_SDK_MD5 441.9 ns 9.98 ns 0.55 ns 0.1211 760 B

As you can see, .NET 6 built-in MD5 api is 1.5x faster and allocates 20x less memory. Any reason for not using .NET built in MD5.HashData() static method?

Use Case

MD5 hashing is used heavily in the SDK, switching to .NET 's built-in will speed things up a lot.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

AWS .NET SDK and/or Package version used

3.7.106.30

Targeted .NET Platform

.NET 5/6

Operating System and version

Windows 10, Ubuntu

@alex-jitbit alex-jitbit added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels May 1, 2023
@ashishdhingra
Copy link
Contributor

@alex-jitbit Good afternoon. Thanks for submitting feature request and the statistics w.r.t. performance on the MD5 calculation. Kindly note that AWS SDK for .NET also targets NetStandard20 netcoreapp3.1 and .NET Framework. This needs review with the team. Feel free to contribute a PR which could be reviewed by the team.

@ashishdhingra ashishdhingra added p2 This is a standard priority issue needs-review and removed needs-triage This issue or PR still needs to be triaged. labels May 1, 2023
@normj
Copy link
Member

normj commented May 3, 2023

@alex-jitbit I do like the idea of conditionally switching to the .NET version when it is available. We need to see how easy it would be for the SDK to use either implementation depending on the environment.

@ashishdhingra ashishdhingra added p3 This is a minor priority issue queued and removed p2 This is a standard priority issue needs-review labels May 5, 2023
@dscpinheiro
Copy link
Contributor

I'm going to close this as we have a separate issue with more opportunities to improve our usage of crypto algorithms: #3386

In V4 (currently in preview and being tracked in #3362), we'd like to bring System.IO.Hashing as a dependency and replace some of our implementations (if possible).

Copy link

github-actions bot commented Oct 3, 2024

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. module/sdk-core p3 This is a minor priority issue perf queued
Projects
None yet
Development

No branches or pull requests

4 participants