Skip to content

Conversation

@weissi
Copy link
Contributor

Motivation:

At the moment, AHC assumes that creating a NIOSSLContext is both cheap
and doesn't block.

Neither of these two assumptions are true.

To create a NIOSSLContext, BoringSSL will have to read a lot of
certificates in the trust store (on disk) which require a lot of ASN1
parsing and much much more.

On my Ubuntu test machine, creating one NIOSSLContext is about 27,000
allocations!!! To make it worse, AHC allocates a fresh NIOSSLContext
for every single connection, whether HTTP or HTTPS. Yes, correct.

Modification:

  • Cache NIOSSLContexts per TLSConfiguration in a LRU cache
  • Don't get an NIOSSLContext for HTTP (plain text) connections

Result:

New connections should be much faster in general assuming that you're
not using a different TLSConfiguration for every connection.

@weissiweissiforce-pushed the jw-cache-sslcontext branch 2 times, most recently from c6bfa23 to c946b23CompareMay 8, 2021 11:59
@weissi
Copy link
ContributorAuthor

before

49,640 allocations for 100 plain-text HTTP requests over 1 connection

before

after

22,158 allocations for 100 plain-text HTTP requests over 1 connection

after

@weissiweissiforce-pushed the jw-cache-sslcontext branch from c946b23 to 15b2e3bCompareMay 11, 2021 13:43
varport:Int
varunixPath:String
vartlsConfiguration:BestEffortHashableTLSConfiguration?
privatevartlsConfiguration:BestEffortHashableTLSConfiguration?
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

note: make this private because nobody accesses it from the outside and if not private, then there are places where we have both a key.tlsConfiguration and a configuration.tlsConfiguration which aren't necessarily the same (eg. when doing https:// over a http:// proxy).

@weissiweissiforce-pushed the jw-cache-sslcontext branch 3 times, most recently from 65ff26c to e6f5270CompareMay 11, 2021 14:28
@weissiweissi marked this pull request as ready for review May 11, 2021 14:29
@weissiweissiforce-pushed the jw-cache-sslcontext branch 2 times, most recently from e184939 to ae9c9acCompareMay 11, 2021 14:44
@weissiweissiforce-pushed the jw-cache-sslcontext branch from ae9c9ac to 539671dCompareMay 11, 2021 18:49
Comment on lines +294 to +279
varisNIOTS:Bool{
#if canImport(Network)
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0,*){
returnself.underlyingBootstrap is NIOTSConnectionBootstrap
}else{
returnfalse
}
#else
returnfalse
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason, why we don't use this pattern?

Suggested change
varisNIOTS:Bool{
#if canImport(Network)
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0,*){
return self.underlyingBootstrap is NIOTSConnectionBootstrap
}else{
return false
}
#else
return false
#endif
}
varisNIOTS:Bool{
#if canImport(Network)
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0,*){
return self.underlyingBootstrap is NIOTSConnectionBootstrap
}
#endif
return false
}

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I reduced the amount of code we need here but happy to add another } else{return false }

port: key.port,
requiresTLS: requiresTLS,
configuration: configuration,
sslContextCache: sslContextCache,
Copy link
Member

@fabianfettfabianfettMay 12, 2021

Choose a reason for hiding this comment

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

Why do we pass the SSLContextCache down here? We already have a sslContext (a Future) created before. IMHO it would make more sense to pass that down.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That is an excellent question, I have no idea. I'll look into this, maybe we can just inject the SSLContext into that method.

Comment on lines +215 to +102
sslContext.map{ sslContext ->(Channel,NIOSSLContext?)in
(channel, sslContext)
}
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Because we now need the actual context. We're kicking off the connection creation and an SSLContext creation in parallel and this is the point where we need both.

Comment on lines 101 to 103
bootstrap = sslContextCache.sslContext(tlsConfiguration: configuration.tlsConfiguration ??.forClient(),
eventLoop: eventLoop,
logger: logger)
Copy link
Member

Choose a reason for hiding this comment

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

This is hit with in a test that doesn't require https: testUploadStreamingBackpressure

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

great catch! Thanks you

@weissiweissiforce-pushed the jw-cache-sslcontext branch from 539671d to e9c243bCompareMay 12, 2021 11:45
@weissiweissiforce-pushed the jw-cache-sslcontext branch 2 times, most recently from 2d23f5a to 064060eCompareMay 12, 2021 13:18
@weissiweissiforce-pushed the jw-cache-sslcontext branch 2 times, most recently from 5fa35bd to a7a30a8CompareMay 12, 2021 13:36
@weissiweissi requested review from Lukasa and fabianfettMay 12, 2021 13:40
@LukasaLukasa added the 🔨 semver/patch No public API change. label May 12, 2021
Copy link
Collaborator

@LukasaLukasa left a comment

Choose a reason for hiding this comment

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

Cool, this LGTM.

@weissiweissiforce-pushed the jw-cache-sslcontext branch from a7a30a8 to 89336a0CompareMay 12, 2021 17:49
Copy link
Member

@fabianfettfabianfett left a comment

Choose a reason for hiding this comment

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

LGTM. 🎉

@weissiweissiforce-pushed the jw-cache-sslcontext branch from 89336a0 to b2092fcCompareMay 13, 2021 11:04
Motivation: At the moment, AHC assumes that creating a `NIOSSLContext` is both cheap and doesn't block. Neither of these two assumptions are true. To create a `NIOSSLContext`, BoringSSL will have to read a lot of certificates in the trust store (on disk) which require a lot of ASN1 parsing and much much more. On my Ubuntu test machine, creating one `NIOSSLContext` is about 27,000 allocations!!! To make it worse, AHC allocates a fresh `NIOSSLContext` for _every single connection_, whether HTTP or HTTPS. Yes, correct. Modification: - Cache NIOSSLContexts per TLSConfiguration in a LRU cache - Don't get an NIOSSLContext for HTTP (plain text) connections Result: New connections should be _much_ faster in general assuming that you're not using a different TLSConfiguration for every connection.
@weissiweissiforce-pushed the jw-cache-sslcontext branch from b2092fc to 97b4896CompareMay 13, 2021 11:05
@weissiweissi merged commit e2d03ff into swift-server:mainMay 13, 2021
@weissiweissi deleted the jw-cache-sslcontext branch May 13, 2021 11:16
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patchNo public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@weissi@artemredkin@Lukasa@fabianfett