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

Onboard Tests for OpenSSL 3 #3388

Merged
merged 10 commits into from
Feb 2, 2023
Merged

Onboard Tests for OpenSSL 3 #3388

merged 10 commits into from
Feb 2, 2023

Conversation

nibanks
Copy link
Member

@nibanks nibanks commented Feb 1, 2023

Description

Adds tests for the recently onboarded OpenSSL 3 build support.

  • OpenSSL3 has worse throughput and HPS perf
  • Tests that rely on resumption ticket callbacks are failing
  • Refactored test filters so they are disabled by default in the build so filters are mostly unnecessary now

Testing

Automation

Documentation

N/A

@nibanks nibanks requested a review from a team as a code owner February 1, 2023 14:42
@nibanks
Copy link
Member Author

nibanks commented Feb 1, 2023

@wfurt it looks like Windows build fails?

  OpenSSL configure
  Can't open perl script "D:/a/1/msquic/submodules/openssl/Configure": No such file or directory
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(247,5): error MSB8066: Custom build for 'D:\a\1\msquic\build\windows\x64_openssl3\CMakeFiles\0f71981407617e4ecf2e545cde1afa36\1f86d0fb6f165948.rule;D:\a\1\msquic\build\windows\x64_openssl3\CMakeFiles\0f71981407617e4ecf2e545cde1afa36\468b167170316640.rule;D:\a\1\msquic\build\windows\x64_openssl3\CMakeFiles\0f71981407617e4ecf2e545cde1afa36\OpenSSL_Target.rule;D:\a\1\msquic\submodules\CMakeLists.txt' exited with code 2. [D:\a\1\msquic\build\windows\x64_openssl3\_deps\opensslquic-build\OpenSSL_Target.vcxproj]

@nibanks
Copy link
Member Author

nibanks commented Feb 1, 2023

Configure

Ok. I think I fixed it in my most recent commit. Builds locally now.

@nibanks nibanks added the blocked label Feb 1, 2023
@nibanks
Copy link
Member Author

nibanks commented Feb 1, 2023

Marking currently blocked on the test failures.

@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

Hi @paulidale. We've started looking at OpenSSL 3.0 again, and it looks like HPS is pretty terrible with OpenSSL 3.0. Here's the data on Linux for instance.

Linux HPS Perf v1.1.1

Test Run Took 00:00:10.9550682
Run 1: 9764 HPS
Test Run Took 00:00:10.9642312
Run 2: 9990 HPS
Test Run Took 00:00:11.0954863
Run 3: 10086 HPS
Test Run Took 00:00:10.9084721
Run 4: 10276 HPS
Test Run Took 00:00:10.9453742
Run 5: 10166 HPS
Median: 10086 HPS

Linux HPS Perf v3.0

Test Run Took 00:00:11.1963015
Run 1: 1891 HPS
Test Run Took 00:00:11.4757571
Run 2: 1949 HPS
Test Run Took 00:00:11.3739960
Run 3: 1879 HPS
Test Run Took 00:00:11.3007463
Run 4: 1888 HPS
Test Run Took 00:00:11.4470352
Run 5: 1978 HPS
Median: 1891 HPS

@wfurt
Copy link
Member

wfurt commented Feb 2, 2023

nice @nibanks. At lest we can now run them side by side. I assume the test does not depend on PGO?

@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

@wfurt yes, we can run them side by side now, and PGO isn't required.

@paulidale it seems throughput also suffered (again, here's Linux data)

v1.1.1

Test Run Took 00:00:13.9332980
Run 1: 5627775 kbps
Test Run Took 00:00:14.0494522
Run 2: 5535505 kbps
Test Run Took 00:00:13.8022312
Run 3: 5621339 kbps
Test Run Took 00:00:14.0703712
Run 4: 5354296 kbps
Test Run Took 00:00:14.0590394
Run 5: 5633485 kbps
Median: 5621339 kbps

v3.0

Test Run Took 00:00:14.1823326
Run 1: 4650012 kbps
Test Run Took 00:00:13.9744334
Run 2: 4620731 kbps
Test Run Took 00:00:14.1512972
Run 3: 4618331 kbps
Test Run Took 00:00:13.9970742
Run 4: 4698704 kbps
Test Run Took 00:00:14.2107039
Run 5: 4655014 kbps
Median: 4650012 kbps

@t8m
Copy link

t8m commented Feb 2, 2023

Could you please try also openssl-3.1.0-beta1?

@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

Could you please try also openssl-3.1.0-beta1?

I don't think there is a branch for that in quictls/openssl.

@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

I think the logic in CxPlatTlsOnSessionTicketKeyNeeded for OpenSSL3 is what is breaking the resumption failure tests. There is a lot of #ifdef IS_OPENSSL_3 code in there that I'm unsure about. @wfurt if you could take a look that'd be great.

@nibanks nibanks removed the blocked label Feb 2, 2023
@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

I'm going to merge this to unblock testing and usage of openssl3 with MsQuic, but these issues need to be figured out. I'll open up a bug to track.

@nibanks nibanks merged commit 2e2b9c1 into main Feb 2, 2023
@nibanks nibanks deleted the nibanks/test-openssl3 branch February 2, 2023 18:43
@paulidale
Copy link
Contributor

What version of OpenSSL are you basing this from? (quictls/openssl)[https://github.com/quictls/openssl] might need an update?

There have been a number of performance / bug fixes in the 3.0 series of releases & one in particular serialised libctx operations in a multiprocessor environment rather effectively. 3.1 has further improvements.

@nibanks
Copy link
Member Author

nibanks commented Feb 2, 2023

What version of OpenSSL are you basing this from? (quictls/openssl)[https://github.com/quictls/openssl] might need an update?

There have been a number of performance / bug fixes in the 3.0 series of releases & one in particular serialised libctx operations in a multiprocessor environment rather effectively. 3.1 has further improvements.

[submodule "submodules/openssl3"]
	path = submodules/openssl3
	url = https://github.com/quictls/openssl.git
	branch = openssl-3.0.7+quic1

Does 3.0.7 not have all the fixes?

@paulidale
Copy link
Contributor

I think it's got the important ones.

@t8m
Copy link

t8m commented Feb 3, 2023

I think one of the important fixes that reduces the serialization is the libctx refactoring and that was merged only to 3.1 branch.

@wfurt
Copy link
Member

wfurt commented Feb 3, 2023

Do you have pointer @t8m? I think it should be pretty easy to give it try on private build, right @nibanks?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants