Re: RFR: 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl [v4]
> Redo for 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears > to not be fully fixed > > This also fixes JDK-8259516: Alerts sent by peer may not be received > correctly during TLS handshake Clive Verghese has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Add error handling guidelines - Fix bugids and use server port passed as a parameter - 8259662: Don't wrap SocketExceptions into SSLExceptions in SSLSocketImpl - Changes: https://git.openjdk.java.net/jdk/pull/2057/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=03 Stats: 499 lines in 7 files changed: 490 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/2057.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057 PR: https://git.openjdk.java.net/jdk/pull/2057
Re: RFR: 8259662: SocketException should be passed through [v3]
> Redo for 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears > to not be fully fixed > > This also fixes JDK-8259516: Alerts sent by peer may not be received > correctly during TLS handshake Clive Verghese has updated the pull request incrementally with one additional commit since the last revision: Add error handling guidelines - Changes: - all: https://git.openjdk.java.net/jdk/pull/2057/files - new: https://git.openjdk.java.net/jdk/pull/2057/files/da98249d..4e715ba9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2057&range=01-02 Stats: 10 lines in 1 file changed: 10 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/2057.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2057/head:pull/2057 PR: https://git.openjdk.java.net/jdk/pull/2057
Integrated: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL
On Wed, 20 Jan 2021 15:25:18 GMT, Eirik Bjorsnos wrote: > By moving string splitting and concatenation into the canonizeString utility, > we can defer allocation until we determine that canonization is required. > This saves two string allocations and a string concat for the common case > where canonization is not required. > > As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler since > that's the only call site. > > Finally, let's rename the method to canonizalizeString, since canonization is > an rather unrelated term. This pull request has now been integrated. Changeset: e1de0bf8 Author:Eirik Bjorsnos Committer: Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/e1de0bf8 Stats: 98 lines in 2 files changed: 47 ins; 47 del; 4 mod 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL Reviewed-by: redestad, chegar - PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v4]
On Thu, 21 Jan 2021 10:11:11 GMT, Eirik Bjorsnos wrote: >> By moving string splitting and concatenation into the canonizeString >> utility, we can defer allocation until we determine that canonization is >> required. This saves two string allocations and a string concat for the >> common case where canonization is not required. >> >> As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler >> since that's the only call site. >> >> Finally, let's rename the method to canonizalizeString, since canonization >> is an rather unrelated term. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Update copyright year in affected files since the change is non-trivial Looks good. (You need to cast /integrate again for us to be able to /sponsor) - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v4]
> By moving string splitting and concatenation into the canonizeString utility, > we can defer allocation until we determine that canonization is required. > This saves two string allocations and a string concat for the common case > where canonization is not required. > > As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler since > that's the only call site. > > Finally, let's rename the method to canonizalizeString, since canonization is > an rather unrelated term. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Update copyright year in affected files since the change is non-trivial - Changes: - all: https://git.openjdk.java.net/jdk/pull/2167/files - new: https://git.openjdk.java.net/jdk/pull/2167/files/0be597b1..7bde5f15 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2167&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2167&range=02-03 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2167.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2167/head:pull/2167 PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v2]
On Thu, 21 Jan 2021 09:48:50 GMT, Chris Hegarty wrote: >> Eirik Bjorsnos has updated the pull request incrementally with one >> additional commit since the last revision: >> >> As a part of the rename from "canonize" to "canonicalize", doCanonize >> should also be renamed. This was an oversight in the original PR. > > Looks good to me. > Newbie question: Is a copyright year updates required when code is simply > moved around like this? > > More specifically, is it required when strictly deleting code from a file, > like ParseUtil in this case? I would prefer if the copyright year range was updated in these files - this is a non-trivial change. - PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v3]
> By moving string splitting and concatenation into the canonizeString utility, > we can defer allocation until we determine that canonization is required. > This saves two string allocations and a string concat for the common case > where canonization is not required. > > As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler since > that's the only call site. > > Finally, let's rename the method to canonizalizeString, since canonization is > an rather unrelated term. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Replace "canoize" with "canonicalize" in a code comment which left out in the code renaming - Changes: - all: https://git.openjdk.java.net/jdk/pull/2167/files - new: https://git.openjdk.java.net/jdk/pull/2167/files/f4b543d6..0be597b1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2167&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2167&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2167.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2167/head:pull/2167 PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v2]
On Wed, 20 Jan 2021 15:46:13 GMT, Eirik Bjorsnos wrote: >> By moving string splitting and concatenation into the canonizeString >> utility, we can defer allocation until we determine that canonization is >> required. This saves two string allocations and a string concat for the >> common case where canonization is not required. >> >> As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler >> since that's the only call site. >> >> Finally, let's rename the method to canonizalizeString, since canonization >> is an rather unrelated term. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > As a part of the rename from "canonize" to "canonicalize", doCanonize > should also be renamed. This was an oversight in the original PR. src/java.base/share/classes/sun/net/www/protocol/jar/Handler.java line 166: > 164: file = parseContextSpec(url, spec); > 165: > 166: // Canonize the result after the bangslash Maybe we can also replace "Canonize" in the comment while we are in the area. - PR: https://git.openjdk.java.net/jdk/pull/2167
Re: RFR: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL [v2]
On Wed, 20 Jan 2021 15:46:13 GMT, Eirik Bjørsnøs wrote: >> By moving string splitting and concatenation into the canonizeString >> utility, we can defer allocation until we determine that canonization is >> required. This saves two string allocations and a string concat for the >> common case where canonization is not required. >> >> As a refactoring, move ParseUtil.canonizeString/doCanonize into Handler >> since that's the only call site. >> >> Finally, let's rename the method to canonizalizeString, since canonization >> is an rather unrelated term. > > Eirik Bjørsnøs has updated the pull request incrementally with one additional > commit since the last revision: > > As a part of the rename from "canonize" to "canonicalize", doCanonize > should also be renamed. This was an oversight in the original PR. Looks good to me. - Marked as reviewed by chegar (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2167
