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

Improve and dedup GetNonZeroBytes #81340

Merged
merged 1 commit into from Jan 31, 2023
Merged

Conversation

stephentoub
Copy link
Member

  • RsaPaddingProcessor has its own complete copy; that now appears to be an unnecessary duplication.
  • If a zero byte is found, the implementation currently looks at the rest of the bytes byte by byte. We expect 0s to be rare, so copy in bulk instead.
Method Toolchain Length Mean Ratio
GetNonZero \main\corerun.exe 64 184.0 ns 1.00
GetNonZero \pr\corerun.exe 64 176.5 ns 0.93
GetNonZero \main\corerun.exe 1024 1,053.4 ns 1.00
GetNonZero \pr\corerun.exe 1024 660.5 ns 0.63
GetNonZero \main\corerun.exe 16384 15,511.9 ns 1.00
GetNonZero \pr\corerun.exe 16384 6,679.8 ns 0.43
[Params(64, 1024, 16384)]
public int Length { get; set; }

private byte[] _buffer;

[GlobalSetup]
public void Setup() => _buffer = new byte[Length];

[Benchmark]
public void GetNonZero() => RandomNumberGenerator.Create().GetNonZeroBytes(_buffer);

- RsaPaddingProcessor has its own complete copy; that now appears to be an unnecessary duplication.
- If a zero byte is found, the implementation currently looks at the rest of the bytes byte by byte.  We expect 0s to be rare, so copy in bulk instead.
@ghost
Copy link

ghost commented Jan 30, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details
  • RsaPaddingProcessor has its own complete copy; that now appears to be an unnecessary duplication.
  • If a zero byte is found, the implementation currently looks at the rest of the bytes byte by byte. We expect 0s to be rare, so copy in bulk instead.
Method Toolchain Length Mean Ratio
GetNonZero \main\corerun.exe 64 184.0 ns 1.00
GetNonZero \pr\corerun.exe 64 176.5 ns 0.93
GetNonZero \main\corerun.exe 1024 1,053.4 ns 1.00
GetNonZero \pr\corerun.exe 1024 660.5 ns 0.63
GetNonZero \main\corerun.exe 16384 15,511.9 ns 1.00
GetNonZero \pr\corerun.exe 16384 6,679.8 ns 0.43
[Params(64, 1024, 16384)]
public int Length { get; set; }

private byte[] _buffer;

[GlobalSetup]
public void Setup() => _buffer = new byte[Length];

[Benchmark]
public void GetNonZero() => RandomNumberGenerator.Create().GetNonZeroBytes(_buffer);
Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Security

Milestone: -

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

LGTM, removing duplicated code and making it faster at the same time is always good! 👍

@stephentoub stephentoub merged commit 4cbbaea into dotnet:main Jan 31, 2023
@stephentoub stephentoub deleted the improvenonzero branch January 31, 2023 12:51
@bartonjs bartonjs added the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Feb 7, 2023
@dotnet dotnet locked as resolved and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants