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

[release/8.0-staging] Fix performance of System.Security. Cryptography.Tests #95310

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 28, 2023

Backport of #95284 to release/8.0-staging

/cc @vcsjones @bartonjs

This is a test-only change that moves a few slow tests to outerloop to alleviate inner loop test timeouts.

Fixes #95094

These tests take a significant amount of time on Linux due to primality tests of RSA 16384.
@ghost
Copy link

ghost commented Nov 28, 2023

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

Issue Details

Backport of #95284 to release/8.0-staging

/cc @vcsjones

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Security

Milestone: -

@vcsjones vcsjones added the test-enhancement Improvements of test source code label Nov 28, 2023
@vcsjones
Copy link
Member

cc @carlossanlop

@carlossanlop
Copy link
Member

carlossanlop commented Dec 11, 2023

Thanks for tagging me. For future reference:

Area owners can merge their own PRs to the release/x.0-staging branches.

Test-only changes are considered tell mode, so area owners can apply the servicing-approved label - themselves (no need to go through their manager and throughTactics), which unblocks the check-servicing-label CI leg.

@dotnet/area-system-security can you please take a look?

@@ -10,7 +10,8 @@ namespace System.Security.Cryptography.Rsa.Tests
[SkipOnPlatform(TestPlatforms.Browser, "Not supported on Browser")]
public partial class ImportExport
{
public static bool Supports16384 { get; } = TestRsa16384();
private static readonly Lazy<bool> s_supports16384 = new Lazy<bool>(TestRsa16384);
public static bool Supports16384 => s_supports16384.Value;
Copy link
Member

Choose a reason for hiding this comment

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

TestRsa16384 itself takes a lot of time?

Copy link
Member

Choose a reason for hiding this comment

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

~30 seconds.

Copy link
Member

@stephentoub stephentoub Jan 2, 2024

Choose a reason for hiding this comment

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

Woh. Ok. That's not something we have any control over, right? That's all time spent in the OS implementation? Why does testing for even the support for this take so long?

Copy link
Member

Choose a reason for hiding this comment

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

That's not something we have any control over, right? That's all time spent in the OS implementation?
Yeah.

Why does testing for even the support for this take so long?

It basically boils down to: This is the largest possible RSA key. No one uses them (or should), but it is a theoretical max, so we want to test our upper boundary condition. Using an RSA key that large is a guaranteed way to hit performance problems.

The performance hit is in OpenSSL's EVP_PKEY_check. We use this method on key inputs so we can validate the key's consistency (making sure public parameters match private ones, etc). OpenSSL does a rather "thorough" job of this by running Miller-Rabin on the key input. We have no control over that or any flags, and we can't really avoid the called to EVP_PKEY_check.

Woh. Ok.

This PR cut the total execution time of System.Security.Cryptography on Linux by more than half on my Linux box.

@bartonjs bartonjs added the Servicing-approved Approved for servicing release label Jan 3, 2024
@carlossanlop carlossanlop merged commit 69d7219 into release/8.0-staging Jan 12, 2024
108 of 111 checks passed
@carlossanlop carlossanlop deleted the backport/pr-95284-to-release/8.0-staging branch January 12, 2024 22:06
@github-actions github-actions bot locked and limited conversation to collaborators Feb 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Servicing-approved Approved for servicing release test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants