Avoid fetching OpenSSL digests and ciphers #2588

paulidale · 2022-03-28T02:15:16Z

The old cipher returning calls like EVP_aes_128_gcm() perform late binding which means they fetch on initialisation. Fetching in OpenSSL 3.0 is a relatively expensive operation. Instead of fetching every time a cipher is required, it is faster to pre-fetch and reuse the same EVP_CIPHER object.

Likewise, HMAC is better prefetched but it has the additional complexity of fetching a digest internally. Instead of just prefetching the EVP_MAC object, it is better to create an EVP_MAC_CTX object and call EVP_MAC_CTX_dup() as required.

According to the profiling I've done, this represents a circa 4% boost in HPS.

ghost · 2022-03-28T02:15:27Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ paulidale sign now
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

nibanks · 2022-03-28T11:08:35Z

/azp run

azure-pipelines · 2022-03-28T11:08:52Z

Azure Pipelines successfully started running 2 pipeline(s).

nibanks

Thanks a lot for making these changes. I few things need a bit of clean up before we can merge this. Also, please verify you've signed the Microsoft CLA. Thanks!

src/platform/crypt_openssl.c

nibanks · 2022-03-29T12:42:45Z

You also need to run ./scripts/update-sidecar.ps1 but only after all event code changes are made.

paulidale · 2022-03-30T00:45:49Z

You also need to run ./scripts/update-sidecar.ps1 but only after all event code changes are made.

That exploded nicely :(

dotnet: /home/pauli/work/msquic/msquic/scripts/update-sidecar.ps1:45
Line |
  45 |  dotnet publish ../submodules/clog/src/clog -o ${ClogDir} -f net6.0
     |  ~~~~~~
     | The term 'dotnet' is not recognized as a name of a cmdlet,
     | function, script file, or executable program. Check the
     | spelling of the name, or if a path was included, verify that
     | the path is correct and try again.

/home/pauli/work/msquic/msquic/build/clog/clog: The term '/home/pauli/work/msquic/msquic/build/clog/clog' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
/home/pauli/work/msquic/msquic/build/clog/clog: The term '/home/pauli/work/msquic/msquic/build/clog/clog' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
/home/pauli/work/msquic/msquic/build/clog/clog: The term '/home/pauli/work/msquic/msquic/build/clog/clog' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
/home/pauli/work/msquic/msquic/build/clog/clog: The term '/home/pauli/work/msquic/msquic/build/clog/clog' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
/home/pauli/work/msquic/msquic/build/clog/clog: The term '/home/pauli/work/msquic/msquic/build/clog/clog' is not recognized as a name of a cmdlet, function, script file, or executable program.
Check the spelling of the name, or if a path was included, verify that the path is correct and try again.

paulidale · 2022-03-30T00:48:51Z

Also, please verify you've signed the Microsoft CLA.

The CLA is underway, I'll need an OMC vote before I can confirm that my employer is agreeable. I've added this to the next meeting's agenda.

thhous-msft · 2022-03-30T00:51:58Z

You need .net 6 installed to run update-sidecar.ps1. https://docs.microsoft.com/en-us/dotnet/core/install/linux

nibanks · 2022-03-30T12:01:58Z

/azp run ci

azure-pipelines · 2022-03-30T12:02:52Z

Azure Pipelines successfully started running 1 pipeline(s).

paulidale · 2022-03-31T08:15:46Z

I've run the update script and pushed.
Still trying to figure out the ChaCha20-Poly1305 issue.

src/platform/crypt_openssl.c

nibanks · 2022-04-01T21:30:31Z

/azp run ci

azure-pipelines · 2022-04-01T21:30:46Z

Azure Pipelines successfully started running 1 pipeline(s).

nibanks · 2022-04-03T23:53:11Z

/azp run ci

azure-pipelines · 2022-04-03T23:53:47Z

Azure Pipelines successfully started running 1 pipeline(s).

src/platform/crypt_openssl.c

nibanks · 2022-04-05T11:20:10Z

/azp run ci

azure-pipelines · 2022-04-05T11:20:23Z

Azure Pipelines successfully started running 1 pipeline(s).

src/platform/crypt_openssl.c

paulidale · 2022-04-05T23:30:16Z

The TLS changes give me better than 2x improvement to handshakes/second. There is another one coming that will (hopefully) be better.

nibanks · 2022-05-07T19:09:42Z

@paulidale any updates? Is there some particular hold up on getting approval to sign?

paulidale · 2022-05-08T06:50:14Z

The committee wants some input from our lawyers and this is but no means the highest priority. It will happen, the question at the moment is when.

nibanks · 2022-05-24T13:29:24Z

The committee wants some input from our lawyers and this is but no means the highest priority. It will happen, the question at the moment is when.

Just checking in. It's been another two weeks. Any updates or ETA?

paulidale · 2022-05-25T00:02:04Z

Sadly, nope 😢

nibanks · 2022-06-28T16:08:40Z

It's been another month @paulidale. We'd really appreciate if you could poke the folks you're blocked on to get resolution here. Thanks.

paulidale · 2022-06-29T04:13:51Z

Poking won't help. The person who deals with the legals has more important concerns which aren't going away anytime soon. We are working on getting an alternate.

I do apologise for the delay.

nibanks · 2022-07-29T13:26:20Z

It's been another month @paulidale. Do you have any idea how long until you can get approval? Would you please poke the necessary people? Thanks!

paulidale · 2022-08-15T02:49:45Z

Sorry, I've been on vacation. I'll prod this week.

nibanks · 2022-09-30T21:02:49Z

@paulidale another ping. Without this change, we'll never be able to move to openssl 3.0

paulidale · 2022-10-03T04:10:02Z

Our admin person is looking into this now. The CLA link up top is broken.

Edit: found it.

nibanks · 2022-10-03T11:32:50Z

Thanks for the update @paulidale. I think if you push a new commit, the link would probably refresh. BTW, you have a merge conflict with the latest main which needs to be fixed anyways.

The old cipher returning calls like EVP_aes_128_gcm() perform late binding which means they fetch on init. Fetch in OpenSSL 3.0 is a relatively expensive operation. Instead of fetching every time a cipher is required, it is faster to pre-fetch and reuse the same EVP_CIPHER object. Likewise, HMAC is better prefetched but it has the additional complexity of fetching a digest internally. Instead of just prefetching the EVP_MAC object, it is better to create an EVP_MAC_CTX object and call EVP_MAC_CTX_dup() as required. According to the profiling I've done, this represents a circa 4% boost in HPS.

nibanks · 2022-10-04T11:27:56Z

/azp run

azure-pipelines · 2022-10-04T11:28:22Z

Azure Pipelines successfully started running 2 pipeline(s).

paulidale · 2022-10-05T09:11:15Z

I've finally got permission to sign the CLA on behalf of the project.
The links above are still broken and it's not clear how to sign from the CLA page.

nibanks · 2022-10-05T10:43:34Z

Weird. Perhaps try to close and reopen the PR? Otherwise might have to recreate the PR. Or just create a different temp PR to get a new link.

paulidale · 2022-10-05T11:15:46Z

@microsoft-github-policy-service agree company="OpenSSL Software Services"

paulidale · 2022-10-05T11:16:45Z

Got it. The entire process has changed and the CLA page hasn't.

nibanks · 2022-10-05T11:29:57Z

Got it. The entire process has changed and the CLA page hasn't.

Great. Would you mind merging or rebasing on main to fix some CI issues, please? Then hopefully we can get this merged ASAP today. Thanks again!

nibanks · 2022-10-05T11:30:55Z

Know what, never mind. All the issues are fixed issues. I'm going to merge this. Thanks!

paulidale · 2022-10-06T05:01:17Z

Thanks. I'm on holiday for a few days and wouldn't have got to it until next week.

BTW: there are other performance fixes in OpenSSL's master branch that would be very worthwhile here.

paulidale requested a review from a team as a code owner March 28, 2022 02:15

paulidale force-pushed the nofetch-md-cipher branch from e8bde23 to 93d0706 Compare March 28, 2022 02:37

nibanks added TLS: OpenSSL external Proposed by non-MSFT Area: Performance labels Mar 28, 2022

nibanks previously requested changes Mar 29, 2022

View reviewed changes

paulidale force-pushed the nofetch-md-cipher branch from a452b12 to fce2c31 Compare March 30, 2022 00:35

paulidale closed this Mar 30, 2022

paulidale reopened this Mar 30, 2022

anrossi reviewed Apr 1, 2022

View reviewed changes

src/platform/crypt_openssl.c Show resolved Hide resolved

anrossi reviewed Apr 1, 2022

View reviewed changes

src/platform/crypt_openssl.c Outdated Show resolved Hide resolved

nibanks reviewed Apr 3, 2022

View reviewed changes

src/platform/crypt_openssl.c Outdated Show resolved Hide resolved

nibanks reviewed Apr 5, 2022

View reviewed changes

src/platform/crypt_openssl.c Outdated Show resolved Hide resolved

paulidale mentioned this pull request May 4, 2022

3.0 Performance problems openssl/openssl#17627

Open

nibanks added the blocked label May 26, 2022

paulidale force-pushed the nofetch-md-cipher branch from 974a3e3 to 84ed874 Compare October 4, 2022 03:13

paulidale added 3 commits October 4, 2022 14:17

autogenerated files

7f12c1b

Prefetch TLS required cipher and avoid setting digest on TLS HMAC.

59f2a7a

paulidale force-pushed the nofetch-md-cipher branch from 84ed874 to 59f2a7a Compare October 4, 2022 03:17

nibanks merged commit af9e407 into microsoft:main Oct 5, 2022

paulidale deleted the nofetch-md-cipher branch October 6, 2022 05:01

paulidale mentioned this pull request Feb 20, 2023

3.0 performance degraded due to locking openssl/openssl#20286

Open

Avoid fetching OpenSSL digests and ciphers #2588

Avoid fetching OpenSSL digests and ciphers #2588

Conversation

paulidale commented Mar 28, 2022

ghost commented Mar 28, 2022

nibanks commented Mar 28, 2022

azure-pipelines bot commented Mar 28, 2022

nibanks left a comment

Choose a reason for hiding this comment

nibanks commented Mar 29, 2022

paulidale commented Mar 30, 2022

paulidale commented Mar 30, 2022

thhous-msft commented Mar 30, 2022

nibanks commented Mar 30, 2022

azure-pipelines bot commented Mar 30, 2022

paulidale commented Mar 31, 2022

nibanks commented Apr 1, 2022

azure-pipelines bot commented Apr 1, 2022

nibanks commented Apr 3, 2022

azure-pipelines bot commented Apr 3, 2022

nibanks commented Apr 5, 2022

azure-pipelines bot commented Apr 5, 2022

paulidale commented Apr 5, 2022

nibanks commented May 7, 2022

paulidale commented May 8, 2022

nibanks commented May 24, 2022

paulidale commented May 25, 2022

nibanks commented Jun 28, 2022

paulidale commented Jun 29, 2022

nibanks commented Jul 29, 2022

paulidale commented Aug 15, 2022

nibanks commented Sep 30, 2022

paulidale commented Oct 3, 2022 • edited Loading

nibanks commented Oct 3, 2022

nibanks commented Oct 4, 2022

azure-pipelines bot commented Oct 4, 2022

paulidale commented Oct 5, 2022

nibanks commented Oct 5, 2022

paulidale commented Oct 5, 2022

paulidale commented Oct 5, 2022

nibanks commented Oct 5, 2022

nibanks commented Oct 5, 2022

paulidale commented Oct 6, 2022

paulidale commented Oct 3, 2022 •

edited

Loading