KEMBAR78
Improve and dedup GetNonZeroBytes by stephentoub · Pull Request #81340 · dotnet/runtime · GitHub
Skip to content

Improve and dedup GetNonZeroBytes #81340

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 31, 2023
Merged

Improve and dedup GetNonZeroBytes #81340

merged 1 commit into from
Jan 31, 2023

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);

@stephentoub
Improve and dedup GetNonZeroBytes
78c3ab3 
- 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 ghost assigned stephentoub Jan 30, 2023
@ghost ghost added the area-System.Security label Jan 30, 2023
@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: -

adamsitnik
adamsitnik approved these changes Jan 31, 2023
View reviewed changes
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
@ghost ghost locked as resolved and limited conversation to collaborators Mar 9, 2023
@bartonjs bartonjs removed the cryptographic-docs-impact Issues impacting cryptographic docs. Cleared and reused after documentation is updated each release. label Aug 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@adamsitnik adamsitnik adamsitnik approved these changes

+1 more reviewer

@filipnavara filipnavara filipnavara approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@stephentoub stephentoub

Labels

area-System.Security

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@stephentoub @filipnavara @adamsitnik @bartonjs