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

Added Intrinsics and some performance optimisations #58

Closed
wants to merge 3 commits into from

Conversation

macaba
Copy link

@macaba macaba commented Aug 28, 2020

  • Added Intrinsics fast path
  • Added ability for benchmarks to run in .NET48 (for a pre/post intrinsics comparison)

Hello!

I finally got round to pulling in all your latest updates. Fantastic work on the Span & BitUtils support.

A quick bit of benchmarking.
Before adding my pure intrinsics processing (i.e. using BitUtils which I suspect emits some intrinsics under the hood):

image

A decent improvement vs. Framework.

After adding intrinsics:

image

I'm particularly impressed with the memory allocated when using the Span Encrypt method. When I did a memory trace, the GetMacDataRfc8439 was the only method significantly allocating memory. It would be ideal to come up with some sort of caching or memory context scheme for this.

- Added Intrinsics fast path
- Added Thread Static cache for GetMacDataRfc8439
- Added ability for benchmarks to run in .NET48 (for a pre/post intrinsics comparison)
@macaba
Copy link
Author

macaba commented Aug 29, 2020

Commit 28d47c9 added SpanOwner<T> - this is a Span rented from an ArrayPool with IDisposable for automatically returning to the pool.
This allowed me to eliminate the memory allocation in GetMacKey and GetMacDataRfc8439.

@daviddesmet daviddesmet added the enhancement New feature or request label Aug 31, 2020
@daviddesmet
Copy link
Owner

Hello macaba,

Glad to hear from you again. 😀
Thank you, @moien007 helped a lot on it.

Allow me some time to check it in the different platforms since the CI didn't kick on Azure DevOps.
So far, it looks great to me. Awesome job on there!

Copy link
Owner

@daviddesmet daviddesmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@macaba noticed why the CI failed, could you kindly address this little issue?

Comment on lines +4 to +5
<TargetFrameworks>netstandard1.6;netstandard2.0;netstandard2.1;netcoreapp3.1;netcoreapp5.0</TargetFrameworks>
<TargetFrameworks Condition="'$(OS)' != 'Unix'">netstandard1.6;netstandard2.0;netstandard2.1;netcoreapp3.1;netcoreapp5.0;net45;net48</TargetFrameworks>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove .NET 5 since it's still in preview, so the CI doesn't complain?
It will surely be added once .NET 5 is released by the end of the year.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed an update to support .NET 5 now that is RTM.

@daviddesmet
Copy link
Owner

daviddesmet commented Aug 31, 2020

@macaba I did a quick test under Windows and the Wycheproof test vectors from ChaCha20Poly1305 and XChaCha20Poly1305 are failing. Could you check those out, please? Apparently, the changes on GetMacKey and GetMacDataRfc8439 caused a regression since the test cases fail when verifying the MACs.

@daviddesmet
Copy link
Owner

daviddesmet commented Sep 18, 2020

Hello @macaba

I wonder if you got any progress on this? I'm planning on releasing the updated version on NuGet tagged as 2.0.0 since in the current state (prior to the PR) we got some breaking changes. Would be nice to have yours included.

@daviddesmet daviddesmet linked an issue Sep 18, 2020 that may be closed by this pull request
@daviddesmet daviddesmet added the major Requiring a major version update according to semantic versioning label Sep 28, 2020
@daviddesmet
Copy link
Owner

@macaba @moien007
FYI, I've just released v2.0.0 with all the previous improvements. This PR is still pending until the regression gets addressed.

@daviddesmet daviddesmet changed the title Performance optimisations Added Intrinsics and some performance optimisations Nov 22, 2020
@stale
Copy link

stale bot commented Jan 23, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in one week if no further activity occurs.
Thank you for your contributions.

@stale stale bot added the stale This issue had no activity for two months label Jan 23, 2021
@stale stale bot closed this Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request major Requiring a major version update according to semantic versioning stale This issue had no activity for two months
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Runtime.Intrinsics.X86
2 participants