Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update [v2]
On Thu, 14 Dec 2023 22:17:54 GMT, Alisen Chung wrote: >> Translation drop for JDK22 RDP1 > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > removed quotes around Ablehnen The java.xml changes look good. - Marked as reviewed by joehw (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1783142189
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 13:29:43 GMT, Jorn Vernee wrote:
>> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>>
>>> 49: runInNewProcess(UpcallRunner.class, true,
>>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>>> 50: .shouldNotHaveExitValue(0)
>>> 51: .shouldNotHaveFatalError();
>>
>> Just curious what non-zero exit value is actually expected here and below?
>
> Any non-zero exit value is acceptable. The intent here is to check that the
> process didn't complete normally.
A non-zero non-crashing value. Just wondering what that actually would be?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1427557140
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v2]
On Thu, 14 Dec 2023 21:14:33 GMT, Eirik Bjorsnos wrote:
>> Archie Cobbs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Address review comments.
>
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 37:
>
>> 35: public static void main(String [] args) throws IOException {
>> 36:
>> 37: // Create gz data
>
> Perhaps expand the comment to explain that you are creating a concatenated
> stream?
Thanks - should be addressed in c7087e55319.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54:
>
>> 52: try (GZIPInputStream in = new GZIPInputStream(new
>> ByteArrayInputStream(gz32))) {
>> 53: count1 = countBytes(in);
>> 54: }
>
> Consider rewriting countBytes to take the
> ByteArrayInputStream/ZeroAvailableInputStream as a parameter, so you could
> just do:
>
> ```suggestion
> long count1 = countBytes(new ByteArrayInputStream(gz32));
Thanks - should be addressed in c7087e55319.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
>
>> 54: }
>> 55:
>> 56: // (a) Read it from a stream where available() always returns
>> zero
>
> Suggestion:
>
> // (b) Read it from a stream where available() always returns zero
Thanks - should be addressed in c7087e55319.
> test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 64:
>
>> 62: // They should be the same
>> 63: if (count2 != count1)
>> 64: throw new AssertionError(count2 + " != " + count1);
>
> Consider converting the test to JUnit, this would allow:
> Suggestion:
>
> asssertEquals(count1, count2);
Thanks - should be addressed in c7087e55319.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427507217
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427507107
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427507154
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427507189
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream [v2]
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the > underlying `InputStream` says is how many bytes are `available()`. But this > is inappropriate because `InputStream.available()` is just an estimate and is > allowed (for example) to always return zero. > > The fix is to ignore what's `available()` and just proceed and see what > happens. If fewer bytes are available than required, the attempt to extend to > another stream is canceled just as it was before, e.g., when the next stream > header couldn't be read. Archie Cobbs has updated the pull request incrementally with one additional commit since the last revision: Address review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/17113/files - new: https://git.openjdk.org/jdk/pull/17113/files/c07554d0..c7087e55 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17113&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17113&range=00-01 Stats: 29 lines in 1 file changed: 6 ins; 7 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/17113.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17113/head:pull/17113 PR: https://git.openjdk.org/jdk/pull/17113
RFR: 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing
ConcurrentHashMap's copy constructor calls `putAll()` -> `tryPresize()` -> `transfer()`. When coming from the copy constructor, the Map is empty, so there is nothing to transfer. But `transfer()` will still copy all the empty nodes from the old table to the new table. This patch avoids this work by only calling `tryPresize()` if the table is already initialized. If `table` is null, the initialization is deferred to `putVal()`, which calls `initTable()`. --- ### JMH results for testCopyConstructor before patch: Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": 937395.686 ±(99.9%) 99074.324 ns/op [Average] (min, avg, max) = (825732.550, 937395.686, 1072024.041), stdev = 92674.184 CI (99.9%): [838321.362, 1036470.010] (assumes normal distribution) after patch: Result "org.openjdk.bench.java.util.concurrent.Maps.testCopyConstructor": 620871.469 ±(99.9%) 59195.406 ns/op [Average] (min, avg, max) = (545304.633, 620871.469, 689013.573), stdev = 55371.419 CI (99.9%): [561676.063, 680066.875] (assumes normal distribution) Average time is decreased by about 33%. - Commit messages: - 8322149: ConcurrentHashMap copy constructor should not transfer from old table on presizing Changes: https://git.openjdk.org/jdk/pull/17116/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17116&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8322149 Stats: 22 lines in 2 files changed: 19 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17116.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17116/head:pull/17116 PR: https://git.openjdk.org/jdk/pull/17116
Re: RFR: 8321958: @param/@return descriptions of ZoneRules#isDaylightSavings() are incorrect [v2]
On Thu, 14 Dec 2023 17:08:02 GMT, Naoto Sato wrote: >> Small document correction on a return description. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Corrected @param description Thank you for the update Naoto. The current change looks good to me. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17098#pullrequestreview-1782999804
Re: More support for offset and length in methods operating on byte arrays
Thanks for the good suggestion Pavel - will look into that - for large arrays absolutely less costly than copying! In the general case I would still prefer if the libraries generally provided offset & length for byte array arguments to allow for "garbage free" code (i.e. avoiding to frequently create various wrapper objects like ByteBuffer from heap memory). Best Regards Trist On Thu, Dec 14, 2023 at 8:57 PM Pavel Rappo wrote: > > > > On 14 Dec 2023, at 06:10, Magnus wrote: > > > > In the java libraries there are many methods that operate on byte arrays > that do not allow you to specify offset and length for the relevant data > instead forcing you to copy the relevant part to new arrays before using > the methods reducing performance - I am for instance struggling with this > in java.util.Base64 where the Encoders and Decoders lack a length parameter > (also an offset would have been great even though I don't need that in my > case). > > Re: java.util.Base64. Encoder and Decoder also seem to be able to work > with ByteBuffer. If you have an array, you can cheaply create a ByteBuffer > wrapper around that array. The now-backing array would be read or written > though from the specific position and up to the specific limit. Would that > help? > > -Pavel > >
Re: RFR: 6356745: Introduce constructor for PriorityQueue with existing collection and custom comparator [v2]
On Thu, 14 Dec 2023 22:18:35 GMT, Valeh Hajiyev wrote: > @AlanBateman thanks for having a look. would you be able to create a CRS > request as I don't have permission to do so? You may not be able to create one, but you sure can look at some. If you do so, you'll see that obtaining a permission to create a [CSR](https://wiki.openjdk.org/display/csr/Main) is the least of your concerns. A CSR is a structured essay with elements of a checklist and it takes some effort to compose one. Here are some examples for you (feel free to look at more): - https://bugs.openjdk.org/browse/JDK-8189407 - https://bugs.openjdk.org/browse/JDK-8284377 - https://bugs.openjdk.org/browse/JDK-8287419 - https://bugs.openjdk.org/browse/JDK-8266572 - https://bugs.openjdk.org/browse/JDK-8191517 You've posted your suggestion on core-libs-dev and provided this essentially draft PR. Now simply wait for discussion, if any. While waiting, though, consider how you would test your proposal. Generally, familiarise yourself with other PRs, both Open and Closed (in OpenJDK PRs are not Merged in GitHub sense). See how changes similar to what you propose are processed. - PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1857046857
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update [v2]
On Thu, 14 Dec 2023 19:01:22 GMT, Joe Wang wrote: >> Alisen Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> removed quotes around Ablehnen > > src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/XMLMessages_de.properties > line 331: > >> 329: >> 330: # Implementation Property DTD >> 331: JDK_DTD_DENY = JAXP00010008: DOCTYPE ist nicht zulässig, wenn >> die DTD-Eigenschaft auf "Ablehnen" gesetzt wurde. Weitere Informationen: >> Eigenschaft jdk.xml.dtd.support in java.xml/module-summary. > > This version quoted "Ablehnen" while the other two (ja - "拒否" and zh_CN - > "拒绝") didn't. If we want to be consistent, "deny" would be better since > that's the literal value. The English version should have quoted "deny". > > The previous translations in these files have not been consistent. Some of > the key words were translated. If we want to keep this translation as is, > it's probably better to remove the quote in the "de" file. removed quotes around Ablehnen - PR Review Comment: https://git.openjdk.org/jdk/pull/17096#discussion_r1427423629
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 21:54:53 GMT, Markus KARG wrote: >> I mean SequenceInputStream, not the base class: >> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249 >> >> Could you please double-check? > > IMHO @vlsi is right. It is incorrect that we stop the transfer in the > overflow case. We should fix it as he suggested. I can do that if you like. Right, the base class. The suggested change was made for `InputStream` in 254d6990bcf75700f1c3aa18e0f8b73b639d9bad. It looks like it should be in `SequenceInputStream` as well. I created [JDK-8322141](https://bugs.openjdk.org/browse/JDK-8322141) to track this. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427398726
Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v13]
On Mon, 11 Dec 2023 16:37:38 GMT, Severin Gehwolf wrote: >> Please review this patch which adds a jlink mode to the JDK which doesn't >> need the packaged modules being present. A.k.a run-time image based jlink. >> Fundamentally this patch adds an option to use `jlink` even though your JDK >> install might not come with the packaged modules (directory `jmods`). This >> is particularly useful to further reduce the size of a jlinked runtime. >> After the removal of the concept of a JRE, a common distribution mechanism >> is still the full JDK with all modules and packaged modules. However, >> packaged modules can incur an additional size tax. For example in a >> container scenario it could be useful to have a base JDK container including >> all modules, but without also delivering the packaged modules. This comes at >> a size advantage of `~25%`. Such a base JDK container could then be used to >> `jlink` application specific runtimes, further reducing the size of the >> application runtime image (App + JDK runtime; as a single image *or* >> separate bundles, depending on the app being modularized). >> >> The basic design of this approach is to add a jlink plugin for tracking >> non-class and non-resource files of a JDK install. I.e. files which aren't >> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` >> class which has all the info of what constitutes the final jlinked runtime. >> >> Basic usage example: >> >> $ diff -u <(./bin/java --list-modules --limit-modules java.se) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules java.se) >> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) >> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules >> --limit-modules jdk.jlink) >> $ ls ../linux-x86_64-server-release/images/jdk/jmods >> java.base.jmodjava.net.http.jmod java.sql.rowset.jmod >> jdk.crypto.ec.jmod jdk.internal.opt.jmod >> jdk.jdi.jmod jdk.management.agent.jmod jdk.security.auth.jmod >> java.compiler.jmodjava.prefs.jmod java.transaction.xa.jmod >> jdk.dynalink.jmod jdk.internal.vm.ci.jmod >> jdk.jdwp.agent.jmod jdk.management.jfr.jmodjdk.security.jgss.jmod >> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod >> jdk.editpad.jmod jdk.internal.vm.compiler.jmod >> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod >> java.desktop.jmod java.scripting.jmod java.xml.jmod >> jdk.hotspot.agent.jmod jdk.i... > > Severin Gehwolf has updated the pull request incrementally with two > additional commits since the last revision: > > - Disallow packaged modules and run-time image link > - Only check for existing path when not a scratch task > >When using a run-time image link and the initial build was produced with >the --keep-packaged-modules option, we don't need to check existing >paths to the location where packaged modules need to be copied. This >breaks the --verbose output validation. Sorry for the delay. I started the review. I think there may be better places to make these changes e.g. read `fs_$MODULE_files` and prepare and print out the verbose log. I am spending time to get familiar with the code and suggest better places to implement this. Hope I can send something next week. - PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-1856912188
Re: RFR: 8320971: Use BufferedInputStream.buf directly when param of implTransferTo() is trusted [v15]
On Thu, 14 Dec 2023 08:47:03 GMT, Sergey Tsypanov wrote:
>> It looks like we can skip copying of `byte[]` in
>> `BufferedInputStream.implTransferTo()` for `OutputStreams` residing in
>> `java.io`.
>>
>> See comment by @vlsi in
>> https://github.com/openjdk/jdk/pull/10525/files#diff-e19c508d1bb6ee78697ecca66947c395adda0d9c49a85bf696e677ecbd977af1R612
>
> Sergey Tsypanov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8320971: Revert irrelevant changes
src/java.base/share/classes/java/io/BufferedInputStream.java line 646:
> 644: int avail = count - pos;
> 645: if (avail > 0) {
> 646: if (isTrusted(out)) {
It might be cleaner for now to drop `isTrusted()` and just do the class check
explicitly here. That still takes care of the main intent of not passing the
buffer to an untrustworthy stream. Further cleanup and consolidation can be
done later.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16879#discussion_r1427391530
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 22:35:18 GMT, Serguei Spitsyn wrote:
>> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>>
>>> 1041: notifyJvmtiDisableSuspend(true);
>>> 1042: try {
>>> 1043: // include the carrier thread state and name when
>>> mounted
>>
>> This one too, can you move the comment to before the
>> notifyJvmtiDisableSuspend.
>
> Moved both comments out of try blocks.
> What about this one (it seems we would wont to do the same) ? :
>
> notifyJvmtiDisableSuspend(true);
> try {
> // unpark carrier thread when pinned
> synchronized (carrierThreadAccessLock()) {
> Thread carrier = carrierThread;
> if (carrier != null && ((s = state()) == PINNED || s
> == TIMED_PINNED)) {
> U.unpark(carrier);
> }
> }
> } finally {
> notifyJvmtiDisableSuspend(false);
> }
Moved 3 comments out of try blocks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427386103
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v6]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: moved a couple of comments out of try blocks - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/ad990422..917dc724 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=04-05 Stats: 6 lines in 1 file changed: 3 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 19:50:00 GMT, Alan Bateman wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: moved notifyJvmtiDisableSuspend(true) out of try-block
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>
>> 1041: notifyJvmtiDisableSuspend(true);
>> 1042: try {
>> 1043: // include the carrier thread state and name when
>> mounted
>
> This one too, can you move the comment to before the
> notifyJvmtiDisableSuspend.
Moved both comments out of try blocks.
What about this one (it seems we would wont to do the same) ? :
notifyJvmtiDisableSuspend(true);
try {
// unpark carrier thread when pinned
synchronized (carrierThreadAccessLock()) {
Thread carrier = carrierThread;
if (carrier != null && ((s = state()) == PINNED || s ==
TIMED_PINNED)) {
U.unpark(carrier);
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427373522
Re: RFR: 6356745: Introduce constructor for PriorityQueue with existing collection and custom comparator [v2]
On Thu, 14 Dec 2023 20:36:50 GMT, Valeh Hajiyev wrote: >> This commit addresses the current limitation in the `PriorityQueue` >> implementation, which lacks a constructor to efficiently create a priority >> queue with a custom comparator and an existing collection. In order to >> create such a queue, we currently need to initialize a new queue with custom >> comparator, and after that populate the queue using `addAll()` method, which >> in the background calls `add()` method (which takes `O(logn)` time) for each >> element of the collection (`n` times). This is resulting in an overall time >> complexity of `O(nlogn)`. >> >> >> PriorityQueue pq = new PriorityQueue<>(customComparator); >> pq.addAll(existingCollection); >> >> >> The pull request introduces a new constructor to streamline this process and >> reduce the time complexity to `O(n)`. If you create the queue above using >> the new constructor, the contents of the collection will be copied (which >> takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) >> will be called once (another `O(n)` time). Overall the operation will be >> reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. >> >> >> PriorityQueue pq = new PriorityQueue<>(existingCollection, >> customComparator); > > Valeh Hajiyev has updated the pull request incrementally with one additional > commit since the last revision: > > updated the javadoc @AlanBateman thanks for having a look. would you be able to create a CRS request as I don't have permission to do so? - PR Comment: https://git.openjdk.org/jdk/pull/17045#issuecomment-1856784903
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update [v2]
> Translation drop for JDK22 RDP1 Alisen Chung has updated the pull request incrementally with one additional commit since the last revision: removed quotes around Ablehnen - Changes: - all: https://git.openjdk.org/jdk/pull/17096/files - new: https://git.openjdk.org/jdk/pull/17096/files/3c8e871d..0e964a06 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17096&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17096&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17096/head:pull/17096 PR: https://git.openjdk.org/jdk/pull/17096
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 mach5 looks green - PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1856733375
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 20:21:10 GMT, Vladimir Sitnikov wrote: >>> What do you think? >> >> I think that you are looking at an obsolete repository: >> >> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802 > > I mean SequenceInputStream, not the base class: > https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249 > > Could you please double-check? IMHO @vlsi is right. It is incorrect that we stop the transfer in the overflow case. We should fix it as he suggested. I can do that if you like. - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427342442
Re: RFR: 8320919: Clarify Locale related system properties [v5]
On Thu, 14 Dec 2023 21:27:06 GMT, Roger Riggs wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reflects review comments > > src/java.base/share/classes/java/util/Locale.java line 264: > >> 262: * Default Locale >> 263: * >> 264: * The default Locale is mainly provided for any locale-sensitive >> methods if no > > The word "mainly" can be omitted, it doesn't add any clarity. Removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1427332501
Re: RFR: 8320919: Clarify Locale related system properties [v6]
> This is a doc change to clarify what the `Default Locale` is, and how it is > established during the system startup using the system properties. Those > locale-related system properties have existed since the early days of Java, > but have never been publicly documented before. It is also the intention of > this PR to clarify those system properties and how they are overridden. A > corresponding CSR has been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflects review comments - Changes: - all: https://git.openjdk.org/jdk/pull/17065/files - new: https://git.openjdk.org/jdk/pull/17065/files/48466817..27195f51 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17065&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17065.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17065/head:pull/17065 PR: https://git.openjdk.org/jdk/pull/17065
Re: RFR: 8320919: Clarify Locale related system properties [v5]
On Wed, 13 Dec 2023 19:00:52 GMT, Naoto Sato wrote: >> This is a doc change to clarify what the `Default Locale` is, and how it is >> established during the system startup using the system properties. Those >> locale-related system properties have existed since the early days of Java, >> but have never been publicly documented before. It is also the intention of >> this PR to clarify those system properties and how they are overridden. A >> corresponding CSR has been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reflects review comments src/java.base/share/classes/java/util/Locale.java line 264: > 262: * Default Locale > 263: * > 264: * The default Locale is mainly provided for any locale-sensitive > methods if no The word "mainly" can be omitted, it doesn't add any clarity. - PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1427307955
Integrated: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > > Thanks This pull request has now been integrated. Changeset: 692be577 Author:David Holmes URL: https://git.openjdk.org/jdk/commit/692be577385844bf00a01ff10e390e014191569f Stats: 193 lines in 27 files changed: 36 ins; 51 del; 106 mod 8322065: Initial nroff manpage generation for JDK 23 Reviewed-by: alanb - PR: https://git.openjdk.org/jdk/pull/17101
Integrated: 8321480: ISO 4217 Amendment 176 Update
On Thu, 7 Dec 2023 19:43:14 GMT, Justin Lu wrote: > Please review this PR which incorporates the ISO 4217 Amendment 176 Update. > As the replacement of `ANG` to `XCG` won't occur until 2025, this change does > not need to go into JDK22. `HR` was also updated to remove the past cutover > dates. > > This update also demonstrated that `Currency::getAvailableCurrencies` may > return a future currency that should not be returned. > _GenerateCurrencyData.java_ was updated to ensure such currencies are not > added to the other currency list. > > Additionally, this change also converted a parameterized test to a normal > JUnit test, due to output overflow. This pull request has now been integrated. Changeset: 8b24851b Author:Justin Lu URL: https://git.openjdk.org/jdk/commit/8b24851b9d3619c41c7a6cdb9193ed26a9b732dc Stats: 43 lines in 5 files changed: 10 ins; 0 del; 33 mod 8321480: ISO 4217 Amendment 176 Update Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/17023
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
On Thu, 14 Dec 2023 20:15:39 GMT, Archie Cobbs wrote:
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the
> underlying `InputStream` says is how many bytes are `available()`. But this
> is inappropriate because `InputStream.available()` is just an estimate and is
> allowed (for example) to always return zero.
>
> The fix is to ignore what's `available()` and just proceed and see what
> happens. If fewer bytes are available than required, the attempt to extend to
> another stream is canceled just as it was before, e.g., when the next stream
> header couldn't be read.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 37:
> 35: public static void main(String [] args) throws IOException {
> 36:
> 37: // Create gz data
Perhaps expand the comment to explain that you are creating a concatenated
stream?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427291799
Re: RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
On Thu, 14 Dec 2023 20:15:39 GMT, Archie Cobbs wrote:
> `GZIPInputStream`, when looking for a concatenated stream, relies on what the
> underlying `InputStream` says is how many bytes are `available()`. But this
> is inappropriate because `InputStream.available()` is just an estimate and is
> allowed (for example) to always return zero.
>
> The fix is to ignore what's `available()` and just proceed and see what
> happens. If fewer bytes are available than required, the attempt to extend to
> another stream is canceled just as it was before, e.g., when the next stream
> header couldn't be read.
The test could benefit from a conversion to JUnit. Not sure I love final local
variables, I see the split assignment inside the try/catch makes it useful, but
perhaps if you rewrite countBytes as suggested, final will be less useful.
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 54:
> 52: try (GZIPInputStream in = new GZIPInputStream(new
> ByteArrayInputStream(gz32))) {
> 53: count1 = countBytes(in);
> 54: }
Consider rewriting countBytes to take the
ByteArrayInputStream/ZeroAvailableInputStream as a parameter, so you could just
do:
```suggestion
long count1 = countBytes(new ByteArrayInputStream(gz32));
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 56:
> 54: }
> 55:
> 56: // (a) Read it from a stream where available() always returns zero
Suggestion:
// (b) Read it from a stream where available() always returns zero
test/jdk/java/util/zip/GZIP/GZIPInputStreamAvailable.java line 64:
> 62: // They should be the same
> 63: if (count2 != count1)
> 64: throw new AssertionError(count2 + " != " + count1);
Consider converting the test to JUnit, this would allow:
Suggestion:
asssertEquals(count1, count2);
-
PR Review: https://git.openjdk.org/jdk/pull/17113#pullrequestreview-1782746089
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427283057
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427283554
PR Review Comment: https://git.openjdk.org/jdk/pull/17113#discussion_r1427285307
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
One more request – Current PR cannot be merged without CRS. Could someone
please help me to create a CSR request for the issue below?
https://bugs.openjdk.org/browse/JDK-6356745
Unfortunately, I don't have permission to create a CSR request.
-Valeh
On Thu, Dec 14, 2023 at 9:40 PM Archie Cobbs wrote:
> Looks great - thanks (I'm not an official reviewer so I can't approve it
> though).
>
> -Archie
>
> On Thu, Dec 14, 2023 at 2:37 PM Valeh Hajiyev
> wrote:
>
>> Yes, there's no such precondition. Thanks for having a look, I updated
>> the javadoc as you suggested, and linked it to the old existing ticket.
>> It's now ready for review.
>>
>> I would appreciate if you could have a look again.
>>
>> Cheers,
>> Valeh
>>
>> On Thu, Dec 14, 2023 at 4:22 PM Archie Cobbs
>> wrote:
>>
>>> On Thu, Dec 14, 2023 at 4:56 AM Viktor Klang
>>> wrote:
>>>
I presume that the precondition to have the original collection be
pre-ordered according to the supplied Comparator can be verified by
checking before adding each element in the collection to the PQ that it
compareTo equal-or-greater to the previous one?
>>>
>>> Hmm... something is not adding up.
>>>
>>> Either (a) there is a precondition that the collection already be sorted
>>> or (b) there's no such precondition.
>>>
>>> In case (a):
>>>
>>>- Why isn't the new constructor invoking
>>>initElementsFromCollection() instead of initFromCollection()?
>>>- The precondition is not very obvious from the Javadoc description,
>>>and moreover what happens when the precondition is not met is not
>>>documented at all.
>>>
>>> In case (b):
>>>
>>>- The Javadoc is misleading because it (ambiguously) implies there
>>>is a precondition with the wording "collection that orders its elements
>>>according to the specified comparator" (the referent of "that" is
>>> ambiguous
>>>- does it refer to the collection or the PriorityQueue?)
>>>
>>> From the PR description it seems clear that there is no such
>>> precondition. So maybe the Javadoc should say this instead:
>>>
>>> Creates a {@code PriorityQueue} containing the elements in the specified
collection. The {@code PriorityQueue} will order its elements according to
the specified comparator.
>>>
>>> -Archie
>>>
>>> --
>>> Archie L. Cobbs
>>>
>>
>
> --
> Archie L. Cobbs
>
Re: More support for offset and length in methods operating on byte arrays
Yes, overloads that accepted both source and destination ByteBuffer were there
at some point, but then were removed:
commit 591834e28d482ea6a375ab215958e1635a7b111d
Author: Xueming Shen
Date: Tue Dec 3 17:44:31 2013 -0800
8028397: Undo the lenient MIME BASE64 decoder support change (JDK-8025003)
and remove methods de/encode(buf, buf)
Updated the spec and implementation as requested
Reviewed-by: alanb
Let me paste the respective JBS description here for readers convenience:
The change set for "lenient mime decoder" support change (JDK-8025003) does not
meet the requirement needed by the submitter and there is different
understanding and opinion of what the "lenient" should be for the Base64
decoder. Given we are in late stage of the release. The decision is to back out
the change and leave the functionality for future releases.
Also during the discussion, it became clear that the decode/encode(ByteBuffer,
ByteBuffer) method pair are insufficient to support the requested de/encoding
continuation functionality ("underflow" handling, in charset coder's term).
Given the the most usage of these pair is for the channel read/writing, it
appears it might be more useful/desired to provide a pair of
wrap(readable/writableByteChannel) methods stead. While the wrap(channel)
methods are under development, it does not appear it will make jdk8. For jdk8
we have decided to remove the decode/encode(Buffer, Buffer).
> On 14 Dec 2023, at 20:24, Pavel Rappo wrote:
>
> Upon a closer look, switching to ByteBuffer would only get you 50% towards
> where you want to be: the resulting ByteBuffer, whether encoded or decoded,
> is *allocated* by the respective methods and then returned as a result rather
> than accepted by those methods as a parameter.
>
>> On 14 Dec 2023, at 19:57, Pavel Rappo wrote:
>>
>>
>>
>>> On 14 Dec 2023, at 06:10, Magnus wrote:
>>>
>>> In the java libraries there are many methods that operate on byte arrays
>>> that do not allow you to specify offset and length for the relevant data
>>> instead forcing you to copy the relevant part to new arrays before using
>>> the methods reducing performance - I am for instance struggling with this
>>> in java.util.Base64 where the Encoders and Decoders lack a length parameter
>>> (also an offset would have been great even though I don't need that in my
>>> case).
>>
>> Re: java.util.Base64. Encoder and Decoder also seem to be able to work with
>> ByteBuffer. If you have an array, you can cheaply create a ByteBuffer
>> wrapper around that array. The now-backing array would be read or written
>> though from the specific position and up to the specific limit. Would that
>> help?
>>
>> -Pavel
>>
>
RFR: 7036144: GZIPInputStream readTrailer uses faulty available() test for end-of-stream
`GZIPInputStream`, when looking for a concatenated stream, relies on what the underlying `InputStream` says is how many bytes are `available()`. But this is inappropriate because `InputStream.available()` is just an estimate and is allowed (for example) to always return zero. The fix is to ignore what's `available()` and just proceed and see what happens. If fewer bytes are available than required, the attempt to extend to another stream is canceled just as it was before, e.g., when the next stream header couldn't be read. - Commit messages: - Fix bug in GZIPInputStream when underlying available() returns short. Changes: https://git.openjdk.org/jdk/pull/17113/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17113&range=00 Issue: https://bugs.openjdk.org/browse/JDK-7036144 Stats: 101 lines in 2 files changed: 86 ins; 9 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17113.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17113/head:pull/17113 PR: https://git.openjdk.org/jdk/pull/17113
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 Marked as reviewed by asemenyuk (Reviewer). jpackage changes look good - PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1782697421 PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1856551939
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
Looks great - thanks (I'm not an official reviewer so I can't approve it
though).
-Archie
On Thu, Dec 14, 2023 at 2:37 PM Valeh Hajiyev
wrote:
> Yes, there's no such precondition. Thanks for having a look, I updated the
> javadoc as you suggested, and linked it to the old existing ticket. It's
> now ready for review.
>
> I would appreciate if you could have a look again.
>
> Cheers,
> Valeh
>
> On Thu, Dec 14, 2023 at 4:22 PM Archie Cobbs
> wrote:
>
>> On Thu, Dec 14, 2023 at 4:56 AM Viktor Klang
>> wrote:
>>
>>> I presume that the precondition to have the original collection be
>>> pre-ordered according to the supplied Comparator can be verified by
>>> checking before adding each element in the collection to the PQ that it
>>> compareTo equal-or-greater to the previous one?
>>>
>>
>> Hmm... something is not adding up.
>>
>> Either (a) there is a precondition that the collection already be sorted
>> or (b) there's no such precondition.
>>
>> In case (a):
>>
>>- Why isn't the new constructor invoking initElementsFromCollection()
>>instead of initFromCollection()?
>>- The precondition is not very obvious from the Javadoc description,
>>and moreover what happens when the precondition is not met is not
>>documented at all.
>>
>> In case (b):
>>
>>- The Javadoc is misleading because it (ambiguously) implies there is
>>a precondition with the wording "collection that orders its elements
>>according to the specified comparator" (the referent of "that" is
>> ambiguous
>>- does it refer to the collection or the PriorityQueue?)
>>
>> From the PR description it seems clear that there is no such
>> precondition. So maybe the Javadoc should say this instead:
>>
>> Creates a {@code PriorityQueue} containing the elements in the specified
>>> collection. The {@code PriorityQueue} will order its elements according to
>>> the specified comparator.
>>>
>>
>> -Archie
>>
>> --
>> Archie L. Cobbs
>>
>
--
Archie L. Cobbs
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
Yes, there's no such precondition. Thanks for having a look, I updated the
javadoc as you suggested, and linked it to the old existing ticket. It's
now ready for review.
I would appreciate if you could have a look again.
Cheers,
Valeh
On Thu, Dec 14, 2023 at 4:22 PM Archie Cobbs wrote:
> On Thu, Dec 14, 2023 at 4:56 AM Viktor Klang
> wrote:
>
>> I presume that the precondition to have the original collection be
>> pre-ordered according to the supplied Comparator can be verified by
>> checking before adding each element in the collection to the PQ that it
>> compareTo equal-or-greater to the previous one?
>>
>
> Hmm... something is not adding up.
>
> Either (a) there is a precondition that the collection already be sorted
> or (b) there's no such precondition.
>
> In case (a):
>
>- Why isn't the new constructor invoking initElementsFromCollection()
>instead of initFromCollection()?
>- The precondition is not very obvious from the Javadoc description,
>and moreover what happens when the precondition is not met is not
>documented at all.
>
> In case (b):
>
>- The Javadoc is misleading because it (ambiguously) implies there is
>a precondition with the wording "collection that orders its elements
>according to the specified comparator" (the referent of "that" is ambiguous
>- does it refer to the collection or the PriorityQueue?)
>
> From the PR description it seems clear that there is no such precondition.
> So maybe the Javadoc should say this instead:
>
> Creates a {@code PriorityQueue} containing the elements in the specified
>> collection. The {@code PriorityQueue} will order its elements according to
>> the specified comparator.
>>
>
> -Archie
>
> --
> Archie L. Cobbs
>
Re: RFR: 6356745: Introduce constructor for PriorityQueue with existing collection and custom comparator [v2]
> This commit addresses the current limitation in the `PriorityQueue` > implementation, which lacks a constructor to efficiently create a priority > queue with a custom comparator and an existing collection. In order to create > such a queue, we currently need to initialize a new queue with custom > comparator, and after that populate the queue using `addAll()` method, which > in the background calls `add()` method (which takes `O(logn)` time) for each > element of the collection (`n` times). This is resulting in an overall time > complexity of `O(nlogn)`. > > > PriorityQueue pq = new PriorityQueue<>(customComparator); > pq.addAll(existingCollection); > > > The pull request introduces a new constructor to streamline this process and > reduce the time complexity to `O(n)`. If you create the queue above using > the new constructor, the contents of the collection will be copied (which > takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) > will be called once (another `O(n)` time). Overall the operation will be > reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > customComparator); Valeh Hajiyev has updated the pull request incrementally with one additional commit since the last revision: updated the javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/17045/files - new: https://git.openjdk.org/jdk/pull/17045/files/8f35fe91..1cce713d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17045&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17045&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17045.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17045/head:pull/17045 PR: https://git.openjdk.org/jdk/pull/17045
RFR: 6356745: Introduce constructor for PriorityQueue with existing collection and custom comparator
This commit addresses the current limitation in the `PriorityQueue` implementation, which lacks a constructor to efficiently create a priority queue with a custom comparator and an existing collection. In order to create such a queue, we currently need to initialize a new queue with custom comparator, and after that populate the queue using `addAll()` method, which in the background calls `add()` method (which takes `O(logn)` time) for each element of the collection (`n` times). This is resulting in an overall time complexity of `O(nlogn)`. PriorityQueue pq = new PriorityQueue<>(customComparator); pq.addAll(existingCollection); The pull request introduces a new constructor to streamline this process and reduce the time complexity to `O(n)`. If you create the queue above using the new constructor, the contents of the collection will be copied (which takes `O(n)` time) and then later `heapify()` operation (Floyd's algorithm) will be called once (another `O(n)` time). Overall the operation will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. PriorityQueue pq = new PriorityQueue<>(existingCollection, customComparator); - Commit messages: - fix styling - Introduce constructor for PriorityQueue with existing collection and custom comparator Changes: https://git.openjdk.org/jdk/pull/17045/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17045&range=00 Issue: https://bugs.openjdk.org/browse/JDK-6356745 Stats: 19 lines in 1 file changed: 19 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17045.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17045/head:pull/17045 PR: https://git.openjdk.org/jdk/pull/17045
Re: More support for offset and length in methods operating on byte arrays
Upon a closer look, switching to ByteBuffer would only get you 50% towards where you want to be: the resulting ByteBuffer, whether encoded or decoded, is *allocated* by the respective methods and then returned as a result rather than accepted by those methods as a parameter. > On 14 Dec 2023, at 19:57, Pavel Rappo wrote: > > > >> On 14 Dec 2023, at 06:10, Magnus wrote: >> >> In the java libraries there are many methods that operate on byte arrays >> that do not allow you to specify offset and length for the relevant data >> instead forcing you to copy the relevant part to new arrays before using the >> methods reducing performance - I am for instance struggling with this in >> java.util.Base64 where the Encoders and Decoders lack a length parameter >> (also an offset would have been great even though I don't need that in my >> case). > > Re: java.util.Base64. Encoder and Decoder also seem to be able to work with > ByteBuffer. If you have an array, you can cheaply create a ByteBuffer wrapper > around that array. The now-backing array would be read or written though from > the specific position and up to the specific limit. Would that help? > > -Pavel >
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 19:22:18 GMT, Brian Burkhalter wrote:
>> src/java.base/share/classes/java/io/SequenceInputStream.java line 249:
>>
>>> 247: transferred = Math.addExact(transferred,
>>> in.transferTo(out));
>>> 248: } catch (ArithmeticException ignore) {
>>> 249: return Long.MAX_VALUE;
>>
>> @bplb , this looks like a bug to me: once `transferred` reaches
>> `Long.MAX_VALUE` the transfer loop will terminate and the transfer will stop
>> even in the case there are more streams available in the sequence.
>>
>> I think the proper code should be `transferred = Long.MAX_VALUE` instead of
>> `return Long.MAX_VALUE` (and there should be no `if (transferred <
>> Long.MAX_VALUE)` check)
>>
>> What do you think?
>
>> What do you think?
>
> I think that you are looking at an obsolete repository:
>
> https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802
I mean SequenceInputStream, not the base class:
https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/SequenceInputStream.java#L249
Could you please double-check?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427231492
Re: Bug in GZIPInputStream.read() causing data loss
Hi Louis, On first glance this looks easy to fix. I've filed a draft PR here (pending tests) https://github.com/openjdk/jdk/pull/17113 -Archie On Thu, Dec 14, 2023 at 1:10 PM Louis Bergelson wrote: > Hello. This is my first time posting here so I apologize if this is the > wrong forum. I wanted to bring up an issue in the GZipInputStream which > was first identified in 2011, confirmed as a bug, and then never resolved. > > When reading certain GZIP files from certain types of InputStreams the > GZIPInputStream can misidentify the end of the stream and close early > resulting in silently truncated data. > > You can see the bug report which has a detailed description here: > https://bugs.openjdk.org/browse/JDK-7036144 > > In short it comes down to incorrect use of the (quite confusing) > InputStream.available() method to detect the end of stream. This typically > works fine with local files, but with network streams that might not have > bytes available at any given moment it fails nondeterministically. > > How could I go about getting this fixed? I can contribute a patch or > additional examples if necessary. > > Genomics data is typically encoded as block gzipped files, so this comes > up regularly and causes a lot of confusion. The workaround is to just not > use the GZIPInput stream. It seems like a core java class though so it > would be nice if it worked. > > Thank you, > Louis > -- Archie L. Cobbs
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 20:06:03 GMT, Archie Cobbs wrote: > One of the three `XMLStreamException` constructors that takes a `Throwable` > fails to pass it to the superclass constructor. > > This simple patch fixes that omission. > > It's worth considering if there is any code out there that is working around > this problem already by invoking `initCause()` manually. If so, that code > would start throwing an `IllegalStateException` after this change. > > So a more conservative fix would be to just add another constructor taking > the same arguments in a different order. But then again that's not much > better than just saying "always use initCause() with the broken constructor", > i.e., don't change anything. > > Hmm. I agree - let's go the CSR route. - PR Comment: https://git.openjdk.org/jdk/pull/17090#issuecomment-1856510107
Re: More support for offset and length in methods operating on byte arrays
> On 14 Dec 2023, at 06:10, Magnus wrote: > > In the java libraries there are many methods that operate on byte arrays that > do not allow you to specify offset and length for the relevant data instead > forcing you to copy the relevant part to new arrays before using the methods > reducing performance - I am for instance struggling with this in > java.util.Base64 where the Encoders and Decoders lack a length parameter > (also an offset would have been great even though I don't need that in my > case). Re: java.util.Base64. Encoder and Decoder also seem to be able to work with ByteBuffer. If you have an array, you can cheaply create a ByteBuffer wrapper around that array. The now-backing array would be read or written though from the specific position and up to the specific limit. Would that help? -Pavel
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 18:26:55 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: moved notifyJvmtiDisableSuspend(true) out of try-block
src/java.base/share/classes/java/lang/VirtualThread.java line 918:
> 916: notifyJvmtiDisableSuspend(true);
> 917: try {
> 918: // if mounted then return state of carrier thread
Can you move this comment line to before the notifyJvmtiDisableSuspend(true)?
src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
> 1041: notifyJvmtiDisableSuspend(true);
> 1042: try {
> 1043: // include the carrier thread state and name when
> mounted
This one too, can you move the comment to before the notifyJvmtiDisableSuspend.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198296
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198673
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:24:16 GMT, Serguei Spitsyn wrote: > Thank you, Alan. Fixed now. I believe, all your suggestions have been > addressed now. Thanks, it looks much better now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856485757
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 All around LGTM. From a skim I don't see any of the typical l10n related file mode issues, line ending issues, or values that should not be changed (like _WixLocalization Culture_). Might be worth filing a bug against the translation tool to see if it could support multi line translations for localized versions as @sormuras pointed out. src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_de.properties line 2293: > 2291: compiler.err.underscore.as.identifier=Ab Release 9 ist "_" ein > Schlüsselwort und kann nicht als ID verwendet werden > 2292: > 2293: compiler.err.use.of.underscore.not.allowed=Unterstrich ist hier nicht > zulässig\nAb Release 9 ist ''_'' ein Schlüsselwort und kann nicht als ID > verwendet werden\nAb Release 22 kann ''_'' als Name in der Deklaration > unbenannter Muster, lokaler Variablen, Ausnahmeparameter oder > Lambda-Parameter verwendet werden Just an observation, but the double quotes were converted to (two) single quotes. Previously, localized de files represented '' '' as " ", as different languages have different l10n rules (according to the translation contact). And this is seen with the other values on this file using "" instead of '' ''. But here it looks like they updated the value to match the quotes in of the value in the original .properties files. Not sure if they updated the rules and this is intentional, or unintentional. - PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1782404582 PR Review Comment: https://git.openjdk.org/jdk/pull/17096#discussion_r1427080553
More support for offset and length in methods operating on byte arrays
In the java libraries there are many methods that operate on byte arrays that do not allow you to specify offset and length for the relevant data instead forcing you to copy the relevant part to new arrays before using the methods reducing performance - I am for instance struggling with this in java.util.Base64 where the Encoders and Decoders lack a length parameter (also an offset would have been great even though I don't need that in my case). In my system I adds data of unknown length to large buffers that in many cases only will be partially filled and when I need to BASE64 encode data in these buffers I now have to create new "right sized" byte arrays and copy the relevant data from the larger buffer to the new byte array. This is a waste of CPU time & memory access that I would be happy to avoid. In this specific case of the Encoders there exists already internal methods that accepts a length for the source buffer but these are private rather than protected preventing me from simply creating a subclass that adds such methods. Making JDK libraries a bit more subclassing friendly would also be great :-) As it stands now I have created by own implementation of the Base64 class (from the JDK sources) that adds a method with the desired "length" parameter for use in my project but this is obviously not a good solution as I from now on needs to maintain this class and also misses out on improvements made to the original JDK library version... /Trist
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote:
> Translation drop for JDK22 RDP1
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets_ja.properties
line 111:
> 109: doclet.Factory=ファクトリ:
> 110: doclet.UnknownTag=不明なタグ。未登録のカスタム・タグ?
> 111: doclet.UnknownTagWithHint=不明なタグ。入力ミスによる@{0}または未登録のカスタム・タグ?
Simply appending a question mark may read a bit odd, although it is a
straightforward translation from English.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/resources/doclint_ja.properties
line 88:
> 86: dc.tag.start.unmatched = 終了タグがありません:
> 87: dc.unknown.javadoc.tag = 不明なタグ。未登録のカスタム・タグ?
> 88: dc.unknown.javadoc.tag.with.hint = 不明なタグ。入力ミスによる@{0}または未登録のカスタム・タグ?
same here
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17096#discussion_r1427170568
PR Review Comment: https://git.openjdk.org/jdk/pull/17096#discussion_r1427171135
Re: RFR: 8297632: InputStream.transferTo() method should specify what the return value should be when the number of bytes transfered is larger than Long.MAX_VALUE [v7]
On Thu, 14 Dec 2023 06:12:40 GMT, Vladimir Sitnikov wrote: > What do you think? I think that you are looking at an obsolete repository: https://github.com/openjdk/jdk/blob/c328f9589ddc3a981a2c63801bd991f8e593e69f/src/java.base/share/classes/java/io/InputStream.java#L802 - PR Review Comment: https://git.openjdk.org/jdk/pull/11403#discussion_r1427171307
Re: RFR: 8322027: One XMLStreamException constructor fails to initialize cause
On Wed, 13 Dec 2023 21:25:19 GMT, Archie Cobbs wrote: > After filing this PR, I had some second thoughts and added them to the > summary but by then it was too late for the `core-libs-dev` email, so I'll > repeat here in case anyone has some comments: > > > It's worth considering if there is any code out there that is working > > around this problem already by invoking `initCause()` manually. If so, that > > code would start throwing an `IllegalStateException` after this change. > > So a more conservative fix would be to just add another constructor taking > > the same arguments in a different order. But then again that's not much > > better than just saying "always use `initCause()` with the broken > > constructor", i.e., don't change anything. > > Hmm. Yeah, it may make sense to create a CSR to document the behavior change. My take is that the benefit outweighs the potential drawback. I've seen a case a few days ago where the use of this constructor caused the original cause to be lost. Attempting to work around the issue by invoking 'initCause()" won't necessarily be a good solution given other constructors can be used to avoid the issue. - PR Comment: https://git.openjdk.org/jdk/pull/17090#issuecomment-1856445263
Bug in GZIPInputStream.read() causing data loss
Hello. This is my first time posting here so I apologize if this is the wrong forum. I wanted to bring up an issue in the GZipInputStream which was first identified in 2011, confirmed as a bug, and then never resolved. When reading certain GZIP files from certain types of InputStreams the GZIPInputStream can misidentify the end of the stream and close early resulting in silently truncated data. You can see the bug report which has a detailed description here: https://bugs.openjdk.org/browse/JDK-7036144 In short it comes down to incorrect use of the (quite confusing) InputStream.available() method to detect the end of stream. This typically works fine with local files, but with network streams that might not have bytes available at any given moment it fails nondeterministically. How could I go about getting this fixed? I can contribute a patch or additional examples if necessary. Genomics data is typically encoded as block gzipped files, so this comes up regularly and causes a lot of confusion. The workaround is to just not use the GZIPInput stream. It seems like a core java class though so it would be nice if it worked. Thank you, Louis
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote: > Translation drop for JDK22 RDP1 src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/msg/XMLMessages_de.properties line 331: > 329: > 330: # Implementation Property DTD > 331: JDK_DTD_DENY = JAXP00010008: DOCTYPE ist nicht zulässig, wenn > die DTD-Eigenschaft auf "Ablehnen" gesetzt wurde. Weitere Informationen: > Eigenschaft jdk.xml.dtd.support in java.xml/module-summary. This version quoted "Ablehnen" while the other two (ja - "拒否" and zh_CN - "拒绝") didn't. If we want to be consistent, "deny" would be better since that's the literal value. The English version should have quoted "deny". The previous translations in these files have not been consistent. Some of the key words were translated. If we want to keep this translation as is, it's probably better to remove the quote in the "de" file. - PR Review Comment: https://git.openjdk.org/jdk/pull/17096#discussion_r1427152545
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
On Thu, 14 Dec 2023 18:03:00 GMT, Alan Bateman wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1042:
>
>> 1040: if (carrier != null) {
>> 1041: try {
>> 1042: notifyJvmtiDisableSuspend(true);
>
> this one too.
Thanks. All cases fixed now.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427095561
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:16:34 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/javaThread.hpp line 320: >> >>> 318: bool _is_in_VTMS_transition; // thread is >>> in virtual thread mount state transition >>> 319: bool _is_in_tmp_VTMS_transition; // thread is >>> in temporary virtual thread mount state transition >>> 320: bool _is_in_critical_section; // thread is >>> in a locking critical section >> >> might make sense to add a comment, that his variable Is changed/read only by >> current thread and no sync is needed. > > Good suggestion, thanks. Fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427099325
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:04:02 GMT, Alan Bateman wrote: > Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to > before the try in all cases, I've pointed out the cases that we missed. Thank you, Alan. Fixed now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856366484
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: moved notifyJvmtiDisableSuspend(true) out of try-block - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/4e5f6447..ad990422 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=03-04 Stats: 10 lines in 1 file changed: 5 ins; 5 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
On Thu, 14 Dec 2023 17:30:54 GMT, Serguei Spitsyn wrote:
>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1
>> time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical
>> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`,
>> `getAndClearInterrupt()`, `threadState()`, `toString()`),
>> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report
>> comment.
>> In simple words, a virtual thread should not be suspended during
>> 'interruptLock' critical sections.
>>
>> The fix is to record that a virtual thread is in a critical section
>> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about
>> begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any
>> `HandshakeOperation` if a target `JavaThread` is in a critical section.
>>
>> Some of new notifications with `notifyJvmtiSync()` method is on a
>> performance critical path. It is why this method has been intrincified.
>>
>> New test was developed by Patricio:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without
>> the fix.
>> The test is never failing with this fix.
>>
>> Testing:
>> - tested with newly added test:
>> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request incrementally with one
> additional commit since the last revision:
>
> review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
src/java.base/share/classes/java/lang/VirtualThread.java line 746:
> 744: } else if ((s == PINNED) || (s == TIMED_PINNED)) {
> 745: try {
> 746: notifyJvmtiDisableSuspend(true);
Move to before the try.
src/java.base/share/classes/java/lang/VirtualThread.java line 853:
> 851: checkAccess();
> 852: try {
> 853: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 886:
> 884: if (oldValue) {
> 885: try {
> 886: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 917:
> 915: case RUNNING:
> 916: try {
> 917: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 1042:
> 1040: if (carrier != null) {
> 1041: try {
> 1042: notifyJvmtiDisableSuspend(true);
this one too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080057
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080394
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080484
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080704
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080811
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:19:43 GMT, Alan Bateman wrote: > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to before the try in all cases, I've pointed out the cases that we missed. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856339508
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 17:06:05 GMT, Alan Bateman wrote: >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > > Are you planning to drop the changes to mount/unmount too? They shouldn't be > needed. > > notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called > before the try, not in the block. We have changes coming that will require > moving these hooks to critical section enter/exit methods, so the naming will > be less important then. Yes, I've dropped changes in the mount/unmount methods. I've already done renaming to `notifyJvmtiDisableSuspend(boolean)`. Let's see if it is okay with you. It is not a problem to rename it back to `notifyJvmtiCriticalLock(boolean)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427032721
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v4]
> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 > time frame. > It is fixing a deadlock issue between `VirtualThread` class critical sections > with the `interruptLock` (in methods: `unpark()`, `interrupt()`, > `getAndClearInterrupt()`, `threadState()`, `toString()`), > `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. > The deadlocking scenario is well described by Patricio in a bug report > comment. > In simple words, a virtual thread should not be suspended during > 'interruptLock' critical sections. > > The fix is to record that a virtual thread is in a critical section > (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about > begin/end of critical section. > This bit is used in `HandshakeState::get_op_for_self()` to filter out any > `HandshakeOperation` if a target `JavaThread` is in a critical section. > > Some of new notifications with `notifyJvmtiSync()` method is on a performance > critical path. It is why this method has been intrincified. > > New test was developed by Patricio: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > The test is very nice as it reliably in 100% reproduces the deadlock without > the fix. > The test is never failing with this fix. > > Testing: > - tested with newly added test: > `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` > - tested with mach5 tiers 1-6 Serguei Spitsyn has updated the pull request incrementally with one additional commit since the last revision: review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/18f1752e..4e5f6447 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=02-03 Stats: 68 lines in 14 files changed: 9 ins; 10 del; 49 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 16:57:25 GMT, Serguei Spitsyn wrote: > Implemented this renaming suggestion. Let's wait if Alan ia okay with it. Are you planning to drop the changes to mount/unmount too? They shouldn't be needed. notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called before the try, not in the block. We have changes coming that will require moving these hooks to critical section enter/exit methods, so the naming will be less important then. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427000950
Re: RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect [v2]
> Small document correction on a return description. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Corrected @param description - Changes: - all: https://git.openjdk.org/jdk/pull/17098/files - new: https://git.openjdk.org/jdk/pull/17098/files/1025c8a0..248fc643 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17098&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17098&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17098/head:pull/17098 PR: https://git.openjdk.org/jdk/pull/17098
Re: RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect [v2]
On Thu, 14 Dec 2023 09:32:23 GMT, Jaikiran Pai wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Corrected @param description
>
> src/java.base/share/classes/java/time/zone/ZoneRules.java line 831:
>
>> 829: * and {@link #getStandardOffset(java.time.Instant) standard}
>> offsets.
>> 830: *
>> 831: * @param instant the instant to find the offset information for,
>> not null, but null
>
> Hello Naoto, do you think this `@param` description too will have to be
> reworded? It seems to be a copy/paste of the `getDaylightSavings()` method.
Hi Jai, looks like it was copied/pasted from `getStandardOffset()`, but you are
right, the description needs to be corrected.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17098#discussion_r1426999528
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:11:42 GMT, Serguei Spitsyn wrote: >> src/java.base/share/classes/java/lang/VirtualThread.java line 1164: >> >>> 1162: >>> 1163: @IntrinsicCandidate >>> 1164: private native void notifyJvmtiCriticalLock(boolean enter); >> >> The name is confusing to me, the CriticalLock looks like it is the section >> is critical and might be taken by a single thread only. Or it's just unclear >> what is critical here. >> However, the purpose is to disable suspend >> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name >> here? >> or comment what critical means here. > > Okay, thanks. I like your name suggestion but let's check with Alan first. Implemented this renaming suggestion. Let's wait if Alan ia okay with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426990736
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
On Thu, Dec 14, 2023 at 4:56 AM Viktor Klang
wrote:
> I presume that the precondition to have the original collection be
> pre-ordered according to the supplied Comparator can be verified by
> checking before adding each element in the collection to the PQ that it
> compareTo equal-or-greater to the previous one?
>
Hmm... something is not adding up.
Either (a) there is a precondition that the collection already be sorted or
(b) there's no such precondition.
In case (a):
- Why isn't the new constructor invoking initElementsFromCollection()
instead of initFromCollection()?
- The precondition is not very obvious from the Javadoc description, and
moreover what happens when the precondition is not met is not documented at
all.
In case (b):
- The Javadoc is misleading because it (ambiguously) implies there is a
precondition with the wording "collection that orders its elements
according to the specified comparator" (the referent of "that" is ambiguous
- does it refer to the collection or the PriorityQueue?)
>From the PR description it seems clear that there is no such precondition.
So maybe the Javadoc should say this instead:
Creates a {@code PriorityQueue} containing the elements in the specified
> collection. The {@code PriorityQueue} will order its elements according to
> the specified comparator.
>
-Archie
--
Archie L. Cobbs
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v7]
On Thu, 14 Dec 2023 14:55:08 GMT, Eirik Bjorsnos wrote: >> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and >> `ZipFile/input.jar`. >> >> Binary test vectors are harder to analyze, and sharing test vectors across >> unrelated tests increases maintenance burden. It would be better to have >> each test produce its own test vectors independently. >> >> While visiting these dusty tests, we should take the opportunity to convert >> them to JUnit, add more comments and perform some mild modernization and >> cleanups where appropriate. We should also consider whether any test are >> duplicated and can be retired, see comments below. >> >> To help reviewers, here are some comments on the updated tests: >> >> `Available.java` >> This test currently has no jtreg `@test` header, so isn't currently active. >> After discussion, we decided to merge this test into `ReadZip.java`. I added >> some checks to verify that reading from the stream reduces the number of >> available bytes accordingly, also checking the behavior when the stream is >> closed. >> >> `CopyJar.java` >> The concern of copying entries seems to now have better coverage in the test >> `zip/CopyZipFile`. We decided to retire this test rather than convert it and >> instead convert `CopyZipFile` to JUnit. >> >> `EnumAfterClose.java` >> To prevent confusion with Java Enums, I suggest we rename this test to >> `EnumerateAfterClose`. >> >> `FinalizeInflater.java` >> The code verified by this test has been updated to use cleaners instead of >> finalizers. Still, this test code relies on finalizers. Not sure if this is >> an issue, but this test will need to be updated when finalizers are finally >> removed. >> >> `GetDirEntry.java` >> We decided to merge this test into `ReadZip.readDirectoryEntries` rather >> than keeping it as a separate test. >> >> `ReadZip.java` >> Nothing exciting here, the single main method was split into multiple JUnit >> methods, each focusing on a separate concern. >> >> `ReleaseInflater.java` >> Nothing exciting, tried to add some comment to help understanding of what is >> tested. >> >> `StreamZipEntriesTest.java` >> This test was using TestNG so was converted to JUnit for consistency. Added >> some comments to help understanding. >> >> `ReadAfterClose.java` >> This uses the binary test vector `crash.jar`. The test is converted to JUnit >> and moved to `ReadZip.readAfterClose´. The test is expanded to exercise more >> read methods. > > Eirik Bjorsnos has updated the pull request incrementally with one additional > commit since the last revision: > > Merge the ReadAfterClose test into ReadZip, converting it to JUnit. Seeing that `ReadAfterClose.java` uses a binary test vector `crash.jar`, I think it makes sense to include it in this PR, convert it to JUnit and move it into `ReadZip.readAfterClose`. This removes the last remaining binary test vector ZIP in the `ZipFile/` directory. - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1856008970
Re: [jdk22] RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 17:38:27 GMT, Jorn Vernee wrote: > Hi all, > > This pull request contains a backport of commit > [7ece9e90](https://github.com/openjdk/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jorn Vernee on 13 Dec 2023 and > was reviewed by Maurizio Cimadamore. > > This is a test-only P4 change. So it's allowed under the release process in > RDP1: https://openjdk.org/jeps/3 > > Thanks! (already approved in 23) - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk22/pull/11#pullrequestreview-1782001700
[jdk22] Integrated: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 17:38:27 GMT, Jorn Vernee wrote: > Hi all, > > This pull request contains a backport of commit > [7ece9e90](https://github.com/openjdk/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jorn Vernee on 13 Dec 2023 and > was reviewed by Maurizio Cimadamore. > > This is a test-only P4 change. So it's allowed under the release process in > RDP1: https://openjdk.org/jeps/3 > > Thanks! This pull request has now been integrated. Changeset: d62249a3 Author:Jorn Vernee URL: https://git.openjdk.org/jdk22/commit/d62249a3abec28be0b3b9797f899adbdfd941cbe Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion Reviewed-by: mcimadamore Backport-of: 7ece9e90c0198f92cdf8d620e346c4a9832724cd - PR: https://git.openjdk.org/jdk22/pull/11
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v7]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it and > instead convert `CopyZipFile` to JUnit. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Merge the ReadAfterClose test into ReadZip, converting it to JUnit. - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/8623effd..27fe1131 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=05-06 Stats: 84 lines in 3 files changed: 33 ins; 51 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Thu, 14 Dec 2023 12:03:11 GMT, Aleksei Voitylov wrote: > Thank you, Roger. Yes, I'll work on the 22 backport. See https://wiki.openjdk.org/display/SKARA/Backports Once the mainline is integrated, find the commit and add a "/backport jdk21" comment. Skara should do the rest (but read the directions also) Thanks - PR Comment: https://git.openjdk.org/jdk/pull/17057#issuecomment-1855969887
Integrated: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled
On Mon, 11 Dec 2023 13:48:18 GMT, Aleksei Voitylov wrote: > Since JDK-8311906, if CompactStrings is not enabled, index is not considered > when calling extractCodepoints from StringUTF16.toBytes(). Because of that > the last elements of the source codepoints array are stripped from the > resulting UTF16 string, which fires in other places (e.g. during RegEx > processing). > > The fix replaces len in extractCodepoints parameters with end that is index + > len. This pull request has now been integrated. Changeset: fde5b168 Author:Aleksei Voitylov Committer: Roger Riggs URL: https://git.openjdk.org/jdk/commit/fde5b16817c3263236993f2e8c2d2469610d99bd Stats: 27 lines in 2 files changed: 25 ins; 0 del; 2 mod 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled Co-authored-by: Roger Riggs Reviewed-by: rriggs - PR: https://git.openjdk.org/jdk/pull/17057
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v6]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. > After discussion, we decided to merge this test into `ReadZip.java`. I added > some checks to verify that reading from the stream reduces the number of > available bytes accordingly, also checking the behavior when the stream is > closed. > > `CopyJar.java` > The concern of copying entries seems to now have better coverage in the test > `zip/CopyZipFile`. We decided to retire this test rather than convert it. > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > We decided to merge this test into `ReadZip.readDirectoryEntries` rather than > keeping it as a separate test. > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was using TestNG so was converted to JUnit for consistency. Added > some comments to help understanding. Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Improve comment explaining what happens when ZipEntry.setCompressedSize is calledExplicitly - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/4e9c97d4..8623effd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=04-05 Stats: 3 lines in 1 file changed: 2 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v5]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Convert CopyZipFile.java to JUnit - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/593a76cd..4e9c97d4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=03-04 Stats: 195 lines in 1 file changed: 82 ins; 19 del; 94 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen wrote: > (though CopyZipFile could use a junit conversion ;-) Agreed, I have included that conversion in this PR :-) This test can make good use of `assertThrows` and splitting different concerns into separate test methods. - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855914377
Omission in javadoc Stream.toArray(): Safe array contract stipulation
Hi core libs team,
I think I found a rather inconsequential and esoteric bug, though the term
is somewhat less objectively defined when the problem exists solely in how
complete some method’s javadoc is.
Two questions: Is there a plausible argument that the javadoc as is,
shouldn’t be updated? If not, what’s the right place to report this javadoc
‘bug’?
The issue:
A snippet of the javadoc of Collection.toArray, from current HEAD jdk22u:
The returned array will be "safe" in that no references to it are
* maintained by this collection. (In other words, this method must
* allocate a new array even if this collection is backed by an array).
* The caller is thus free to modify the returned array.
The javadoc of Stream.toArray, from current HEAD jdk22u - has no such rider
anywhere in its documentation nor on the javadoc of the Stream interface,
nor on the package javadoc. The javadoc is quite short; the complete docs
on toArray():
/**
* Returns an array containing the elements of this stream.
*
* This is a terminal
* operation.
*
* @return an array, whose {@linkplain Class#getComponentType runtime
component
* type} is {@code Object}, containing the elements of this stream
*/
The more usually used variant taking a function that makes the array has
slightly longer javadoc, but it similarly makes no mention whatsoever about
the contract stipulation that any implementors must not keep a reference.
A snippet from Stuart Marks on a stack overflow question about a to the
asker weird choice about stream’s toList()’s default implementation (
https://stackoverflow.com/questions/77473755/is-it-necessary-to-copy-a-list-to-be-safe/77474199?noredirect=1#comment136909551_77474199
):
@Holger The extra step in the default implementation is there to force
a defensive copy if this.toArray were to violate its spec and keep a
reference to the returned array. Without the defensive copy, it would be
possible to modify the list returned from the default toList implementation.
Which leads to the obvious question: Where is that ’spec’ that Stuart is
referring to? Either the javadoc is the spec and therefore the javadoc is
buggy, in that it fails to mention this stipulation, or the spec is
elsewhere, in which case surely the javadoc should link to it or copy it.
Possibly this is jus filed away as: “Unlike with Collection, Stream
instances are disposable; after a terminal operation (and toArray is
terminal) has been invoked on it, that stream object has ceased being
useful. Therefore, there is no perceived value to caching any created
array, therefore, it doesn’t need mentioning".
Except, as per Stuart’s comment, actual OpenJDK code is written partly to
deal with any violators of this invisible spec, and the discrepancy (where
Collection.toArray explicitly mentions the contract stipulation that
toArray() must make safe arrays, but Stream’s toArray() does not) suggests
a fundamental difference where none exists (in fact, literally: Apparently
its a spec violation if your Stream implementation return a non-safe array
from toArray!)
--Reinier Zwitserloot
Integrated: 8319647: Few java/lang/System/LoggerFinder/modules tests ignore vm flags
On Mon, 4 Dec 2023 10:39:05 GMT, Darragh Clarke wrote: > Updated tests to use vm.flagless as they already ignore Vm flags This pull request has now been integrated. Changeset: 62b7c5ea Author:Darragh Clarke URL: https://git.openjdk.org/jdk/commit/62b7c5eaed1e6ffd6f2c8371eb4cf01dd9d53a06 Stats: 14 lines in 7 files changed: 7 ins; 0 del; 7 mod 8319647: Few java/lang/System/LoggerFinder/modules tests ignore vm flags Reviewed-by: lmesnik - PR: https://git.openjdk.org/jdk/pull/16946
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23 [v2]
> Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > > Thanks David Holmes has updated the pull request incrementally with one additional commit since the last revision: Revert "8309981: Remove expired flags in JDK 23" This reverts commit 0324a90e936ae01e42ae099e7235156326cc318a. - Changes: - all: https://git.openjdk.org/jdk/pull/17101/files - new: https://git.openjdk.org/jdk/pull/17101/files/65a8c9ed..8b052141 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17101&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17101&range=00-01 Stats: 23 lines in 2 files changed: 10 ins; 11 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17101.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17101/head:pull/17101 PR: https://git.openjdk.org/jdk/pull/17101
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 09:01:17 GMT, Alan Bateman wrote: > Initially I wondered if JDK-8309981 should be separated but include keeps > things in sync so I think okay. Thanks for the review @AlanBateman . Yeah I was in two minds there myself. I started fixing [JDK-8309981](https://bugs.openjdk.org/browse/JDK-8309981) only to discover that the start of release updates had not been done as part of the start of release, so I figured I may as well fix it all together given I'd generated all the updated files anyway. But I'm still a little unsure ... in fact I think I will remove it in the morning. - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855761906
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v4]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Remove trailing whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/03cb8354..593a76cd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 09:17:05 GMT, Pavel Rappo wrote: > Thanks for doing this, David. I only note that the changes for JDK-8321384 > were published in [JDK-8308715](https://bugs.openjdk.org/browse/JDK-8308715), > which was integrated in the mainline before JDK 22 RDP 1. So they are already > present in the mainline. Ah I see. Thanks for correcting that, I will update the PR and JBS issue. And thanks for looking at this @pavelrappo . - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855755042
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 11:39:22 GMT, Lance Andersen wrote: >> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> - `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? >> - `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? >> - `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > >> The scope of this PR has now expanded to removing uses of the `input.zip` >> and `input.jar` files, updating any test using them to produce their own >> test vectors, and convert affected tests to JUnit. >> >> I'm marking the PR ready for review again. Before looking too closely at the >> code, it would be useful to discuss the following tests: >> >> * `Available.java`: This test has no jtreg header. I've added one and >> converted the test. Is this worthwhile, or should we rather remove it? > > This could be moved into ReadZip. I do not believe we have a specific test > and it is trivial > >> * `CopyJar.java`: The concern tested seems to have superior coverage in the >> test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of >> coverting it? > > Yes CopyZipFile already exercises Zipfile.ZipInputStream so it is safe to > retire CopyJar (though CopyZipFile could use a junit conversion ;-) >> * `DirEntry.java`: There is duplication between this test and >> `ReadZip.readDirectoryEntry()`. Should we retire one of these? > > I believe you meant GetDirEntry.java not DirEntry.java? > > Having a test that specifically validates we can read META-INF is not a bad > thing, but I suspect we have a test that already does that if not in the > java/util/zip tests or java/util/jar tests. If not we should keep it but > merge it as you suggest @LanceAndersen Thanks for your guidance! I moved `Available` into `ReadZip`, deleted `CopyJar` and merged `GetDirEntry` into ReadZip.readDirectoryEntries (adding a 'META-INF/' directory just in case) - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855753641
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v3]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos has updated the pull request incrementally with three additional commits since the last revision: - Merge GetDirEntry.java into ReadZip.readDirectoryEntries() - Retire redundant test ZipFile/CopyJar - Merge Available.java into ReadZip.java - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/56f55d89..03cb8354 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=01-02 Stats: 361 lines in 4 files changed: 59 ins; 296 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:06:41 GMT, Serguei Spitsyn wrote: > Carrier thread also can be suspended when executing the "critical code". Why > do you think it can't be a problem? Do you think the deadlocking scenario > described in the bug report is not possible? It's a different scenario. When mounting, the coordination of the interrupt status is done before the thread identity is changed. Similarly, when unmounting, the coordination is done after reverting the thread identity to the carrier. So if there is an agent randomly suspending threads when it shouldn't be an issue here. > > toggle_is_in_critical_section needs to detect reentrancy, it is otherwise > > too easy to refactor the Java code, e.g. call threadState while holding > > the interrupt lock. > > Is your concern a recursive `interruptLock` enter? I was also thinking if > this scenario is possible, so a counter can be used instead of boolean. Minimally an assert. A counter might be needed later. > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? We have changes in the works that require pinning during some critical sections so I think I prefer to use that terminology. We can move the notification to JVMTI to the enter/leave methods. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855748841
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:54:43 GMT, Leonid Mesnik wrote: >> Serguei Spitsyn has updated the pull request incrementally with one >> additional commit since the last revision: >> >> review: (1) rename notifyJvmti method; (2) add try-final statements to >> VirtualThread methods > > src/hotspot/share/prims/jvm.cpp line 4013: > >> 4011: // Notification from VirtualThread about entering/exiting sync >> critical section. >> 4012: // Needed to avoid deadlocks with JVMTI suspend mechanism. >> 4013: JVM_ENTRY(void, JVM_VirtualThreadCriticalLock(JNIEnv* env, jobject >> vthread, jboolean enter)) > > the jobject vthread is not used. Can't be the method made static to reduce > the number of arguments? > It is the performance-critical code, I don't know if it is optimized by C2. Good question. In general, I'd like to keep this unified with the other `notiftJvmti` methods. Let me double check how it fits together. Also, I'm not sure how is going to impact the intrinsification. > src/hotspot/share/runtime/javaThread.hpp line 320: > >> 318: bool _is_in_VTMS_transition; // thread is >> in virtual thread mount state transition >> 319: bool _is_in_tmp_VTMS_transition; // thread is >> in temporary virtual thread mount state transition >> 320: bool _is_in_critical_section; // thread is >> in a locking critical section > > might make sense to add a comment, that his variable Is changed/read only by > current thread and no sync is needed. Good suggestion, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643218 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643663
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 GMT, Serguei Spitsyn wrote: >> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 >> time frame. >> It is fixing a deadlock issue between `VirtualThread` class critical >> sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, >> `getAndClearInterrupt()`, `threadState()`, `toString()`), >> `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms. >> The deadlocking scenario is well described by Patricio in a bug report >> comment. >> In simple words, a virtual thread should not be suspended during >> 'interruptLock' critical sections. >> >> The fix is to record that a virtual thread is in a critical section >> (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about >> begin/end of critical section. >> This bit is used in `HandshakeState::get_op_for_self()` to filter out any >> `HandshakeOperation` if a target `JavaThread` is in a critical section. >> >> Some of new notifications with `notifyJvmtiSync()` method is on a >> performance critical path. It is why this method has been intrincified. >> >> New test was developed by Patricio: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> The test is very nice as it reliably in 100% reproduces the deadlock without >> the fix. >> The test is never failing with this fix. >> >> Testing: >> - tested with newly added test: >> `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock` >> - tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: (1) rename notifyJvmti method; (2) add try-final statements to > VirtualThread methods @AlanBateman Thank you for reviewing an the comment. > It shouldn't be necessary to touch mount/unmount as the thread identity is > the carrier, not the virtual thread, when executing the "critical code". Carrier thread also can be suspended when executing the "critical code". Why do you think it can't be a problem? Do you think the deadlocking scenario described in the bug report is not possible? > toggle_is_in_critical_section needs to detect reentrancy, it is otherwise > too easy to refactor the Java code, e.g. call threadState while holding the > interrupt lock. Is your concern a recursive `interruptLock` enter? I was also thinking if this scenario is possible, so a counter can be used instead of boolean. > All the use-sides will need to use try-finally to more reliably revert the > critical section flag when rewinding. Right, thanks. It is already done. > The naming is very problematic, we'll need to replace with methods that are > clearly named enter and exit critical section. Ongoing work in this area to > support monitors has to introduce some temporary pinning so there will be > enter/exitCriticalSection methods, that's a better place for the JVMTI hooks. Okay. What about the Leonid's suggestion to name it `notifyJvmtiDisableSuspend()` ? - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855730274
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: (1) rename notifyJvmti method; (2) add try-final statements to
>> VirtualThread methods
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1164:
>
>> 1162:
>> 1163: @IntrinsicCandidate
>> 1164: private native void notifyJvmtiCriticalLock(boolean enter);
>
> The name is confusing to me, the CriticalLock looks like it is the section is
> critical and might be taken by a single thread only. Or it's just unclear
> what is critical here.
> However, the purpose is to disable suspend
> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name
> here?
> or comment what critical means here.
Okay, thanks. I like your name suggestion but let's check with Alan first.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 30:
>
>> 28: * @requires vm.continuations
>> 29: * @library /testlibrary
>> 30: * @run main/othervm -Xint SuspendWithInterruptLock
>
> Doesn't it make sense to add a testcase without -Xint also? Just to give
> stress testing with compilation.
Thanks. I was also thinking about this. Will add a sub-test without -Xint.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 36:
>
>> 34:
>> 35: public class SuspendWithInterruptLock {
>> 36: static boolean done;
>
> done is accessed from different threads, should be volatile.
Good suggestion, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 54:
>
>> 52: Thread.yield();
>> 53: }
>> 54: done = true;
>
> I think it is better to use done to stop all threads and set it to true in
> the main thread after some time. So you could be sure that the yielder hadn't
> been completed before the suspender started. But it is just proposal.
Thank you. Will consider this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426638981
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426635613
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426636196
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426637200
Re: RFR: 8321514: UTF16 string gets constructed incorrectly from codepoints if CompactStrings is not enabled [v3]
On Wed, 13 Dec 2023 11:39:19 GMT, Aleksei Voitylov wrote: >> Since JDK-8311906, if CompactStrings is not enabled, index is not considered >> when calling extractCodepoints from StringUTF16.toBytes(). Because of that >> the last elements of the source codepoints array are stripped from the >> resulting UTF16 string, which fires in other places (e.g. during RegEx >> processing). >> >> The fix replaces len in extractCodepoints parameters with end that is index >> + len. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > review comments Thank you, Roger. Yes, I'll work on the 22 backport. - PR Comment: https://git.openjdk.org/jdk/pull/17057#issuecomment-1855725539
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Thu, 14 Dec 2023 10:13:31 GMT, Eirik Bjorsnos wrote: > The scope of this PR has now expanded to removing uses of the `input.zip` and > `input.jar` files, updating any test using them to produce their own test > vectors, and convert affected tests to JUnit. > > I'm marking the PR ready for review again. Before looking too closely at the > code, it would be useful to discuss the following tests: > > * `Available.java`: This test has no jtreg header. I've added one and > converted the test. Is this worthwhile, or should we rather remove it? This could be moved into ReadZip. I do not believe we have a specific test and it is trivial > * `CopyJar.java`: The concern tested seems to have superior coverage in the > test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of > coverting it? Yes CopyZipFile already exercises Zipfile.ZipInputStream so it is safe to retire CopyJar (though CopyZipFile could use a junit conversion ;-) > * `DirEntry.java`: There is duplication between this test and > `ReadZip.readDirectoryEntry()`. Should we retire one of these? I believe you meant GetDirEntry.java not DirEntry.java? Having a test that specifically validates we can read META-INF is not a bad thing, but I suspect we have a test that already does that if not in the java/util/zip tests or java/util/jar tests. If not we should keep it but merge it as you suggest - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855690222
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update
On Wed, 13 Dec 2023 22:12:48 GMT, Alisen Chung wrote:
> Translation drop for JDK22 RDP1
Marked as reviewed by cstein (Committer).
German translations read OK.
Nit: some layouts get lost in translated files - it would be great to have
similar usage of `\n` and `` in the translated files.
For example, the English file
`src/java.base/share/classes/sun/launcher/resources/launcher.properties` has
good (easy to compare) layout:
java.launcher.opt.header = Usage: {0} [options] [args...]\n\
\ (to execute a class)\n\
\ or {0} [options] -jar [args...]\n\
\ (to execute a jar file)\n\
\ or {0} [options] -m [/] [args...]\n\
\ {0} [options] --module [/] [args...]\n\
\ (to execute the main class in a module)\n\
\ or {0} [options] [args]\n\
\ (to execute a source-file program)\n\n\
[...]
The German translation in
`src/java.base/share/classes/sun/launcher/resources/launcher_de.properties`
uses only a single line in its properties file:
java.launcher.opt.header = Verwendung: {0} [Optionen]
[args...]\n (zur Ausführung einer Klasse)\n oder {0} [Optionen]
-jar [args...]\n (zur Ausführung einer JAR-Datei)\n
oder {0} [Optionen] -m [/] [args...]\n {0}
[Optionen] --module [/] [args...]\n(zur
Ausführung der Hauptklasse in einem Modul)\n oder {0} [Optionen]
[args]\n (zur Ausführung eines Programms mit einer
Quelldatei)\n\n[...]
Changes in the latter are not easy to spot and review.
-
PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1781603832
PR Comment: https://git.openjdk.org/jdk/pull/17096#issuecomment-1855667603
Re: RFR: 8303374: Compiler Implementation for Primitive types in patterns, instanceof, and switch (Preview) [v38]
> This is the proposed patch for Primitive types in patterns, instanceof, and > switch (Preview). > > Draft spec here: https://cr.openjdk.org/~abimpoudis/instanceof/latest/ Aggelos Biboudis has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 54 commits: - Merge branch 'master' into primitive-patterns - Cleanup - Merge branch 'master' into primitive-patterns - Remove trailing spaces - Merge branch 'primitive-patterns-and-generating-dispatch' into primitive-patterns - Fixed switch in the cases of unboxing and widening - Merge branch 'JDK-8319220' into primitive-patterns - Merge branch 'master' into JDK-8319220 - reflecting review comment: fixing letter case. - Reflecting review feedback. - ... and 44 more: https://git.openjdk.org/jdk/compare/d2ba3b1e...a03fea7c - Changes: https://git.openjdk.org/jdk/pull/15638/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15638&range=37 Stats: 3248 lines in 41 files changed: 2985 ins; 110 del; 153 mod Patch: https://git.openjdk.org/jdk/pull/15638.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15638/head:pull/15638 PR: https://git.openjdk.org/jdk/pull/15638
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
I presume that the precondition to have the original collection be pre-ordered according to the supplied Comparator can be verified by checking before adding each element in the collection to the PQ that it compareTo equal-or-greater to the previous one? Cheers, √ Viktor Klang Software Architect, Java Platform Group Oracle From: core-libs-dev on behalf of Pavel Rappo Sent: Thursday, 14 December 2023 10:55 To: Valeh Hajiyev Cc: core-libs-dev@openjdk.org Subject: Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator You might want to use this older, unresolved, duplicated bug for your PR: https://bugs.openjdk.org/browse/JDK-6356745 > On 13 Dec 2023, at 23:37, Valeh Hajiyev wrote: > > Hi Pavel, > > Thanks for the reply. If I understand correctly, I need this change to be > discussed in one of the mailing lists there, so that someone would sponsor me > to create a tracking issue in JBS. Do you know which mailing list is the most > relevant for me to propose the change? > > Thanks, > Valeh > > On Thu, Dec 14, 2023 at 12:26 AM Pavel Rappo wrote: > Sorry, there's a necessary process that a PR must follow. You seem to have > signed OCA already. For the rest, see this resource: > https://openjdk.org/guide/. In particular, this part: > https://openjdk.org/guide/#contributing-to-an-openjdk-project. > > -Pavel > > > On 13 Dec 2023, at 23:09, Valeh Hajiyev wrote: > > > > Hi all, > > > > I have raised the following PR, could someone please help me to get it > > merged? > > > > https://github.com/openjdk/jdk/pull/17045 > > > > More details: > > > > This commit addresses the current limitation in the `PriorityQueue` > > implementation, which lacks a constructor to efficiently create a priority > > queue with a custom comparator and an existing collection. In order to > > create such a queue, we currently need to initialize a new queue with > > custom comparator, and after that populate the queue using `addAll()` > > method, which in the background calls `add()` method (which takes `O(logn)` > > time) for each element of the collection (`n` times). This is resulting in > > an overall time complexity of `O(nlogn)`. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(customComparator); > > pq.addAll(existingCollection); > > ``` > > > > The pull request introduces a new constructor to streamline this process > > and reduce the time complexity to `O(n)`. If you create the queue above > > using the new constructor, the contents of the collection will be copied > > (which takes `O(n)` time) and then later `heapify()` operation (Floyd's > > algorithm) will be called once (another `O(n)` time). Overall the operation > > will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > > customComparator); > > ``` > > > > Best regards, > > Valeh Hajiyev > > >
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip [v2]
> This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... Eirik Bjorsnos 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 six additional commits since the last revision: - Add @bug reference 4401122 on the Available.java test - Merge branch 'master' into readzip-junit - The input.jar test vector included a META-INF/ directory before the manifest, lets keep it that way - Remove the use of binary test vector ZIP/JAR files input.zip and input.jar, converting affected tests to JUnit - Update copyright year - Convert ReadZip to JUnit - Changes: - all: https://git.openjdk.org/jdk/pull/17038/files - new: https://git.openjdk.org/jdk/pull/17038/files/f6a00f0c..56f55d89 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17038&range=00-01 Stats: 5608 lines in 92 files changed: 3340 ins; 1807 del; 461 mod Patch: https://git.openjdk.org/jdk/pull/17038.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17038/head:pull/17038 PR: https://git.openjdk.org/jdk/pull/17038
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v9]
> Please review this PR to use modern APIs and language features to simplify > equals, hashCode, and compareTo for BigInteger. If you have any performance > concerns, please raise them. > > This PR is cherry-picked from a bigger, not-yet-published PR, to test the > waters. That latter PR will be published soon. Pavel Rappo 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 16 additional commits since the last revision: - Merge branch 'master' into 8310813 - Merge branch 'master' into 8310813 - Merge branch 'master' into 8310813 - Fix bugs in Shared.createSingle - Merge branch 'master' into 8310813 - Group params coarser (suggested by @cl4es) - Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. Every testXYZ method invokes M operations, where M is the maximum number of elements in a group. Shorter groups are cyclically padded. - Uses the org.openjdk.jmh.infra.Blackhole API and increases benchmark time. - Fixes a bug in Shared that precluded 0 from being in a pair. - Use better overloads (suggested by @cl4es) - Uses simpler, more suitable overloads for the subrange starting from 0 - Improve benchmarks - Merge branch 'master' into 8310813 - Restore hash code values BigInteger is old and ubiquitous enough so that there might be inadvertent dependencies on its hash code. This commit also includes a test, to make sure hash code is unchanged. - ... and 6 more: https://git.openjdk.org/jdk/compare/5f6cebff...ef8b0c46 - Changes: - all: https://git.openjdk.org/jdk/pull/14630/files - new: https://git.openjdk.org/jdk/pull/14630/files/155fedba..ef8b0c46 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14630&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14630&range=07-08 Stats: 782591 lines in 3816 files changed: 173277 ins; 536256 del; 73058 mod Patch: https://git.openjdk.org/jdk/pull/14630.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14630/head:pull/14630 PR: https://git.openjdk.org/jdk/pull/14630
Re: RFR: 8321616: Retire binary test vectors ZipFile/input.jar and ZipFile/input.zip
On Fri, 8 Dec 2023 20:28:20 GMT, Eirik Bjorsnos wrote: > This PR suggests we retire the binary test vectors `ZipFile/input.zip` and > `ZipFile/input.jar`. > > Binary test vectors are harder to analyze, and sharing test vectors across > unrelated tests increases maintenance burden. It would be better to have each > test produce its own test vectors independently. > > While visiting these dusty tests, we should take the opportunity to convert > them to JUnit, add more comments and perform some mild modernization and > cleanups where appropriate. We should also consider whether any test are > duplicated and can be retired, see comments below. > > To help reviewers, here are some comments on the updated tests: > > `Available.java` > This test currently has no jtreg `@test` header, so isn't currently active. I > added a jtreg header with `@bug 4401122`. I added some checks to verify that > reading from the stream reduces the number of available bytes accordingly, > also checking the behavior when the stream is closed. An alternative action > would be to remove this test. It seems to validate the current implementation > more than the specification. > > `CopyJar.java` > The 1999 bug description JDK-4239446 is short and somewhat confusing. It > seems at some point that the CRC of `ZipEntry` read by `ZipFile.getEntry` > would be read from a LOC header instead of the CEN header, which means it > could be zero for streaming entries with data descriptors. (However, the bug > description also mentions calling `getNextEntry`, which is a `ZipInputStream` > method?) In any case, this concern seems to now have better coverage in the > test `zip/CopyZipFile` from 2020, so it would perhaps be desirable to simply > remove `CopyJar.java` to reduce duplication? > > `EnumAfterClose.java` > To prevent confusion with Java Enums, I suggest we rename this test to > `EnumerateAfterClose`. > > `FinalizeInflater.java` > The code verified by this test has been updated to use cleaners instead of > finalizers. Still, this test code relies on finalizers. Not sure if this is > an issue, but this test will need to be updated when finalizers are finally > removed. > > `GetDirEntry.java` > This test seems to duplicate the test `ReadZip.readDirectoryEntry`. It would > perhaps be better to remove `GetDirEntry.java` to reduce duplication? > > `ReadZip.java` > Nothing exciting here, the single main method was split into multiple JUnit > methods, each focusing on a separate concern. > > `ReleaseInflater.java` > Nothing exciting, tried to add some comment to help understanding of what is > tested. > > `StreamZipEntriesTest.java` > This test was us... The scope of this PR has now expanded to removing uses of the `input.zip` and `input.jar` files, updating any test using them to produce their own test vectors, and convert affected tests to JUnit. I'm marking the PR ready for review again. Before looking too closely at the code, it would be useful to discuss the following tests: - `Available.java`: This test has no jtreg header. I've added one and converted the test. Is this worthwhile, or should we rather remove it? - `CopyJar.java`: The concern tested seems to have superior coverage in the test `zip/CopyZipFile.java`. Should we retire `CopyJar.java` instead of coverting it? - `DirEntry.java`: There is duplication between this test and `ReadZip. readDirectoryEntry`. Should we retire one of these? - PR Comment: https://git.openjdk.org/jdk/pull/17038#issuecomment-1855560373
Re: RFR: 8311864: Add ArraysSupport.hashCode(int[] a, fromIndex, length, initialValue) [v3]
> This PR adds an internal method to calculate hash code from the provided > integer array, offset and length into that array, and the initial hash code > value. > > Current options for calculating a hash code for int[] are inflexible. It's > either ArraysSupport.vectorizedHashCode with an offset, length and initial > value, or Arrays.hashCode with just an array. > > For an arbitrary int[], unconditional vectorization might be unwarranted or > puzzling. Unfortunately, it's the only hash code method that operates on an > array subrange or accepts the initial hash code value. > > A more convenient method could be added and then used, for example, here: > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/concurrent/CopyOnWriteArrayList.java#L1076-L1083 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/java/util/ArrayList.java#L669-L680 > > * > https://github.com/openjdk/jdk/blob/0ef03f122866f010ebf50683097e9b92e41cdaad/src/java.base/share/classes/sun/security/pkcs10/PKCS10.java#L356-L362 > > This PR adds such a method and provides tests for it. Additionally, this PR > adds tests for `null` passed to `java.util.Arrays.hashCode` overloads, > behaviour which was specified but untested. Pavel Rappo 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: - Merge remote-tracking branch 'jdk/master' into 8311864 - Merge branch 'master' into 8311864 - Initial commit - Changes: - all: https://git.openjdk.org/jdk/pull/14831/files - new: https://git.openjdk.org/jdk/pull/14831/files/f874f161..655442eb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14831&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14831&range=01-02 Stats: 782591 lines in 3816 files changed: 173277 ins; 536256 del; 73058 mod Patch: https://git.openjdk.org/jdk/pull/14831.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14831/head:pull/14831 PR: https://git.openjdk.org/jdk/pull/14831
Re: RFR: 8310813: Simplify and modernize equals, hashCode, and compareTo for BigInteger [v8]
On Tue, 7 Nov 2023 07:58:40 GMT, Pavel Rappo wrote: >> Please review this PR to use modern APIs and language features to simplify >> equals, hashCode, and compareTo for BigInteger. If you have any performance >> concerns, please raise them. >> >> This PR is cherry-picked from a bigger, not-yet-published PR, to test the >> waters. That latter PR will be published soon. > > Pavel Rappo 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 15 additional > commits since the last revision: > > - Merge branch 'master' into 8310813 > - Merge branch 'master' into 8310813 > - Fix bugs in Shared.createSingle > - Merge branch 'master' into 8310813 > - Group params coarser (suggested by @cl4es) > >- Splits 20 params into 3 groups: (S)mall, (M)edium and (L)arge. > Every testXYZ method invokes M operations, where M is the maximum > number of elements in a group. Shorter groups are cyclically padded. >- Uses the org.openjdk.jmh.infra.Blackhole API and increases > benchmark time. >- Fixes a bug in Shared that precluded 0 from being in a pair. > - Use better overloads (suggested by @cl4es) > >- Uses simpler, more suitable overloads for the subrange > starting from 0 > - Improve benchmarks > - Merge branch 'master' into 8310813 > - Restore hash code values > >BigInteger is old and ubiquitous enough so that there might be >inadvertent dependencies on its hash code. > >This commit also includes a test, to make sure hash code is >unchanged. > - Merge branch 'master' into 8310813 > - ... and 5 more: https://git.openjdk.org/jdk/compare/5a26cc03...155fedba Dear reviewers, what do you think about that performance data I recently published? - PR Comment: https://git.openjdk.org/jdk/pull/14630#issuecomment-184882
Re: RFR: 8322018: Test java/lang/String/CompactString/MaxSizeUTF16String.java fails with -Xcomp
On Wed, 13 Dec 2023 21:38:43 GMT, Roger Riggs wrote: > The test java/lang/String/CompactString/MaxSizeUTF16String.java fails when > run with -Xcomp. > > Both the java implementation and the intrinsic for StringUTF16.toBytes() > allocate memory for a copy of the string. > The java implementation of `toBytes()` throws an exception with a message in > terms of length of the string. > The intrinsic uses a generic message when allocating a byte array that is too > large for the implementation. > > Test should accept either message on the OOME exception, the message is an > implementation detail and should reflect the cause of the error and not be > confused with a general out of java heap message. The update to the definition introduces a new `@run` with `-Xcomp` so as to explicitly control the test run instead of relying on external test launch mechanisms to pass around the `-Xcomp`. That then means that the introduction of `@requires vm.flagless`, in this change, is fine. The change in the test logic itself looks reasonable to me based on what's explained in the PR description. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17095#pullrequestreview-1781459943
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
You might want to use this older, unresolved, duplicated bug for your PR: https://bugs.openjdk.org/browse/JDK-6356745 > On 13 Dec 2023, at 23:37, Valeh Hajiyev wrote: > > Hi Pavel, > > Thanks for the reply. If I understand correctly, I need this change to be > discussed in one of the mailing lists there, so that someone would sponsor me > to create a tracking issue in JBS. Do you know which mailing list is the most > relevant for me to propose the change? > > Thanks, > Valeh > > On Thu, Dec 14, 2023 at 12:26 AM Pavel Rappo wrote: > Sorry, there's a necessary process that a PR must follow. You seem to have > signed OCA already. For the rest, see this resource: > https://openjdk.org/guide/. In particular, this part: > https://openjdk.org/guide/#contributing-to-an-openjdk-project. > > -Pavel > > > On 13 Dec 2023, at 23:09, Valeh Hajiyev wrote: > > > > Hi all, > > > > I have raised the following PR, could someone please help me to get it > > merged? > > > > https://github.com/openjdk/jdk/pull/17045 > > > > More details: > > > > This commit addresses the current limitation in the `PriorityQueue` > > implementation, which lacks a constructor to efficiently create a priority > > queue with a custom comparator and an existing collection. In order to > > create such a queue, we currently need to initialize a new queue with > > custom comparator, and after that populate the queue using `addAll()` > > method, which in the background calls `add()` method (which takes `O(logn)` > > time) for each element of the collection (`n` times). This is resulting in > > an overall time complexity of `O(nlogn)`. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(customComparator); > > pq.addAll(existingCollection); > > ``` > > > > The pull request introduces a new constructor to streamline this process > > and reduce the time complexity to `O(n)`. If you create the queue above > > using the new constructor, the contents of the collection will be copied > > (which takes `O(n)` time) and then later `heapify()` operation (Floyd's > > algorithm) will be called once (another `O(n)` time). Overall the operation > > will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > > customComparator); > > ``` > > > > Best regards, > > Valeh Hajiyev > > >
Re: [External] : Re: Introduce constructor for PriorityQueue with existing collection and custom comparator
Like David said earlier, this is the correct list. However, given the festive season, expect some delays in communication. > On 13 Dec 2023, at 23:37, Valeh Hajiyev wrote: > > Hi Pavel, > > Thanks for the reply. If I understand correctly, I need this change to be > discussed in one of the mailing lists there, so that someone would sponsor me > to create a tracking issue in JBS. Do you know which mailing list is the most > relevant for me to propose the change? > > Thanks, > Valeh > > On Thu, Dec 14, 2023 at 12:26 AM Pavel Rappo wrote: > Sorry, there's a necessary process that a PR must follow. You seem to have > signed OCA already. For the rest, see this resource: > https://openjdk.org/guide/. In particular, this part: > https://openjdk.org/guide/#contributing-to-an-openjdk-project. > > -Pavel > > > On 13 Dec 2023, at 23:09, Valeh Hajiyev wrote: > > > > Hi all, > > > > I have raised the following PR, could someone please help me to get it > > merged? > > > > https://github.com/openjdk/jdk/pull/17045 > > > > More details: > > > > This commit addresses the current limitation in the `PriorityQueue` > > implementation, which lacks a constructor to efficiently create a priority > > queue with a custom comparator and an existing collection. In order to > > create such a queue, we currently need to initialize a new queue with > > custom comparator, and after that populate the queue using `addAll()` > > method, which in the background calls `add()` method (which takes `O(logn)` > > time) for each element of the collection (`n` times). This is resulting in > > an overall time complexity of `O(nlogn)`. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(customComparator); > > pq.addAll(existingCollection); > > ``` > > > > The pull request introduces a new constructor to streamline this process > > and reduce the time complexity to `O(n)`. If you create the queue above > > using the new constructor, the contents of the collection will be copied > > (which takes `O(n)` time) and then later `heapify()` operation (Floyd's > > algorithm) will be called once (another `O(n)` time). Overall the operation > > will be reduced from `O(nlogn)` to `O(2n)` -> `O(n)` time. > > > > ``` > > PriorityQueue pq = new PriorityQueue<>(existingCollection, > > customComparator); > > ``` > > > > Best regards, > > Valeh Hajiyev > > >
Re: RFR: 8321718: ProcessTools.executeProcess calls waitFor before logging [v2]
On Tue, 12 Dec 2023 09:01:08 GMT, Stefan Karlsson wrote: >> There is some logging printed when tests spawns processes. This logging is >> triggered from calls to `OutputAnalyzer`, when it delegates calls to >> `LazyOutputBuffer`. >> >> If we write code like this: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = new OutputAnalyzer(pb.start()); >> int exitValue = output.getExitValue(); >> >> >> We get the following logging: >> >> [timestamp0] "Gathering output for process >> [timestamp1] Waiting for completion for process >> [timestamp2] Waiting for completion finished for process >> >> >> The first line comes from the `OutputAnalyzer` constructor and the two other >> lines comes from the `getExitValue` call, which calls logs the above >> messages around the call to `waitFor`. >> >> If instead write the code above as: >> >> ProcessBuilder pb = ProcessTools.createTestJavaProcessBuilder(...); >> OutputAnalyzer output = ProcessTools.executeProcess(pb); >> int exitValue = output.getExitValue(); >> >> >> We get the same logging, but timestamp1 and timestamp2 are almost the same. >> This happens because there's an extra call to `waitFor` inside >> `executeProcess`, but that `waitFor` does not have the "wait for" logging. >> So, instead we get the logging for the no-op `waitFor` inside `getExitValue`. >> >> I would like to propose a small workaround to solve this. The workaround is >> that `executeProcess` delegates the `waitFor` call to the `LazyOutputBuffer` >> via `OutputAnalyzer`. This is a small, limited workaround for this issue. >> Ideally I would like to extract the entire Process handling code out of >> LazyOutputBuffer and OutputAnalyzer, but the prototype for that rewrites all >> usages of `new OutputAnalyzer(pb.start())` and stretches far and wide in the >> test directories, so I'm starting out with this suggestion. >> >> We can see of this patch by looking at the timestamps generated from the >> included test. Without the proposed workaround: >> >> Without >> >> testExecuteProcessExit >> [2023-12-11T11:05:41.854579260Z] Gathering output for process 2547719 >> [2023-12-11T11:05:44.018335073Z] Waiting for completion for process 2547719 >> [2023-12-11T11:05:44.018851972Z] Waiting for completion finished for process >> 2547719 >> >> testExecuteProcessStdout >> [2023-12-11T11:05:44.049509860Z] Gathering output for process 2547741 >> [2023-12-11T11:05:46.227768897Z] Waiting for completion for process 2547741 >> [2023-12-11T11:05:46.228021173Z] Waiting for completion finished for process >> 2547741 >> >> >> testNewOutp... > > Stefan Karlsson has updated the pull request incrementally with one > additional commit since the last revision: > > Typo Hello Stefan, these changes look good to me. Like you note, the new test case in this PR, isn't needed, so can be removed. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17052#pullrequestreview-1781418785
Re: RFR: 8321958: javadoc error: @returns of ZoneRules#isDaylightSavings() is incorrect
On Wed, 13 Dec 2023 23:10:05 GMT, Naoto Sato wrote:
> Small document correction on a return description.
src/java.base/share/classes/java/time/zone/ZoneRules.java line 831:
> 829: * and {@link #getStandardOffset(java.time.Instant) standard}
> offsets.
> 830: *
> 831: * @param instant the instant to find the offset information for,
> not null, but null
Hello Naoto, do you think this `@param` description too will have to be
reworded? It seems to be a copy/paste of the `getDaylightSavings()` method.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17098#discussion_r1426464323
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > - [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) > > > In addition this includes the updates for > > - [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags > in JDK 23 > > Thanks > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > * [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) Thanks for doing this, David. I only note that the changes for JDK-8321384 were published in [JDK-8308715](https://bugs.openjdk.org/browse/JDK-8308715), which was integrated in the mainline before JDK 22 RDP 1. So they are already present in the mainline. - PR Comment: https://git.openjdk.org/jdk/pull/17101#issuecomment-1855467435
Re: RFR: 8317678: Fix up hashCode() for ZipFile.Source.Key [v11]
On Mon, 23 Oct 2023 16:12:45 GMT, Sean Coffey wrote: >> Fix up java.util.zip.ZipFile$Source hashCode() impl so that duplicate Source >> objects aren't created for the same zip file. > > Sean Coffey has updated the pull request incrementally with one additional > commit since the last revision: > > Update jmh test comments Hello Eirik, I have opened https://bugs.openjdk.org/browse/JDK-8322078 to track this failure. - PR Comment: https://git.openjdk.org/jdk/pull/16115#issuecomment-1855465863
Re: RFR: 8322065: Initial nroff manpage generation for JDK 23
On Thu, 14 Dec 2023 05:46:01 GMT, David Holmes wrote: > Updated the version to 23-ea and year to 2024. > > This initial generation also picks up the unpublished changes from: > > - [JDK-8302233](https://bugs.openjdk.org/browse/JDK-8302233) (keytool & > jarsigner) > - [JDK-8290702](https://bugs.openjdk.org/browse/JDK-8290702) (javadoc) (JDK > 23 backport) > - [JDK-8321384](https://bugs.openjdk.org/browse/JDK-8321384) (javadoc) > > > In addition this includes the updates for > > - [JDK-8309981](https://bugs.openjdk.org/browse/8309981) Remove expired flags > in JDK 23 > > Thanks Initially I wondered if JDK-8309981 should be separated but include keeps things in sync so I think okay. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17101#pullrequestreview-1781343785
