AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]

[Daniel Jeliński]

Hi Tony & Sean,
As it turns out, caching the availability of algorithms is sufficient
to get a massive speedup here. Check out the results on
https://github.com/openjdk/jdk/pull/8349 and let me know what you
think.

Regards,
Daniel

śr., 20 kwi 2022 o 22:22 Seán Coffey <sean.coffey at oracle.com> napisał(a):
>
> I think the work done with 8284694 will help alot in any case since I
> suspect the same SSLAlgorithmConstraints Object will be shared much more
> now (rather than spin off new copies)
>
> Some recent JFRs I looked at show that alot of CPU cycles[1] get taken
> in the HandshakeContext methods of :
> sun.security.ssl.HandshakeContext#getActiveCipherSuites
> sun.security.ssl.HandshakeContext#getActiveProtocols
>
> Both methods get called per Handshakecontext construction and I think
> each TLS handshake gets a new Handshakecontext.I was thinking that a
> cache of the last known variables used to deduce the
> getActiveCipherSuites and getActiveProtocols Lists could be created.
> That might have the potential to avoid alot of needless CPU cycles in
> this area since the parameters used to produce these Lists don't really
> change that often. I'm still looking into this potential and hope to
> share a patch shortly.
>
> regards,
> Sean.
>
> [1]
>
> Stack Trace    Count    Percentage
> TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035
> 100 %
> TreeMap$Entry java.util.TreeMap.getEntry(Object)    1035    100 %
> boolean java.util.TreeMap.containsKey(Object)    1035    100 %
> boolean java.util.TreeSet.contains(Object)    1035    100 %
> boolean
> sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set,
> String, AlgorithmDecomposer)    1035    100 %
> boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set,
> String, AlgorithmParameters)    1035    100 %
> boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
> AlgorithmParameters)    1035    100 %
> boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
> AlgorithmParameters)    553    53.4 %
> boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite,
> AlgorithmConstraints, Map)    309    29.9 %
> List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List,
> AlgorithmConstraints)    302    29.2 %
> void sun.security.ssl.HandshakeContext.<init>(SSLContextImpl,
> TransportContext)    302    29.2 %
> void sun.security.ssl.ClientHandshakeContext.<init>(SSLContextImpl,
> TransportContext)    302    29.2 %
> void sun.security.ssl.TransportContext.kickstart()    302 29.2 %
> void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 302    29.2 %
>
>
> On 13/04/2022 18:05, Anthony Scarpino wrote:
> > Hi Sean,
> >
> > Caching is an interesting idea.  I've wondered for a while off and on
> > about how to speed it up, but hadn't come up with a solution I liked.
> > The complication with caching is while something like an algorithm
> > name only could be easy in a hashmap, it gets more complicated when
> > you get into key sizes. Such as, how to represent RSA 1k being
> > disallowed and but 2k allowed.. or certificate usage..
> >
> > Tony
> >
> > On 4/13/22 2:03 AM, Sean Coffey wrote:
> >> On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński
> >> <djelinski at openjdk.org> wrote:
> >>
> >>> During TLS handshake, hundreds of constraints are evaluated to
> >>> determine which cipher suites are usable. Most of the evaluations
> >>> are performed using `HandshakeContext#algorithmConstraints` object.
> >>> By default that object contains a `SSLAlgorithmConstraints` instance
> >>> wrapping another `SSLAlgorithmConstraints` instance. As a result the
> >>> constraints defined in `SSLAlgorithmConstraints` are evaluated twice.
> >>>
> >>> This PR improves the default case; if the user-specified constraints
> >>> are left at defaults, we use a single `SSLAlgorithmConstraints`
> >>> instance, and avoid duplicate checks.
> >>
> >> Nice work. I've been looking at this area myself in recent weeks also
> >> while debugging some JDK 11u performance issues. JFR shows this area
> >> as costly. Some other JDK fixes in this area have already improved
> >> performance. This will certainly help. Pasting a stacktrace[1] from
> >> an old Oracle JDK 11.0.12 report for history purposes.
> >>
> >> I think there are other improvements that can be made in the TLS
> >> constraints code. Caching the last known values from a constraints
> >> permits test is something I've begun looking into.
> >>
> >> [1]
> >> Stack Trace    Count    Percentage
> >> boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[],
> >> int, int)    1637    100 %
> >> boolean java.lang.String.regionMatches(boolean, int, String, int,
> >> int)    1637    100 %
> >> boolean java.lang.String.equalsIgnoreCase(String)    1637 100 %
> >> boolean
> >> sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List,
> >> String, AlgorithmDecomposer)    1631    99.6 %
> >> boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set,
> >> String, AlgorithmParameters)    1631    99.6 %
> >> boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
> >> AlgorithmParameters)    1631    99.6 %
> >> boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
> >> AlgorithmParameters)    836    51.1 %
> >> boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite,
> >> AlgorithmConstraints, Map)    428    26.1 %
> >> List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List,
> >> List, AlgorithmConstraints)    418    25.5 %
> >> void sun.security.ssl.HandshakeContext.<init>(SSLContextImpl,
> >> TransportContext)    418    25.5 %
> >> void sun.security.ssl.ClientHandshakeContext.<init>(SSLContextImpl,
> >> TransportContext)    418    25.5 %
> >> void sun.security.ssl.TransportContext.kickstart()    418 25.5 %
> >> void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 418
> >> 25.5 %
> >> void sun.security.ssl.SSLSocketImpl.startHandshake()    418 25.5 %
> >>
> >> -------------
> >>
> >> Marked as reviewed by coffeys (Reviewer).
> >>
> >> PR: https://git.openjdk.java.net/jdk/pull/8199