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 [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=8199=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8199=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
