Re: RFR: 8258186: Replace use of JNI_COMMIT mode with mode 0
On Wed, 23 Dec 2020 02:13:38 GMT, Valerie Peng wrote: > Could someone please help review this trivial change - just replace > JNI_COMMIT with 0 for all ReleasePrimitiveArrayCritical calls. > > Thanks, > Valerie It looks a safe update to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1875
RFR: 8258186: Replace use of JNI_COMMIT mode with mode 0
Could someone please help review this trivial change - just replace JNI_COMMIT with 0 for all ReleasePrimitiveArrayCritical calls. Thanks, Valerie - Commit messages: - 8258186: Replace use of JNI_COMMIT mode with mode 0 Changes: https://git.openjdk.java.net/jdk/pull/1875/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1875=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258186 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/1875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1875/head:pull/1875 PR: https://git.openjdk.java.net/jdk/pull/1875
RFR: 8258852: Arrays.asList() for single item could be replaced with Collections.singletonList()
If there is only one item, the call to Arrays.asList() could be replaced with Collections.singletonList() for less memory occupation. This update also includes some other code cleanup, like redundant variables in the related files. Code cleanup only, no new regression test. Bug: https://bugs.openjdk.java.net/browse/JDK-8258852 - Commit messages: - 8258852: Arrays.asList() for single item could be replaced with Collections.singletonList() Changes: https://git.openjdk.java.net/jdk/pull/1872/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1872=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258852 Stats: 16 lines in 3 files changed: 0 ins; 5 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/1872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1872/head:pull/1872 PR: https://git.openjdk.java.net/jdk/pull/1872
Integrated: 8258828: The method local variable is not really used
On Tue, 22 Dec 2020 17:16:15 GMT, Xue-Lei Andrew Fan wrote: > The local variable "knownSignatureSchemes" in the > CRSignatureSchemesConsumer.consume() method is assigned, but it is not really > queried. It is safe to remove the local variable and the related code. > > Code cleanup, no new regression test. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258828 This pull request has now been integrated. Changeset: 47c9b437 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/47c9b437 Stats: 11 lines in 1 file changed: 2 ins; 8 del; 1 mod 8258828: The method local variable is not really used Reviewed-by: jnimeh, wetmore - PR: https://git.openjdk.java.net/jdk/pull/1867
Re: RFR: 8258828: The method local variable is not really used
On Tue, 22 Dec 2020 18:11:23 GMT, Bradford Wetmore wrote: > Looks ok as long as you don't need this in chc.peerRequestedSignatureSchemes. No, the chc.peerRequestedSignatureSchemes will be set in the update consumer later on. - PR: https://git.openjdk.java.net/jdk/pull/1867
Integrated: 8258804: Collection.toArray() should use empty array
On Mon, 21 Dec 2020 23:58:03 GMT, Xue-Lei Andrew Fan wrote: > Comparing to Collection.toArray(new T[size)), the Collection.toArray(new > T[0]) seems faster, safer and contractually cleaner. In the update, the use > of Collection.toArray(new T[size)) in the SunJSSE provider implementation is > replaced with Collection.toArray(new T[0]). > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258804 This pull request has now been integrated. Changeset: 39e03a0b Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/39e03a0b Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod 8258804: Collection.toArray() should use empty array Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/1861
Re: RFR: 8258804: Collection.toArray() should use empty array
On Tue, 22 Dec 2020 12:39:23 GMT, David M. Lloyd wrote: >> Comparing to Collection.toArray(new T[size)), the Collection.toArray(new >> T[0]) seems faster, safer and contractually cleaner. In the update, the use >> of Collection.toArray(new T[size)) in the SunJSSE provider implementation is >> replaced with Collection.toArray(new T[0]). >> >> Bug: https://bugs.openjdk.java.net/browse/JDK-8258804 > > src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 854: > >> 852: // Use the customized TLS protocols. >> 853: candidates = >> 854: refactored.toArray(new ProtocolVersion[0]); > > If this level of performance improvement is justified, then would it not be > better to use a constant empty array instead of constructing a new one each > time? I think empty array construction could be optimized in the compiler layer. - PR: https://git.openjdk.java.net/jdk/pull/1861
Re: RFR: 8258828: The method local variable is not really used
On Tue, 22 Dec 2020 17:16:15 GMT, Xue-Lei Andrew Fan wrote: > The local variable "knownSignatureSchemes" in the > CRSignatureSchemesConsumer.consume() method is assigned, but it is not really > queried. It is safe to remove the local variable and the related code. > > Code cleanup, no new regression test. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258828 Looks ok as long as you don't need this in chc.peerRequestedSignatureSchemes. - Marked as reviewed by wetmore (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1867
Re: RFR: 8258804: Collection.toArray() should use empty array
On Mon, 21 Dec 2020 23:58:03 GMT, Xue-Lei Andrew Fan wrote: > Comparing to Collection.toArray(new T[size)), he Collection.toArray(new T[0]) > seems faster, safer and contractually cleaner. In the update, the use of > Collection.toArray(new T[size)) in the SunJSSE provider implementation is > replaced with Collection.toArray(new T[0]). > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258804 src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 854: > 852: // Use the customized TLS protocols. > 853: candidates = > 854: refactored.toArray(new ProtocolVersion[0]); If this level of performance improvement is justified, then would it not be better to use a constant empty array instead of constructing a new one each time? - PR: https://git.openjdk.java.net/jdk/pull/1861
Re: RFR: 8258804: Collection.toArray() should use empty array
On Mon, 21 Dec 2020 23:58:03 GMT, Xue-Lei Andrew Fan wrote: > Comparing to Collection.toArray(new T[size)), he Collection.toArray(new T[0]) > seems faster, safer and contractually cleaner. In the update, the use of > Collection.toArray(new T[size)) in the SunJSSE provider implementation is > replaced with Collection.toArray(new T[0]). > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258804 Looks fine to me. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1861
Re: RFR: 8258828: The method local variable is not really used
On Tue, 22 Dec 2020 17:16:15 GMT, Xue-Lei Andrew Fan wrote: > The local variable "knownSignatureSchemes" in the > CRSignatureSchemesConsumer.consume() method is assigned, but it is not really > queried. It is safe to remove the local variable and the related code. > > Code cleanup, no new regression test. > > Bug: https://bugs.openjdk.java.net/browse/JDK-8258828 Looks fine. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1867
RFR: 8258828: The method local variable is not really used
The local variable "knownSignatureSchemes" in the CRSignatureSchemesConsumer.consume() method is assigned, but it is not really queried. It is safe to remove the local variable and the related code. Code cleanup, no new regression test. Bug: https://bugs.openjdk.java.net/browse/JDK-8258828 - Commit messages: - 8258828: The method local variable is not really used Changes: https://git.openjdk.java.net/jdk/pull/1867/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1867=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258828 Stats: 11 lines in 1 file changed: 2 ins; 8 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1867.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1867/head:pull/1867 PR: https://git.openjdk.java.net/jdk/pull/1867
Integrated: 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject()
On Tue, 22 Dec 2020 15:47:05 GMT, Weijun Wang wrote: > The method is useless now since the related TLS cipher suite was removed long > ago. This pull request has now been integrated. Changeset: 9e463d1a Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/9e463d1a Stats: 25 lines in 1 file changed: 0 ins; 24 del; 1 mod 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject() Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/1865
Re: RFR: 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject()
On Tue, 22 Dec 2020 16:36:14 GMT, Xue-Lei Andrew Fan wrote: >> The method is useless now since the related TLS cipher suite was removed >> long ago. > > Marked as reviewed by xuelei (Reviewer). Looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/1865
Re: RFR: 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject()
On Tue, 22 Dec 2020 15:47:05 GMT, Weijun Wang wrote: > The method is useless now since the related TLS cipher suite was removed long > ago. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1865
Re: RFR: 8253368: TLS connection always receives close_notify exception
On Tue, 22 Dec 2020 15:40:12 GMT, Sean Coffey wrote: > @XueleiFan I went ahead with your advice and chose to keep the check in the > code. Since JDK 11, this code path would have thrown an SSLException. I've > chosen to keep that instead of introduce another > Exception(UnsupportedOperationException) -- which would be new for some > application stacks. I've looked at the apache core-http stack and know that > it catches and ignores SSLException in these cases. > > The important change here is that the exception is not fatal and the session > remains valid. Socket is closed out in a finally block. test case updated > also. OK. Thanks for the update. - PR: https://git.openjdk.java.net/jdk/pull/1205
Re: RFR: 8253368: TLS connection always receives close_notify exception [v2]
On Tue, 22 Dec 2020 15:40:22 GMT, Sean Coffey wrote: >> removing the "closing inbound before receiving peer's close_notify" >> exception that can be seen with TLS stack if calling close on inbound. After >> reading the relevant parts of the TLS v1.2/v1.3 RFCs, I believe the local >> end point doesn't have to wait for close_notify alert from remote end. > > Sean Coffey has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - version 2 redesign > - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253368 > - 8253368: TLS connection always receives close_notify exception Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1205
Re: RFR: 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject()
On Tue, 22 Dec 2020 15:47:05 GMT, Weijun Wang wrote: > The method is useless now since the related TLS cipher suite was removed long > ago. > /label remove core-libs Oops, https://github.com/openjdk/skara/pull/980 filed. - PR: https://git.openjdk.java.net/jdk/pull/1865
RFR: 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject()
The method is useless now since the related TLS cipher suite was removed long ago. - Commit messages: - 8258631: Remove sun.security.jgss.krb5.Krb5Util.getSubject() Changes: https://git.openjdk.java.net/jdk/pull/1865/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1865=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258631 Stats: 25 lines in 1 file changed: 0 ins; 24 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1865.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1865/head:pull/1865 PR: https://git.openjdk.java.net/jdk/pull/1865
Re: RFR: 8253368: TLS connection always receives close_notify exception [v2]
> removing the "closing inbound before receiving peer's close_notify" exception > that can be seen with TLS stack if calling close on inbound. After reading > the relevant parts of the TLS v1.2/v1.3 RFCs, I believe the local end point > doesn't have to wait for close_notify alert from remote end. Sean Coffey has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - version 2 redesign - Merge branch 'master' of https://github.com/openjdk/jdk into JDK-8253368 - 8253368: TLS connection always receives close_notify exception - Changes: - all: https://git.openjdk.java.net/jdk/pull/1205/files - new: https://git.openjdk.java.net/jdk/pull/1205/files/85d41030..86d9dac5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1205=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1205=00-01 Stats: 284235 lines in 2912 files changed: 188350 ins; 66757 del; 29128 mod Patch: https://git.openjdk.java.net/jdk/pull/1205.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1205/head:pull/1205 PR: https://git.openjdk.java.net/jdk/pull/1205
Re: RFR: 8253368: TLS connection always receives close_notify exception
On Fri, 13 Nov 2020 14:16:35 GMT, Sean Coffey wrote: > removing the "closing inbound before receiving peer's close_notify" exception > that can be seen with TLS stack if calling close on inbound. After reading > the relevant parts of the TLS v1.2/v1.3 RFCs, I believe the local end point > doesn't have to wait for close_notify alert from remote end. @XueleiFan I went ahead with your advice and chose to keep the check in the code. Since JDK 11, this code path would have thrown an SSLException. I've chosen to keep that instead of introduce another Exception(UnsupportedOperationException) -- which would be new for some application stacks. I've looked at the apache core-http stack and know that it catches and ignores SSLException in these cases. The important change here is that the exception is not fatal and the session remains valid. Socket is closed out in a finally block. test case updated also. - PR: https://git.openjdk.java.net/jdk/pull/1205
