Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 07:02:55 GMT, Сергей Цыпанов wrote:
> - use `String.equalsIgnoreCase()` instead of hand-written code relying on
> `String.charAt()`
> - use `String.compareToIgnoreCase()` instead of hand-written code relying on
> `String.charAt()`
> - drop branches that are never executed
> - drop unused argument from `URI.resolvePath()`
> - simplify String-related operations
Note that if the current locale is Turkish, then `"I".equalsIgnoreCase("i")`
(`"\u0049".equalsIgnoreCase("\u0069")`) returns `false`.
-
PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]
On Thu, 19 May 2022 09:38:40 GMT, Kevin Walls wrote: >> Alexey Ivanov has updated the pull request incrementally with seven >> additional commits since the last revision: >> >> - ...set to the values... >> - ...will result in a Zip64 Extra (EXT) header >> - ...in addition to the main attributes... >> - ...and the address of the current archive >> - Merges the stack at the given bci... >> - Returns a single ... >> - ...when a peer shuts down an association. > > OK. I started with serviceability but then went through everything as it's > hard to record what you have/haven't seen in these long reviews. Thank you @kevinjwalls for your suggestions. I've incorporated them. - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]
On Thu, 19 May 2022 08:47:47 GMT, Kevin Walls wrote:
>> Alexey Ivanov has updated the pull request incrementally with seven
>> additional commits since the last revision:
>>
>> - ...set to the values...
>> - ...will result in a Zip64 Extra (EXT) header
>> - ...in addition to the main attributes...
>> - ...and the address of the current archive
>> - Merges the stack at the given bci...
>> - Returns a single ...
>> - ...when a peer shuts down an association.
>
> src/jdk.jdi/share/classes/com/sun/jdi/ClassType.java line 348:
>
>> 346:
>> 347: /**
>> 348: * Returns the single non-abstract {@link Method} visible from
>
> I would think "Returns a single ..." because the implementation returns the
> first match it finds, from possibly many.
I've accepted it, yet I'm still unsure whether ‘a‘ or ‘the’…
-
PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base [v2]
> Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. Alexey Ivanov has updated the pull request incrementally with seven additional commits since the last revision: - ...set to the values... - ...will result in a Zip64 Extra (EXT) header - ...in addition to the main attributes... - ...and the address of the current archive - Merges the stack at the given bci... - Returns a single ... - ...when a peer shuts down an association. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8768/files - new: https://git.openjdk.java.net/jdk/pull/8768/files/c7e73658..0669cdc1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8768&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8768&range=00-01 Stats: 7 lines in 7 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8768.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8768/head:pull/8768 PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8285521: Minor improvements in java.net.URI
On Thu, 19 May 2022 12:19:25 GMT, ExE Boss wrote: >> - use `String.equalsIgnoreCase()` instead of hand-written code relying on >> `String.charAt()` >> - use `String.compareToIgnoreCase()` instead of hand-written code relying on >> `String.charAt()` >> - drop branches that are never executed >> - drop unused argument from `URI.resolvePath()` >> - simplify String-related operations > > The `String.equalsIgnoreCase(…)` and `String.compareToIgnoreCase(…)` changes > are incorrect, as the `String.*IgnoreCase(…)` methods compare all **Unicode** > code points case‑insensitively using **Unicode** rules for the current or > specified locale, whereas the **URI** specification does case‑insensitive > comparison only for characters in the **US‑ASCII** range [[RFC3986]]. > > > > https://github.com/openjdk/jdk/blob/408a3a8e29006798071cd6f185e415bc2bc62282/src/java.base/share/classes/java/net/URI.java#L1825-L1830 > > https://github.com/openjdk/jdk/blob/408a3a8e29006798071cd6f185e415bc2bc62282/src/java.base/share/classes/java/net/URI.java#L1832-L1844 > > [RFC3986]: https://datatracker.ietf.org/doc/html/rfc3986 @ExE-Boss > String.*IgnoreCase(…) methods compare all Unicode code points > case‑insensitively using Unicode rules for the current or specified locale, > whereas the URI specification does case‑insensitive comparison only for > characters in the US‑ASCII range Aren't all the items of US-ASCII range belong to Unicode regardless of locale? - PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. The security/crypto parts look good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Thu, 19 May 2022 09:31:07 GMT, Kevin Walls wrote: >> Replaces usages of articles that follow each other in all combinations: >> a/the, an?/an?, the/the… >> >> Also, I fixed a couple of spelling mistakes. > > test/jdk/sun/security/tools/jarsigner/OldSig.java line 32: > >> 30: /* >> 31: * See also PreserveRawManifestEntryAndDigest.java for tests with >> arbitrarily >> 32: * formatted individual sections in addition the main attributes tested > > "in addition to the..." +1. - PR: https://git.openjdk.java.net/jdk/pull/8768
Integrated: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. This pull request has now been integrated. Changeset: d24c84e7 Author:Daniel Jeliński URL: https://git.openjdk.java.net/jdk/commit/d24c84e7687890db88550b05ff9eebe9cae361b2 Stats: 140 lines in 10 files changed: 68 ins; 39 del; 33 mod 8286873: Improve websocket test execution time Reviewed-by: dfuchs, prappo - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v3]
On Thu, 19 May 2022 12:05:45 GMT, Daniel Jeliński wrote: >> This PR improves the execution time of jdk_net tests (and, by extension, >> tier2) by about 3 minutes. >> >> Tests located under `jdk/java/net/httpclient/websocket` are never run in >> parallel. Each of the 8 modified `Pending***` tests originally required 40 >> seconds to complete. After the proposed changes, they usually complete in 15 >> seconds. >> >> This PR modifies the tests to initially run with 1 second timeout. If the >> test fails with 1 second timeout, it is retried with timeout increased to 10 >> seconds (the original value). >> >> The modified tests were executed at least 10 times on each of: Windows, >> Linux (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were >> observed. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Make it clear that both actions hang Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8285521: Minor improvements in java.net.URI
On Tue, 26 Apr 2022 07:02:55 GMT, Сергей Цыпанов wrote: > - use `String.equalsIgnoreCase()` instead of hand-written code relying on > `String.charAt()` > - use `String.compareToIgnoreCase()` instead of hand-written code relying on > `String.charAt()` > - drop branches that are never executed > - drop unused argument from `URI.resolvePath()` > - simplify String-related operations The `String.equalsIgnoreCase(…)` and `String.compareToIgnoreCase(…)` changes are incorrect, as the `String.*IgnoreCase(…)` methods compare all **Unicode** code points case‑insensitively using **Unicode** rules for the current or specified locale, whereas the **URI** specification does case‑insensitive comparison only for characters in the **US‑ASCII** range [[RFC3986]]. https://github.com/openjdk/jdk/blob/408a3a8e29006798071cd6f185e415bc2bc62282/src/java.base/share/classes/java/net/URI.java#L1825-L1830 https://github.com/openjdk/jdk/blob/408a3a8e29006798071cd6f185e415bc2bc62282/src/java.base/share/classes/java/net/URI.java#L1832-L1844 [RFC3986]: https://datatracker.ietf.org/doc/html/rfc3986 - Changes requested by exe-b...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/8397
Re: RFR: 8286873: Improve websocket test execution time [v3]
On Thu, 19 May 2022 10:46:35 GMT, Pavel Rappo wrote: > > > What looks questionable is rearrangement of asserts: when `assertHangs` > > > moves down. assertNotDone(cfClose) can transitorry pass even if ping has > > > not hung. > > > > > > `assertHangs` either waits for 5 seconds or throws an exception, so > > `assertNotDone` after `assertHangs` is pretty much equivalent to > > `assertHangs` > > The order of these checks is important. On the other hand, I'm not sure how > important that importance is for the tests. With the proposed change, the > test might become more permissive than it should've been. > > Separately, why does this PR change `assertHangs(cfClose)` to > `assertNotDone(cfClose)`? > > The fact that you say that the tests still pass gives me some confidence. I'm > just trying to make sure that the tests still test what we think they test. Modified the tests to make it clear that we expect both CFs to hang. Let me know if that addresses your concerns. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v3]
> This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Make it clear that both actions hang - Changes: - all: https://git.openjdk.java.net/jdk/pull/8746/files - new: https://git.openjdk.java.net/jdk/pull/8746/files/bd7d3298..832d7f9a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=01-02 Stats: 24 lines in 9 files changed: 6 ins; 9 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8746/head:pull/8746 PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v2]
On Thu, 19 May 2022 09:42:05 GMT, Daniel Jeliński wrote: > > What looks questionable is rearrangement of asserts: when `assertHangs` > > moves down. assertNotDone(cfClose) can transitorry pass even if ping has > > not hung. > > `assertHangs` either waits for 5 seconds or throws an exception, so > `assertNotDone` after `assertHangs` is pretty much equivalent to `assertHangs` The order of these checks is important. On the other hand, I'm not sure how important that importance is for the tests. With the proposed change, the test might become more permissive than it should've been. Separately, why does this PR change `assertHangs(cfClose)` to `assertNotDone(cfClose)`? The fact that you say that the tests still pass gives me some confidence. I'm just trying to make sure that the tests still test what we think they test. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time [v2]
> This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Fix wording - Changes: - all: https://git.openjdk.java.net/jdk/pull/8746/files - new: https://git.openjdk.java.net/jdk/pull/8746/files/52b735bf..bd7d3298 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8746&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8746/head:pull/8746 PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8286873: Improve websocket test execution time
On Thu, 19 May 2022 09:22:57 GMT, Pavel Rappo wrote: > What looks questionable is rearrangement of asserts: when `assertHangs` moves > down. assertNotDone(cfClose) can transitorry pass even if ping has not hung. `assertHangs` either waits for 5 seconds or throws an exception, so `assertNotDone` after `assertHangs` is pretty much equivalent to `assertHangs` - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. OK. I started with serviceability but then went through everything as it's hard to record what you have/haven't seen in these long reviews. test/jdk/java/net/InterfaceAddress/Equals.java line 81: > 79: /** > 80: * Returns an InterfaceAddress instance with its fields set the values > 81: * specificed. probably a typo for "set to the values specified." - Marked as reviewed by kevinw (Committer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. test/jdk/jdk/nio/zipfs/TestLocOffsetFromZip64EF.java line 84: > 82: > 83: /** > 84: * Create a Zip file that will result in an Zip64 Extra (EXT) header "result in a Zip64..." test/jdk/sun/security/tools/jarsigner/OldSig.java line 32: > 30: /* > 31: * See also PreserveRawManifestEntryAndDigest.java for tests with > arbitrarily > 32: * formatted individual sections in addition the main attributes tested "in addition to the..." - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 14:55:11 GMT, Daniel Fuchs wrote: > I am a bit less sure about moving the post-asserts inside the loop I moved them because they too can fail if the original blocked future suddenly completes. Side effect of this change is that any failures that happen after websocket.abort will be retried (isDone will return true). I think this is acceptable. > or closing before asserting that the cf hangs, but if I understand the logic > correctly it seems ok too. `assertHangs` essentially just waits for five seconds to see if the future finishes in the meantime. With the proposed change we are waiting for 2 futures at the same time. - PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/cds/filemap.cpp line 1914: > 1912: > 1913: // the current value of the pointers to be patched must be within > this > 1914: // range (i.e., must be between the requested base address, and the > of the current archive). "the of the" ? Maybe "..and the address of the current archive", or maybe it was a typo for "and that of the current archive". - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8286873: Improve websocket test execution time
On Tue, 17 May 2022 12:45:52 GMT, Daniel Jeliński wrote: > This PR improves the execution time of jdk_net tests (and, by extension, > tier2) by about 3 minutes. > > Tests located under `jdk/java/net/httpclient/websocket` are never run in > parallel. Each of the 8 modified `Pending***` tests originally required 40 > seconds to complete. After the proposed changes, they usually complete in 15 > seconds. > > This PR modifies the tests to initially run with 1 second timeout. If the > test fails with 1 second timeout, it is retried with timeout increased to 10 > seconds (the original value). > > The modified tests were executed at least 10 times on each of: Windows, Linux > (both x64 and aarch64), MacOS (both x64 and aarch64). No failures were > observed. I have poor recollection of the mechanics of these tests. The source of wait is timed get and `assertHang`. You are reducing the former. That looks good. What looks questionable is rearrangement of asserts: when `assertHangs` moves down. assertNotDone(cfClose) can transitorry pass even if ping has not hung. Tiriviality: since you are here, please remove 1. superfluous "are" on BlowupOutputQueue:60 2. "the a" on BlowupOutputQueue:63 test/jdk/java/net/httpclient/websocket/PendingOperations.java line 44: > 42: static final Class IOE = IOException.class; > 43: // Time after which we deem that the local send buffer and remote > 44: // receive buffer must be full. This has been heuristically > determined. "Heuristically" was a weird word to use here; should've been "empirically", I think. You deleted the whole sentence, which is okay too. - Changes requested by prappo (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8746
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/interpreter/bytecodeUtils.cpp line 186: > 184: static const int _max_cause_detail = 5; > 185: > 186: // Merges the stack the given bci with the given stack. If there "...the stack at the given bci..." - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/hotspot/share/opto/graphKit.cpp line 3626: > 3624: // The optional arguments are for specialized use by intrinsics: > 3625: // - If 'extra_slow_test' if not null is an extra condition for the > slow-path. > 3626: // - If 'return_size_val', report the total object size to the caller. "reports the total" - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote:
> Replaces usages of articles that follow each other in all combinations:
> a/the, an?/an?, the/the…
>
> Also, I fixed a couple of spelling mistakes.
src/jdk.jdi/share/classes/com/sun/jdi/ClassType.java line 348:
> 346:
> 347: /**
> 348: * Returns the single non-abstract {@link Method} visible from
I would think "Returns a single ..." because the implementation returns the
first match it finds, from possibly many.
-
PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. src/jdk.sctp/share/classes/com/sun/nio/sctp/ShutdownNotification.java line 28: > 26: > 27: /** > 28: * Notification emitted when a peers shutdowns the association. Maybe: "...when a peer shuts down an association." (could be "shuts down the association" if there is only ever one, but using "an" looks safe) - PR: https://git.openjdk.java.net/jdk/pull/8768
