Re: AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]
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 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 TraceCountPercentage > TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035 > 100 % > TreeMap$Entry java.util.TreeMap.getEntry(Object)1035100 % > boolean java.util.TreeMap.containsKey(Object)1035100 % > boolean java.util.TreeSet.contains(Object)1035100 % > boolean > sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set, > String, AlgorithmDecomposer)1035100 % > boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, > String, AlgorithmParameters)1035100 % > boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, > AlgorithmParameters)1035100 % > boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, > AlgorithmParameters)55353.4 % > boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, > AlgorithmConstraints, Map)30929.9 % > List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, > AlgorithmConstraints)30229.2 % > void sun.security.ssl.HandshakeContext.(SSLContextImpl, > TransportContext)30229.2 % > void sun.security.ssl.ClientHandshakeContext.(SSLContextImpl, > TransportContext)30229.2 % > void sun.security.ssl.TransportContext.kickstart()302 29.2 % > void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 30229.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 > >> 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 TraceCountPercentage > >> boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[], > >> int, int)1637100 % > >> boolean java.lang.String.regionMatches(boolean, int, String, int, > >> int)1637100 % > >> boolean java.lang.String.equalsIgnoreCase(String)1637 100 % > >> boolean > >> sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, > >> String, AlgorithmDecomposer)163199.6 % > >> boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, > >> String, AlgorithmParameters)163199.6 %
Re: AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]
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.(SSLContextImpl, TransportContext) 302 29.2 % void sun.security.ssl.ClientHandshakeContext.(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 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.(SSLContextImpl, TransportContext) 418 25.5 % void sun.se
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v5]
> 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. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Reduce line length - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/2a1f0a1d..1a131d9e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=03-04 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Tue, 19 Apr 2022 17:16:29 GMT, Daniel Jeliński 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. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Replace remaining constructors with factory methods Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 10:28:39 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 94:
>>
>>> 92: AlgorithmConstraints userSpecifiedConstraints,
>>> 93: boolean withDefaultCertPathConstraints) {
>>> 94: if (nullIfDefault(userSpecifiedConstraints) == null) {
>>
>> Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation?
>> The logic of the block is a little bit hard to understand to me.
>
> No I don't; it's for the same reason why I'm using `==` and not `equals`:
> `DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as
> `userSpecifiedConstraints` here.
>
> `DEFAULT` is used because [SSLConfiguration sets
> userSpecifiedAlgorithmConstraints to
> SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129).
> This feels wrong; the name suggests that the constraints should be specified
> by user, and should be null if the user doesn't touch them.
> `userSpecifiedAlgorithmConstraints` are accessible by
> `getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and
> SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would
> be concerned that setting my own algorithm constraints would replace the
> default ones. It doesn't, but that is not immediately apparent.
>
> We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out
> all the other changes from this PR. The only reason why I didn't do that was
> because it would change the observable behavior
> (`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you
> think we can live with that, I'll be happy to do that change.
It is not interested to me to use 'null' constraints in ssl configure. I have
no more comments. Thank you for the update!
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 04:19:38 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Replace remaining constructors with factory methods
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 94:
>
>> 92: AlgorithmConstraints userSpecifiedConstraints,
>> 93: boolean withDefaultCertPathConstraints) {
>> 94: if (nullIfDefault(userSpecifiedConstraints) == null) {
>
> Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation?
> The logic of the block is a little bit hard to understand to me.
No I don't; it's for the same reason why I'm using `==` and not `equals`:
`DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as
`userSpecifiedConstraints` here.
`DEFAULT` is used because [SSLConfiguration sets
userSpecifiedAlgorithmConstraints to
SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129).
This feels wrong; the name suggests that the constraints should be specified
by user, and should be null if the user doesn't touch them.
`userSpecifiedAlgorithmConstraints` are accessible by
`getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and
SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would be
concerned that setting my own algorithm constraints would replace the default
ones. It doesn't, but that is not immediately apparent.
We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out
all the other changes from this PR. The only reason why I didn't do that was
because it would change the observable behavior
(`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you
think we can live with that, I'll be happy to do that change.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Tue, 19 Apr 2022 17:16:29 GMT, Daniel Jeliński 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.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Replace remaining constructors with factory methods
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
94:
> 92: AlgorithmConstraints userSpecifiedConstraints,
> 93: boolean withDefaultCertPathConstraints) {
> 94: if (nullIfDefault(userSpecifiedConstraints) == null) {
Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation?
The logic of the block is a little bit hard to understand to me.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
97:
> 95: return withDefaultCertPathConstraints ? DEFAULT :
> DEFAULT_SSL_ONLY;
> 96: }
> 97: return new SSLAlgorithmConstraints(userSpecifiedConstraints,
> withDefaultCertPathConstraints);
It would be nice to limit each line within 80 characters, which is useful for
terminal users.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Tue, 19 Apr 2022 17:21:20 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 94:
>>
>>> 92: * @return a SSLAlgorithmConstraints instance
>>> 93: */
>>> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
>>
>> I like the idea to use a static method for the construction. What do you
>> think if the same coding style is used for other constructors, by making
>> them private and add forSocket() methods accordingly? For example, changing
>> the following constructor to a private method.
>>
>> SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
>> boolean withDefaultCertPathConstraints) {
>
> it won't change the performance in any way - when `supportedAlgorithms` are
> set, we always need to create a new object. But you're right, it's
> inconsistent to offer both constructors and factory methods in the same
> class. Changed.
The variants of supportedAlgorithms could be limited. There might be a
potential improvement. But it is out of the scope of this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Tue, 19 Apr 2022 14:23:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - add more factory methods, update copyright
>> - return DEFAULT also when user constraints are null
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 94:
>
>> 92: * @return a SSLAlgorithmConstraints instance
>> 93: */
>> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
>
> I like the idea to use a static method for the construction. What do you
> think if the same coding style is used for other constructors, by making them
> private and add forSocket() methods accordingly? For example, changing the
> following constructor to a private method.
>
> SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
> boolean withDefaultCertPathConstraints) {
it won't change the performance in any way - when `supportedAlgorithms` are
set, we always need to create a new object. But you're right, it's inconsistent
to offer both constructors and factory methods in the same class. Changed.
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 118:
>
>> 116: if (userSpecifiedConstraints == null) {
>> 117: return withDefaultCertPathConstraints ? DEFAULT :
>> DEFAULT_SSL_ONLY;
>> 118: }
>
> It looks like a partial duplicate of nullIfDefault(). What do you think if
> merging the logic into nullIfDefault()? Or even merging nullIfDefault()
> logic into the getUserSpecifiedConstraints() method? New parameters may be
> required for the getUserSpecifiedConstraints() methods.
I'm not a big fan of modifying `getUserSpecifiedConstraints`; that method has 3
`return` clauses. But you're right, there was some code duplication that is
removed now.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
> 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. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Replace remaining constructors with factory methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/e4cc8152..2a1f0a1d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=02-03 Stats: 58 lines in 4 files changed: 21 ins; 14 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Thu, 14 Apr 2022 21:05:06 GMT, Daniel Jeliński 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.
>
> Daniel Jeliński has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - add more factory methods, update copyright
> - return DEFAULT also when user constraints are null
Thanks for the update. It looks good to me, except comments for the coding
style.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
94:
> 92: * @return a SSLAlgorithmConstraints instance
> 93: */
> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
I like the idea to use a static method for the construction. What do you think
if the same coding style is used for other constructors, by making them private
and add forSocket() methods accordingly? For example, changing the following
constructor to a private method.
SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
boolean withDefaultCertPathConstraints) {
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
118:
> 116: if (userSpecifiedConstraints == null) {
> 117: return withDefaultCertPathConstraints ? DEFAULT :
> DEFAULT_SSL_ONLY;
> 118: }
It looks like a partial duplicate of nullIfDefault(). What do you think if
merging the logic into nullIfDefault()? Or even merging nullIfDefault() logic
into the getUserSpecifiedConstraints() method? New parameters may be required
for the getUserSpecifiedConstraints() methods.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Wed, 13 Apr 2022 06:46:51 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - add more factory methods, update copyright >> - return DEFAULT also when user constraints are null > > Nice catch. Thank you! > hi @XueleiFan I'd appreciate your approval here. Thanks! Sorry, I missed the notification emails. I will have a look today. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Wed, 13 Apr 2022 06:46:51 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - add more factory methods, update copyright >> - return DEFAULT also when user constraints are null > > Nice catch. Thank you! hi @XueleiFan I'd appreciate your approval here. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Thu, 14 Apr 2022 21:05:06 GMT, Daniel Jeliński 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. > > Daniel Jeliński has updated the pull request incrementally with two > additional commits since the last revision: > > - add more factory methods, update copyright > - return DEFAULT also when user constraints are null I think I addressed all the concerns raised. Could you take another look? - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
> 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. Daniel Jeliński has updated the pull request incrementally with two additional commits since the last revision: - add more factory methods, update copyright - return DEFAULT also when user constraints are null - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/b6e46ae9..e4cc8152 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=01-02 Stats: 65 lines in 6 files changed: 33 ins; 2 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 15:53:53 GMT, Xue-Lei Andrew Fan wrote: >> as of today, this method is never called with a `null` argument >> (`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to >> `DEFAULT` and cannot be reset to `null`), but I can add a null check for >> future-proofing. > > I know. But if the null condition is not added, a code reader may have to > search for its usage and make sure null is not passed. If the usages are in > the same class, I may not add the checking. Otherwise, an additional > checking might save time in the future. In such cases `assert xxx != null;` could be used to tell the reader that `null` is not an expected value. But then you need to be absolutely sure that `null` can never reach here when in production. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 15:43:42 GMT, Daniel Jeliński wrote: >>> @XueleiFan did you mean `||` (not `&&`) ? >> >> Thank you @dfuch. Yes, it should be "||". > > as of today, this method is never called with a `null` argument > (`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to > `DEFAULT` and cannot be reset to `null`), but I can add a null check for > future-proofing. I know. But if the null condition is not added, a code reader may have to search for its usage and make sure null is not passed. If the usages are in the same class, I may not add the checking. Otherwise, an additional checking might save time in the future. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 14:58:24 GMT, Xue-Lei Andrew Fan wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 73:
>>
>>> 71:
>>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>>> userSpecifiedConstraints) {
>>> 73: if (userSpecifiedConstraints == DEFAULT) {
>>
>> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>>
>>
>> -if (userSpecifiedConstraints == DEFAULT) {
>> +if (userSpecifiedConstraints == null &&
>> + userSpecifiedConstraints== DEFAULT) {
>
>> @XueleiFan did you mean `||` (not `&&`) ?
>
> Thank you @dfuch. Yes, it should be "||".
as of today, this method is never called with a `null` argument
(`SSLConfiguration#userSpecifiedAlgorithmConstraints` is initialized to
`DEFAULT` and cannot be reset to `null`), but I can add a null check for
future-proofing.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 04:24:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid nesting SSLAlgorithmConstraints
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 73:
>
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>> 73: if (userSpecifiedConstraints == DEFAULT) {
>
> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>
>
> -if (userSpecifiedConstraints == DEFAULT) {
> +if (userSpecifiedConstraints == null &&
> + userSpecifiedConstraints== DEFAULT) {
> @XueleiFan did you mean `||` (not `&&`) ?
Thank you @dfuch. Yes, it should be "||".
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Thu, 14 Apr 2022 04:24:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Avoid nesting SSLAlgorithmConstraints
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 73:
>
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>> 73: if (userSpecifiedConstraints == DEFAULT) {
>
> Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
>
>
> -if (userSpecifiedConstraints == DEFAULT) {
> +if (userSpecifiedConstraints == null &&
> + userSpecifiedConstraints== DEFAULT) {
@XueleiFan did you mean `||` (not `&&`) ?
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Wed, 13 Apr 2022 20:32:02 GMT, Daniel Jeliński 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.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid nesting SSLAlgorithmConstraints
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
73:
> 71:
> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
> userSpecifiedConstraints) {
> 73: if (userSpecifiedConstraints == DEFAULT) {
Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
-if (userSpecifiedConstraints == DEFAULT) {
+if (userSpecifiedConstraints == null &&
+ userSpecifiedConstraints== DEFAULT) {
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Wed, 13 Apr 2022 16:02:50 GMT, Xue-Lei Andrew Fan wrote: >> Thanks @XueleiFan for the review! >> If we do that, this will result in a behavior change for cases where >> `enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set >> `enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints == >> DEFAULT`? > > I think it is OK. The enabledX509DisabledAlgConstraints should be specified > with the withDefaultCertPathConstraints parameterm, and should not be > overrode by the userSpecifiedConstraints. I think it is a behavior that we'd > like to correct. updated the patch. Let me know if that's what you had in mind. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
> 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. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Avoid nesting SSLAlgorithmConstraints - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/9bcd0b66..b6e46ae9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=00-01 Stats: 14 lines in 1 file changed: 9 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]
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 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) 1637100 % boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 % boolean java.lang.String.equalsIgnoreCase(String) 1637100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer)163199.6 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters)163199.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 163199.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.(SSLContextImpl, TransportContext) 418 25.5 % void sun.security.ssl.ClientHandshakeContext.(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
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Wed, 13 Apr 2022 07:50:55 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 72:
>>
>>> 70: }
>>> 71:
>>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>>> userSpecifiedConstraints) {
>>
>> I may update all of the constructors so that the accumulation of the
>> reference of userSpecifiedConstraints could be avoid further.
>>
>>
>> - this.userSpecifiedConstraints = userSpecifiedConstraints;
>> + this.userSpecifiedConstraints = userSpecifiedConstraints == DEFAULT ?
>> + null : userSpecifiedConstraints;
>>
>>
>>
>> Similar update could be placed in the getUserSpecifiedConstraints()
>> implementation.
>
> Thanks @XueleiFan for the review!
> If we do that, this will result in a behavior change for cases where
> `enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set
> `enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints ==
> DEFAULT`?
I think it is OK. The enabledX509DisabledAlgConstraints should be specified
with the withDefaultCertPathConstraints parameterm, and should not be overrode
by the userSpecifiedConstraints. I think it is a behavior that we'd like to
correct.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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) 1637100 % boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 % boolean java.lang.String.equalsIgnoreCase(String) 1637100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer)163199.6 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters)163199.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 163199.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.(SSLContextImpl, TransportContext) 418 25.5 % void sun.security.ssl.ClientHandshakeContext.(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
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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.
test/micro/org/openjdk/bench/java/security/SSLHandshake.java line 54:
> 52: @OutputTimeUnit(TimeUnit.SECONDS)
> 53: @State(Scope.Benchmark)
> 54: public class SSLHandshake {
Default number of iterations and forks is usually excessive, I suggest some
tuning runs to minimize benchmark execution time, e.g., something like:
@Warmup(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 5, time = 1000, timeUnit = TimeUnit.MILLISECONDS)
@Fork(3)
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Wed, 13 Apr 2022 06:45:20 GMT, Xue-Lei Andrew Fan 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.
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 72:
>
>> 70: }
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>
> I may update all of the constructors so that the accumulation of the
> reference of userSpecifiedConstraints could be avoid further.
>
>
> - this.userSpecifiedConstraints = userSpecifiedConstraints;
> + this.userSpecifiedConstraints = userSpecifiedConstraints == DEFAULT ?
> + null : userSpecifiedConstraints;
>
>
>
> Similar update could be placed in the getUserSpecifiedConstraints()
> implementation.
Thanks @XueleiFan for the review!
If we do that, this will result in a behavior change for cases where
`enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set
`enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints ==
DEFAULT`?
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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 catch. Thank you!
src/java.base/share/classes/sun/security/ssl/HandshakeContext.java line 167:
> 165: this.sslConfig = (SSLConfiguration)conContext.sslConfig.clone();
> 166:
> 167: this.algorithmConstraints = SSLAlgorithmConstraints.wrap(
Maybe, the change could be placed in the SSLAlgorithmConstraints constructors
implementation so that it is easier to avoid this mistake.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
72:
> 70: }
> 71:
> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
> userSpecifiedConstraints) {
I may update all of the constructors so that the accumulation of the reference
of userSpecifiedConstraints could be avoid further.
- this.userSpecifiedConstraints = userSpecifiedConstraints;
+ this.userSpecifiedConstraints = userSpecifiedConstraints == DEFAULT ?
+ null : userSpecifiedConstraints;
Similar update could be placed in the getUserSpecifiedConstraints()
implementation.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 15:40:46 GMT, Claes Redestad wrote: >> While this is technically true, `SSLAlgorithmConstraints` is an internal >> class, so it's very unlikely that we will ever get `SSLAlgorithmConstraints` >> other than `DEFAULT` here. > > Right, I see even `DEFAULT_SSL_ONLY` is only used statically in one place. > > So the patch is probably good enough. Out of scope here, but if these > permits-calls are (somewhat) performance-sensitive and the `DEFAULT` object > is likely the only instance of `SSLAlgorithmConstraints` we'll ever see then > perhaps it should be a specialized implementation that avoid the always-null > `userSpecifiedConstraints != null` and `peerSpecifiedConstraints != null` > checks. Thanks for reviewing! These permits calls are generally performing sufficiently well; the problem is that they in turn end up calling [AlgorithmDecomposer#decomposeImpl](https://github.com/openjdk/jdk/blob/36b59f3814fdaa44c9c04a0e8d63d0ba56929326/src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java#L53), which performs string splitting using a [pattern with negative look-behind](https://github.com/openjdk/jdk/blob/36b59f3814fdaa44c9c04a0e8d63d0ba56929326/src/java.base/share/classes/sun/security/util/AlgorithmDecomposer.java#L43), which is pretty slow. But that's another story. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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. Please make sure @XueleiFan has a chance to look at this before integrating. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 15:19:41 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 73:
>>
>>> 71:
>>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>>> userSpecifiedConstraints) {
>>> 73: if (userSpecifiedConstraints == DEFAULT) {
>>
>> Just thinking out loud: It seems all this does when
>> `userSpecifiedConstraints` is a `SSLAlgorithmConstraints` is force the
>> `enableX509..` flag to `true`. So in addition to the obvious thing for
>> `DEFAULT`, you could also return `DEFAULT` for `DEFAULT_SSL_ONLY`. Or more
>> generally: if `userSpecifiedConstraints instanceof SSLAlgorithmConstraints`
>> then you could either return `userSpecifiedConstraints` as-is if
>> `enabledX509DisabledAlgConstraints` is `true` or else return a clone of it
>> with `enabledX509DisabledAlgConstraints` set to `true`.
>
> While this is technically true, `SSLAlgorithmConstraints` is an internal
> class, so it's very unlikely that we will ever get `SSLAlgorithmConstraints`
> other than `DEFAULT` here.
Right, I see even `DEFAULT_SSL_ONLY` is only used statically in one place.
So the patch is probably good enough. Out of scope here, but if these
permits-calls are (somewhat) performance-sensitive and the `DEFAULT` object is
likely the only instance of `SSLAlgorithmConstraints` we'll ever see then
perhaps it should be a specialized implementation that avoid the always-null
`userSpecifiedConstraints != null` and `peerSpecifiedConstraints != null`
checks.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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. Marked as reviewed by redestad (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 13:38:17 GMT, Claes Redestad 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.
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 73:
>
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>> 73: if (userSpecifiedConstraints == DEFAULT) {
>
> Just thinking out loud: It seems all this does when
> `userSpecifiedConstraints` is a `SSLAlgorithmConstraints` is force the
> `enableX509..` flag to `true`. So in addition to the obvious thing for
> `DEFAULT`, you could also return `DEFAULT` for `DEFAULT_SSL_ONLY`. Or more
> generally: if `userSpecifiedConstraints instanceof SSLAlgorithmConstraints`
> then you could either return `userSpecifiedConstraints` as-is if
> `enabledX509DisabledAlgConstraints` is `true` or else return a clone of it
> with `enabledX509DisabledAlgConstraints` set to `true`.
While this is technically true, `SSLAlgorithmConstraints` is an internal class,
so it's very unlikely that we will ever get `SSLAlgorithmConstraints` other
than `DEFAULT` here.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
73:
> 71:
> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
> userSpecifiedConstraints) {
> 73: if (userSpecifiedConstraints == DEFAULT) {
Just thinking out loud: It seems all this does when `userSpecifiedConstraints`
is a `SSLAlgorithmConstraints` is force the `enableX509..` flag to `true`. So
in addition to the obvious thing for `DEFAULT`, you could also return `DEFAULT`
for `DEFAULT_SSL_ONLY`. Or more generally: if `userSpecifiedConstraints
instanceof SSLAlgorithmConstraints` then you could either return
`userSpecifiedConstraints` as-is if `enabledX509DisabledAlgConstraints` is
`true` or else return a clone of it with `enabledX509DisabledAlgConstraints`
set to `true`.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
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. - Commit messages: - Fix file attributes - Fix file attributes - Avoid wrapping default constraints - Add handshake bench Changes: https://git.openjdk.java.net/jdk/pull/8199/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284694 Stats: 337 lines in 4 files changed: 336 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński 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. Results of the attached benchmark: Before: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 1407.320 ± 302.562 ops/s SSLHandshake.doHandshake true TLS thrpt5 391.037 ± 13.014 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5 280.003 ± 69.273 ops/s SSLHandshake.doHandshake false TLS thrpt5 233.401 ± 9.371 ops/s After: Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units SSLHandshake.doHandshake true TLSv1.2 thrpt5 2267.325 ± 119.800 ops/s SSLHandshake.doHandshake true TLS thrpt5 490.465 ± 24.698 ops/s SSLHandshake.doHandshake false TLSv1.2 thrpt5 340.275 ± 72.833 ops/s SSLHandshake.doHandshake false TLS thrpt5 271.656 ± 5.444 ops/s The results show a nice double-digit improvement across the board. - PR: https://git.openjdk.java.net/jdk/pull/8199
