Re: RFR: 8283715: Update ObjectStreamClass to be final [v2]
On Tue, 29 Mar 2022 16:07:18 GMT, Stuart Marks wrote: >> Pretty much just what it says. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust nonfinal CSMs and rework error reporting in CheckCSMs.java test Hello Stuart, the test change looks fine to me. Just a minor note - the copyright year of the test will need an update. - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.java.net/browse/JDK-8003417? > > The issue notes that this is applicable for `WeakHashMap` which have `null` > keys. However, the issue is even applicable for `WeakHashMap` instances which > don't have `null` keys, as reproduced and shown by the newly added jtreg test > case in this PR. > > The root cause of the issue is that once the iterator is used to iterate till > the end and the `remove()` is called, then the > `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the > key to remove from the map, instead of the key of the last returned entry. > The commit in this PR fixes that part. > > A new jtreg test has been added which reproduces the issue as well as > verifies the fix. > `tier1` testing and this new test have passed after this change. However, I > guess this will require a JCK run to be run too, perhaps? If so, I will need > help from someone who has access to them to have this run against those > please. (work is currently in progress on this one) - PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. The test failures caught by GitHub Actions jobs look related to the area of this change: 2022-03-29T02:21:21.2187570Zat CheckCSMs.main(CheckCSMs.java:98) 2022-03-29T02:21:21.2188140Zat java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) 2022-03-29T02:21:21.2188650Zat java.base/java.lang.reflect.Method.invoke(Method.java:577) 2022-03-29T02:21:21.2189060Zat com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) 2022-03-29T02:21:21.2189420Zat java.base/java.lang.Thread.run(Thread.java:828) 2022-03-29T02:21:21.2189590Z 2022-03-29T02:21:21.2190030Z JavaTest Message: Test threw exception: java.lang.RuntimeException: Unexpected non-final instance method: 2022-03-29T02:21:21.2190400Z java/io/ObjectStreamField#getType ()Ljava/lang/Class; - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8283715: Update ObjectStreamClass to be final
On Tue, 29 Mar 2022 01:07:33 GMT, Stuart Marks wrote: > Pretty much just what it says. Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8009
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 10:55:30 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > 8282648: Problems due to conflicting specification of Inflater::inflate(..) > and InflaterInputStream::read(..) Hello Volker, An additional thing that we might have to consider here is whether adding this javadoc change to `InflaterInputStream` is ever going to "show up" to end user applications. What I mean is, I think in many cases the end user applications won't even know they are dealing with an `InflaterInputStream`. For example, the following code:
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v2]
On Mon, 28 Mar 2022 10:55:30 GMT, Volker Simonis wrote:
>> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)`
>> to highlight that it might write more bytes than the returned number of
>> inflated bytes into the buffer `b`.
>>
>> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off,
>> int len)` will leave the content beyond the last read byte in the read
>> buffer `b` unaffected. However, the overridden `read` method in
>> `InflaterInputStream` passes the read buffer `b` to
>> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this
>> guarantee. Depending on implementation details, `Inflater::inflate` might
>> write more than the returned number of inflated bytes into the buffer `b`.
>>
>> ### TL;DR
>>
>> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater
>> functionality. `Inflater::inflate(byte[] output, int off, int len)`
>> currently calls zlib's native `inflate(..)` function and passes the address
>> of `output[off]` and `len` to it via JNI.
>>
>> The specification of zlib's `inflate(..)` function (i.e. the [API
>> documentation in the original zlib
>> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400))
>> doesn't give any guarantees with regard to usage of the output buffer. It
>> only states that upon completion the function will return the number of
>> bytes that have been written (i.e. "inflated") into the output buffer.
>>
>> The original zlib implementation only wrote as many bytes into the output
>> buffer as it inflated. However, this is not a hard requirement and newer,
>> more performant implementations of the zlib library like
>> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/)
>> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes
>> of the output buffer than they actually inflate as a scratch buffer. See
>> https://github.com/simonis/zlib-chromium for a more detailed description of
>> their approach and its performance benefit.
>>
>> These new zlib versions can still be used transparently from Java (e.g. by
>> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because
>> they still fully comply to specification of `Inflater::inflate(..)`.
>> However, we might run into problems when using the `Inflater` functionality
>> from the `InflaterInputStream` class. `InflaterInputStream` is derived from
>> from `InputStream` and as such, its `read(byte[] b, int off, int len)`
>> method is quite constrained. It specifically specifies that if *k* bytes
>> have been read, then "these bytes will be stored in elements `b[off]`
>> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through
>> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int
>> off, int len)` (which is constrained by `InputStream::read(..)`'s
>> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and
>> directly passes its output buffer down to the native zlib `inflate(..)`
>> method which is free to change the bytes beyond `b[off+`
*k*`]` (where *k* is the number of inflated bytes).
>>
>> From a practical point of view, I don't see this as a big problem, because
>> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never
>> know how many bytes will be written into the output buffer `b` (and in fact
>> its content can always be completely overwritten). It therefore makes no
>> sense to depend on any data there being untouched after the call. Also,
>> having used zlib-cloudflare productively for about two years, we haven't
>> seen real-world issues because of this behavior yet. However, from a
>> specification point of view it is easy to artificially construct a program
>> which violates `InflaterInputStream::read(..)`'s postcondition if using one
>> of the alterantive zlib implementations. A recently integrated JTreg test
>> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails
>> with zlib-chromium but can fixed easily to run with alternative
>> implementations as well (see
>> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)).
>
> Volker Simonis has refreshed the contents of this pull request, and previous
> commits have been removed. The incremental views will show differences
> compared to the previous content of the PR. The pull request contains one new
> commit since the last revision:
>
> 8282648: Problems due to conflicting specification of Inflater::inflate(..)
> and InflaterInputStream::read(..)
src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 133:
> 131: * Unlike the {@link InputStream#read(byte[],int,int) overridden
> method}
> 132: * of {@code InputStream}, this method might write more bytes than
> the returned
> 133: * number of inflated bytes into the buffer {@code b}.
Hello Volker, I think this
Integrated: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. This pull request has now been integrated. Changeset: 85672667 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/8567266795cd1171f5b353d0e0813e7eeff319c2 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8283683: Make ThreadLocalRandom a final class Reviewed-by: smarks, chegar - PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. Thank you everyone for the reviews of the PR and the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. Thank you Doug. - PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: 8283683: Make ThreadLocalRandom a final class
On Fri, 25 Mar 2022 13:32:21 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which marks the `ThreadLocalRandom` > class as `final`? Related JBS issue > https://bugs.openjdk.java.net/browse/JDK-8283683. > > A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. > > tier1, tier2 and tier3 tests have been run with this change and no related > failures have been noticed. Hello @DougLea, do you think this change is worth doing and perhaps something that you would be willing to include in any of your future bulk updates in this area? - PR: https://git.openjdk.java.net/jdk/pull/7958
RFR: 8283683: Make ThreadLocalRandom a final class
Can I please get a review of this change which marks the `ThreadLocalRandom` class as `final`? Related JBS issue https://bugs.openjdk.java.net/browse/JDK-8283683. A CSR has been filed too https://bugs.openjdk.java.net/browse/JDK-8283688. tier1, tier2 and tier3 tests have been run with this change and no related failures have been noticed. - Commit messages: - 8283683: Make ThreadLocalRandom a final class Changes: https://git.openjdk.java.net/jdk/pull/7958/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7958=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283683 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7958.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7958/head:pull/7958 PR: https://git.openjdk.java.net/jdk/pull/7958
Re: RFR: JDK-8283668: Update IllegalFormatException to use sealed classes
On Fri, 25 Mar 2022 04:17:57 GMT, Joe Darcy wrote: > Working down the list of candidates to be sealed, this time > IllegalFormatException. > > Please also review the companion CSR: > https://bugs.openjdk.java.net/browse/JDK-8283669 Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7950
Integrated: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes
On Sun, 20 Mar 2022 04:24:07 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which handles > https://bugs.openjdk.java.net/browse/JDK-8283411? > > The commit here moves the temporary byte array from being a member of the > class to a local variable within the `skip` method which is the only place > where it is used as a temporary buffer. > > tier1, tier2, tier3 tests have been run successfully with this change. This pull request has now been integrated. Changeset: 91fab6ad Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/91fab6ad59d2a4baf58802fc6e6039af3dd8d578 Stats: 4 lines in 1 file changed: 1 ins; 2 del; 1 mod 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes Reviewed-by: lancea, vtewari, alanb - PR: https://git.openjdk.java.net/jdk/pull/7875
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes [v2]
On Sun, 20 Mar 2022 14:05:58 GMT, Jaikiran Pai wrote: >> Can I please get a review of this change which handles >> https://bugs.openjdk.java.net/browse/JDK-8283411? >> >> The commit here moves the temporary byte array from being a member of the >> class to a local variable within the `skip` method which is the only place >> where it is used as a temporary buffer. >> >> tier1, tier2, tier3 tests have been run successfully with this change. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > Use Alan's suggestion and allocate less than 512 bytes if possible. Plus > copyright year fix. Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/7875
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes [v2]
On Sun, 20 Mar 2022 14:01:50 GMT, Jaikiran Pai wrote: >> src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 206: >> >>> 204: int max = (int)Math.min(n, Integer.MAX_VALUE); >>> 205: int total = 0; >>> 206: byte[] b = new byte[512]; >> >> n may be less than 512 so maybe the temporary array can be of length >> Math.min(max, 512). > > That's a good idea. I just pushed a update to this PR with this suggested > change. Plus updated the copyright year too. I ran tier1, tier2 and tier3 tests with this change and they completed successfully. Alan, does the current state of the PR look fine to you? Should I go ahead with the merge? - PR: https://git.openjdk.java.net/jdk/pull/7875
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes [v2]
> Can I please get a review of this change which handles > https://bugs.openjdk.java.net/browse/JDK-8283411? > > The commit here moves the temporary byte array from being a member of the > class to a local variable within the `skip` method which is the only place > where it is used as a temporary buffer. > > tier1, tier2, tier3 tests have been run successfully with this change. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: Use Alan's suggestion and allocate less than 512 bytes if possible. Plus copyright year fix. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7875/files - new: https://git.openjdk.java.net/jdk/pull/7875/files/6bc997fb..f4250e7e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7875=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7875=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7875/head:pull/7875 PR: https://git.openjdk.java.net/jdk/pull/7875
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes [v2]
On Sun, 20 Mar 2022 13:53:34 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Use Alan's suggestion and allocate less than 512 bytes if possible. Plus >> copyright year fix. > > src/java.base/share/classes/java/util/zip/InflaterInputStream.java line 206: > >> 204: int max = (int)Math.min(n, Integer.MAX_VALUE); >> 205: int total = 0; >> 206: byte[] b = new byte[512]; > > n may be less than 512 so maybe the temporary array can be of length > Math.min(max, 512). That's a good idea. I just pushed a update to this PR with this suggested change. Plus updated the copyright year too. - PR: https://git.openjdk.java.net/jdk/pull/7875
Re: RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes
Hello Bernd, On 20/03/22 5:00 pm, Bernd Eckenfels wrote: Hello, Not sure how often skip is actually used so it might not matter, but this change would increase allocations if skip is called regularly. I had given this a thought before changing it. The skip doesn't get called in a regular jar/zip entry content retrieval use case (which from what I can see is the most used one). So instead of allocating an additional 512 bytes, holding onto it till the stream is closed and never using it, for each entry of a zip whose content is fetched, I think it's better to allocate it only when needed in skip(). -Jaikiran
RFR: 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes
Can I please get a review of this change which handles https://bugs.openjdk.java.net/browse/JDK-8283411? The commit here moves the temporary byte array from being a member of the class to a local variable within the `skip` method which is the only place where it is used as a temporary buffer. tier1, tier2, tier3 tests have been run successfully with this change. - Commit messages: - 8283411: InflaterInputStream holds on to a temporary byte array of 512 bytes Changes: https://git.openjdk.java.net/jdk/pull/7875/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7875=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283411 Stats: 3 lines in 1 file changed: 1 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/7875.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7875/head:pull/7875 PR: https://git.openjdk.java.net/jdk/pull/7875
Integrated: 8282572: EnumSet should be a sealed class
On Tue, 8 Mar 2022 10:50:35 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to implement the > enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? > > The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes > - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any > sub-classes of their own. > > In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` > and `RegularEnumSet` are the two permitted sub classes. Both of these > sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for > these 2 sub-classes was an option too. But I decided to start with the more > restrictive option since we don't have any other sub-classes and if and when > we do have their sub-classes, it's possible to change them to `non-sealed`. > > The `EnumSet` class implements the `java.io.Serializable` interface. As part > of this change, manual tests have been run to make sure that changing this > class to `sealed` and marking the sub-classes as `final` don't break any > serialization/deserialization semantics, across Java version and/or user > application versions. > > `tier1` testing across various platforms is currently in progress. This pull request has now been integrated. Changeset: 3cf83a67 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/3cf83a671eaedd78d87197dffa76dcc3fededb78 Stats: 7 lines in 3 files changed: 0 ins; 0 del; 7 mod 8282572: EnumSet should be a sealed class Reviewed-by: sundar - PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: 8282572: EnumSet should be a sealed class
On Fri, 11 Mar 2022 09:04:23 GMT, Athijegannathan Sundararajan wrote: >> Can I please get a review of this change which proposes to implement the >> enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? >> >> The (public) `EnumSet` class has 2 package private (JDK) internal >> sub-classes - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't >> have any sub-classes of their own. >> >> In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` >> and `RegularEnumSet` are the two permitted sub classes. Both of these >> sub-classes are now marked as `final` too. Usage of `non-sealed` modifier >> for these 2 sub-classes was an option too. But I decided to start with the >> more restrictive option since we don't have any other sub-classes and if and >> when we do have their sub-classes, it's possible to change them to >> `non-sealed`. >> >> The `EnumSet` class implements the `java.io.Serializable` interface. As part >> of this change, manual tests have been run to make sure that changing this >> class to `sealed` and marking the sub-classes as `final` don't break any >> serialization/deserialization semantics, across Java version and/or user >> application versions. >> >> `tier1` testing across various platforms is currently in progress. > > LGTM Thank you @sundararajana for the review and thanks everyone for the help on the CSR. - PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Thu, 10 Mar 2022 18:34:27 GMT, Jim Laskey wrote:
>> Several attempts have been made to improve Formatter's numeric performance
>> by caching the current Locale zero. Such fixes, however, ignore the real
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines
>> the process of accessing Locale DecimalFormatSymbols and specifically
>> getZeroDigit(). The result is a general improvement in the performance of
>> numeric formatting.
>>
>>
>> @Benchmark
>> public void bigDecimalDefaultLocale() {
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalLocaleAlternate() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalThaiLocale() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void integerDefaultLocale() {
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerLocaleAlternate() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerThaiLocale() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey 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 14 additional commits since
> the last revision:
>
> - Merge branch 'master' into 8282625
> - Correct indentation
> - Add unit test for DecimalFormatSymbols.getLocale()
> - Add comment about DecimalFormatSymbols being mutable objects.
> - Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
> Formatter. No significant performance degradation."
>
>This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
> - Revert "Drop DecimalFormatSymbols.getLocale change"
>
>This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
> - Revert "Correct caching test"
>
>This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
> - Correct caching test
> - Drop DecimalFormatSymbols.getLocale change
> - Cache DecimalFormatSymbols in DecimalFormatSymbols instead of Formatter.
> No significant performance degradation.
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/76644795...84fa1fe7
Marked as reviewed by jpai (Committer).
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Fri, 11 Mar 2022 10:16:33 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 192:
>>
>>> 190:
>>> 191: /**
>>> 192: * {@return locale used to create this instance}
>>
>> Hello Jim, the opening and closing `{`, I think aren't needed for a `@return`
>
> This is actually a feature of JavaDoc. Accessor methods that require little
> description (self evident) can use {@return ...} to define the description
> and return in one go.
Didn't know that, thank you for clarifying.
The rest of the changes look good to me.
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8282572: EnumSet should be a sealed class
On Tue, 8 Mar 2022 10:50:35 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to implement the > enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? > > The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes > - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any > sub-classes of their own. > > In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` > and `RegularEnumSet` are the two permitted sub classes. Both of these > sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for > these 2 sub-classes was an option too. But I decided to start with the more > restrictive option since we don't have any other sub-classes and if and when > we do have their sub-classes, it's possible to change them to `non-sealed`. > > The `EnumSet` class implements the `java.io.Serializable` interface. As part > of this change, manual tests have been run to make sure that changing this > class to `sealed` and marking the sub-classes as `final` don't break any > serialization/deserialization semantics, across Java version and/or user > application versions. > > `tier1` testing across various platforms is currently in progress. The linked CSR has now been approved. - PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v10]
On Thu, 10 Mar 2022 18:34:27 GMT, Jim Laskey wrote:
>> Several attempts have been made to improve Formatter's numeric performance
>> by caching the current Locale zero. Such fixes, however, ignore the real
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines
>> the process of accessing Locale DecimalFormatSymbols and specifically
>> getZeroDigit(). The result is a general improvement in the performance of
>> numeric formatting.
>>
>>
>> @Benchmark
>> public void bigDecimalDefaultLocale() {
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalLocaleAlternate() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalThaiLocale() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void integerDefaultLocale() {
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerLocaleAlternate() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerThaiLocale() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey 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 14 additional commits since
> the last revision:
>
> - Merge branch 'master' into 8282625
> - Correct indentation
> - Add unit test for DecimalFormatSymbols.getLocale()
> - Add comment about DecimalFormatSymbols being mutable objects.
> - Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
> Formatter. No significant performance degradation."
>
>This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
> - Revert "Drop DecimalFormatSymbols.getLocale change"
>
>This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
> - Revert "Correct caching test"
>
>This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
> - Correct caching test
> - Drop DecimalFormatSymbols.getLocale change
> - Cache DecimalFormatSymbols in DecimalFormatSymbols instead of Formatter.
> No significant performance degradation.
> - ... and 4 more:
> https://git.openjdk.java.net/jdk/compare/11e9685d...84fa1fe7
src/java.base/share/classes/java/text/DecimalFormatSymbols.java line 192:
> 190:
> 191: /**
> 192: * {@return locale used to create this instance}
Hello Jim, the opening and closing `{`, I think aren't needed for a `@return`
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8282572: EnumSet should be a sealed class
On Tue, 8 Mar 2022 10:50:35 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to implement the > enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? > > The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes > - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any > sub-classes of their own. > > In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` > and `RegularEnumSet` are the two permitted sub classes. Both of these > sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for > these 2 sub-classes was an option too. But I decided to start with the more > restrictive option since we don't have any other sub-classes and if and when > we do have their sub-classes, it's possible to change them to `non-sealed`. > > The `EnumSet` class implements the `java.io.Serializable` interface. As part > of this change, manual tests have been run to make sure that changing this > class to `sealed` and marking the sub-classes as `final` don't break any > serialization/deserialization semantics, across Java version and/or user > application versions. > > `tier1` testing across various platforms is currently in progress. I've created the CSR here https://bugs.openjdk.java.net/browse/JDK-8282795 - PR: https://git.openjdk.java.net/jdk/pull/7741
RFR: 8282572: EnumSet should be a sealed class
Can I please get a review of this change which proposes to implement the enhancement requested in https://bugs.openjdk.java.net/browse/JDK-8282572? The (public) `EnumSet` class has 2 package private (JDK) internal sub-classes - the `JumboEnumSet` and `RegularEnumSet`. These 2 classes don't have any sub-classes of their own. In this commit, the `EnumSet` is marked as `sealed` and the `JumboEnumSet` and `RegularEnumSet` are the two permitted sub classes. Both of these sub-classes are now marked as `final` too. Usage of `non-sealed` modifier for these 2 sub-classes was an option too. But I decided to start with the more restrictive option since we don't have any other sub-classes and if and when we do have their sub-classes, it's possible to change them to `non-sealed`. The `EnumSet` class implements the `java.io.Serializable` interface. As part of this change, manual tests have been run to make sure that changing this class to `sealed` and marking the sub-classes as `final` don't break any serialization/deserialization semantics, across Java version and/or user application versions. `tier1` testing across various platforms is currently in progress. - Commit messages: - 8282572: EnumSet should be a sealed class Changes: https://git.openjdk.java.net/jdk/pull/7741/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7741=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282572 Stats: 7 lines in 3 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7741.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7741/head:pull/7741 PR: https://git.openjdk.java.net/jdk/pull/7741
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v7]
On Mon, 7 Mar 2022 20:36:36 GMT, Jim Laskey wrote:
>> Several attempts have been made to improve Formatter's numeric performance
>> by caching the current Locale zero. Such fixes, however, ignore the real
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines
>> the process of accessing Locale DecimalFormatSymbols and specifically
>> getZeroDigit(). The result is a general improvement in the performance of
>> numeric formatting.
>>
>>
>> @Benchmark
>> public void bigDecimalDefaultLocale() {
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalLocaleAlternate() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalThaiLocale() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void integerDefaultLocale() {
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerLocaleAlternate() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerThaiLocale() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Add comment about DecimalFormatSymbols being mutable objects.
> - Revert "Cache DecimalFormatSymbols in DecimalFormatSymbols instead of
> Formatter. No significant performance degradation."
>
>This reverts commit fcbf66a2fe9641d3c3349f12cc7b32d8b84c6f72.
> - Revert "Drop DecimalFormatSymbols.getLocale change"
>
>This reverts commit b93cdb03ec68f24f4b8851c0966bb144c30b5110.
> - Revert "Correct caching test"
>
>This reverts commit bf7975396aaad4ed58d053bde8f54984215eeba5.
src/java.base/share/classes/java/util/Formatter.java line 2019:
> 2017: return dfs;
> 2018: }
> 2019: // Fetch a new local instance of DecimalFormatSymbols. Note
> that DFS are mutatble
Thank you Jim for these comments about multi-threaded access. Looks good to me.
One minor typo on this line - "mutatble" should have been "mutable".
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: 8281560: Matcher.hitEnd returns unexpected results in presence of CANON_EQ flag. [v2]
On Tue, 8 Mar 2022 04:20:17 GMT, Ian Graves wrote: >> test/jdk/java/util/regex/RegExTest.java line 4568: >> >>> 4566: >>> 4567: var matcher1 = Pattern.compile(pat1).matcher(testInput); >>> 4568: var matcher2 = Pattern.compile(pat2).matcher(testInput); >> >> Hello Ian, >> The JBS issue notes that the bug is only noticed when the `Pattern.CANON_EQ` >> is used. The javadoc of this `CANON_EQ` states that this isn't enabled by >> default. Do you think this test should also include matchers which use >> Pattern(s) with this `CANON_EQ` explicitly enabled? > > Oh wow yes. Thanks for this catch. I was rewriting the tests to fit this mold > and left the flags out. Added them back in. Thanks again! Thank you Ian, the changed version of the test looks good. - PR: https://git.openjdk.java.net/jdk/pull/7466
Re: RFR: 8281560: Matcher.hitEnd returns unexpected results in presence of CANON_EQ flag. [v2]
On Mon, 7 Mar 2022 22:29:29 GMT, Ian Graves wrote: >> Fixing a bug in `NFCCharProperty` where code falling through an if-statement >> can prematurely set the matcher's `hitEnd` field to true. > > Ian Graves has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains two commits: > > - merging master > - Catching erronious setting of matcher.hitEnd test/jdk/java/util/regex/RegExTest.java line 4568: > 4566: > 4567: var matcher1 = Pattern.compile(pat1).matcher(testInput); > 4568: var matcher2 = Pattern.compile(pat2).matcher(testInput); Hello Ian, The JBS issue notes that the bug is only noticed when the `Pattern.CANON_EQ` is used. The javadoc of this `CANON_EQ` states that this isn't enabled by default. Do you think this test should also include matchers which use Pattern(s) with this `CANON_EQ` explicitly enabled? - PR: https://git.openjdk.java.net/jdk/pull/7466
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]
On Sat, 5 Mar 2022 14:20:40 GMT, Jaikiran Pai wrote: > will now try and update/use this cached class level static state DFS. That > would thus require some kind of thread safety semantics to be implemented for > this new getDecimalFormatSymbols(Locale) method, isn't it? A bit more closer look at the code and it appears to me that the use of : DecimalFormatSymbols dfs = DFS; and then working off that local variable prevents any kind of race issues that might be caused due to multi-thread access. Of course it still means that multiple threads might still go ahead and do a: dfs = DecimalFormatSymbols.getInstance(locale); on first access (when `DFS` is null) but that in itself should be harmless. If this is intentional (which I suspect it is), should some comment be added in this method explaining this multi-thread access detail? - PR: https://git.openjdk.java.net/jdk/pull/7703
Re: RFR: JDK-8282625 Formatter caches Locale/DecimalFormatSymbols poorly [v3]
On Fri, 4 Mar 2022 21:17:50 GMT, Jim Laskey wrote:
>> Several attempts have been made to improve Formatter's numeric performance
>> by caching the current Locale zero. Such fixes, however, ignore the real
>> issue, which is the slowness of fetching DecimalFormatSymbols. By directly
>> caching DecimalFormatSymbols in the Formatter, this enhancement streamlines
>> the process of accessing Locale DecimalFormatSymbols and specifically
>> getZeroDigit(). The result is a general improvement in the performance of
>> numeric formatting.
>>
>>
>> @Benchmark
>> public void bigDecimalDefaultLocale() {
>> result = String.format("%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalLocaleAlternate() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> other = String.format(DEFAULT, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void bigDecimalThaiLocale() {
>> result = String.format(THAI, "%1$f %1$f %1$f %1$f %1$f %1$f %1$f %1$f
>> %1$f %1$f", X);
>> }
>>
>> @Benchmark
>> public void integerDefaultLocale() {
>> result = String.format("%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerLocaleAlternate() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> other = String.format(DEFAULT, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>> @Benchmark
>> public void integerThaiLocale() {
>> result = String.format(THAI, "%1$d %1$d %1$d %1$d %1$d %1$d %1$d %1$d
>> %1$d %1$d", x);
>> }
>>
>>
>> Before:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 75498.923 ± 3686.966 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 39068.721 ± 162.983 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 77256.530 ± 294.743 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 344093.071 ± 6189.002 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165685.488 ± 440.857 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 327461.302 ± 1168.243 ops/s
>>
>> After:
>> Benchmark Mode Cnt Score Error Units
>> MyBenchmark.bigDecimalDefaultLocale thrpt 25 94735.293 ± 674.587 ops/s
>> MyBenchmark.bigDecimalLocaleAlternate thrpt 25 44215.547 ± 291.664 ops/s
>> MyBenchmark.bigDecimalThaiLocale thrpt 25 91390.997 ± 658.677 ops/s
>> MyBenchmark.integerDefaultLocale thrpt 25 363722.977 ± 2864.554 ops/s
>> MyBenchmark.integerLocaleAlternate thrpt 25 165789.514 ± 779.656 ops/s
>> MyBenchmark.integerThaiLocale thrpt 25 351400.818 ± 1030.246 ops/s
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Too many 'the'
src/java.base/share/classes/java/util/Formatter.java line 2012:
> 2010: public final class Formatter implements Closeable, Flushable {
> 2011: // Caching DecimalFormatSymbols
> 2012: static DecimalFormatSymbols DFS = null;
Hello Jim,
The javadoc of `Formatter` states:
>
> Formatters are not necessarily safe for multithreaded access. Thread safety
> is optional and is the responsibility of users of methods in this class.
>
So I would think that user applications would typically be synchronizing on the
instance of a `Formatter` for any multi-threaded use of a formatter instance.
If I'm reading this changed code correctly, it's now possible that 2 separate
instances of a `Formatter` being concurrently accessed (i.e. in different
threads) will now try and update/use this cached class level `static` state
`DFS`. That would thus require some kind of thread safety semantics to be
implemented for this new `getDecimalFormatSymbols(Locale)` method, isn't it?
-
PR: https://git.openjdk.java.net/jdk/pull/7703
Integrated: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale
On Mon, 21 Feb 2022 14:09:50 GMT, Jaikiran Pai wrote: > Can I please get a review of this test only change which fixes the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8282023? > > As noted in that JBS issue these tests fail when the default locale under > which those tests are run isn't `en_US`. Both these tests were introduced as > part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, > these test do the following: > - Use Properties.store(...) APIs to write out a properties file. This > internally ends up writing a date comment via a call to > `java.util.Date.toString()` method. > - The test then runs a few checks to make sure that the written out `Date` is > an expected one. To run these checks it uses the > `java.time.format.DateTimeFormatter` to parse the date comment out of the > properties file. > > All this works fine when the default locale is (for example) `en_US`. > However, when the default locale is (for example `en_CA` or even `hi_IN`) the > tests fail with an exception in the latter step above while parsing the date > comment using the `DateTimeFormatter` instance. The exception looks like: > > > Using locale: he for Properties#store(OutputStream) test > test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure > java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST > 2022 > at org.testng.Assert.fail(Assert.java:87) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) > at > PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) > at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) > at > org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) > at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) > at > org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) > at > org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) > at > org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) > at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) > at org.testng.TestRunner.privateRun(TestRunner.java:764) > at org.testng.TestRunner.run(TestRunner.java:585) > at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) > at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) > at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) > at org.testng.SuiteRunner.run(SuiteRunner.java:286) > at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) > at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) > at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) > at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) > at org.testng.TestNG.runSuites(TestNG.java:1069) > at org.testng.TestNG.run(TestNG.java:1037) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:828) > Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 > IST 2022' could not be parsed at index 0 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) > ... 30 more > > (in the exception above a locale with language `he` is being used) > > The root cause of this failure lies (only) within the tests - the > `DateTimeFormatter` used in the tests is locale sensitive and uses the > current (`Locale.getDefault()`) locale by default for parsing the date text. > This parsing fails because, although `Date.toString()` javadoc states nothing > about locales, it does mention the exact characters that will be used to >
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v5]
On Fri, 25 Feb 2022 08:44:42 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test only change which fixes the issue >> noted in https://bugs.openjdk.java.net/browse/JDK-8282023? >> >> As noted in that JBS issue these tests fail when the default locale under >> which those tests are run isn't `en_US`. Both these tests were introduced as >> part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, >> these test do the following: >> - Use Properties.store(...) APIs to write out a properties file. This >> internally ends up writing a date comment via a call to >> `java.util.Date.toString()` method. >> - The test then runs a few checks to make sure that the written out `Date` >> is an expected one. To run these checks it uses the >> `java.time.format.DateTimeFormatter` to parse the date comment out of the >> properties file. >> >> All this works fine when the default locale is (for example) `en_US`. >> However, when the default locale is (for example `en_CA` or even `hi_IN`) >> the tests fail with an exception in the latter step above while parsing the >> date comment using the `DateTimeFormatter` instance. The exception looks >> like: >> >> >> Using locale: he for Properties#store(OutputStream) test >> test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure >> java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST >> 2022 >> at org.testng.Assert.fail(Assert.java:87) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) >> at >> PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) >> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) >> at >> org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) >> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) >> at >> org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) >> at >> org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) >> at >> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) >> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) >> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) >> at org.testng.TestRunner.privateRun(TestRunner.java:764) >> at org.testng.TestRunner.run(TestRunner.java:585) >> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) >> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) >> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) >> at org.testng.SuiteRunner.run(SuiteRunner.java:286) >> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) >> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) >> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) >> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) >> at org.testng.TestNG.runSuites(TestNG.java:1069) >> at org.testng.TestNG.run(TestNG.java:1037) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) >> at java.base/java.lang.Thread.run(Thread.java:828) >> Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 >> 19:10:31 IST 2022' could not be parsed at index 0 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) >> ... 30 more >> >> (in the exception above a locale with language `he` is being used) >> >> The root cause of this failure lies (only) within the tests - the >> `DateTime
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Fri, 25 Feb 2022 04:44:45 GMT, Naoto Sato wrote: >> That's a very good point. I've updated the PR to now explicitly use a >> mutable `Set` instead of using `Collectors.toSet()` > > This is ok, although `Collectors.toCollection(HashSet::new)` is a bit concise. Hello Naoto, I've updated the PR to use `HashSet::new`. - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v5]
On Fri, 25 Feb 2022 08:44:42 GMT, Jaikiran Pai wrote: >> Can I please get a review of this test only change which fixes the issue >> noted in https://bugs.openjdk.java.net/browse/JDK-8282023? >> >> As noted in that JBS issue these tests fail when the default locale under >> which those tests are run isn't `en_US`. Both these tests were introduced as >> part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, >> these test do the following: >> - Use Properties.store(...) APIs to write out a properties file. This >> internally ends up writing a date comment via a call to >> `java.util.Date.toString()` method. >> - The test then runs a few checks to make sure that the written out `Date` >> is an expected one. To run these checks it uses the >> `java.time.format.DateTimeFormatter` to parse the date comment out of the >> properties file. >> >> All this works fine when the default locale is (for example) `en_US`. >> However, when the default locale is (for example `en_CA` or even `hi_IN`) >> the tests fail with an exception in the latter step above while parsing the >> date comment using the `DateTimeFormatter` instance. The exception looks >> like: >> >> >> Using locale: he for Properties#store(OutputStream) test >> test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure >> java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST >> 2022 >> at org.testng.Assert.fail(Assert.java:87) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) >> at >> PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) >> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) >> at >> org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) >> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) >> at >> org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) >> at >> org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) >> at >> org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) >> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) >> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) >> at org.testng.TestRunner.privateRun(TestRunner.java:764) >> at org.testng.TestRunner.run(TestRunner.java:585) >> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) >> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) >> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) >> at org.testng.SuiteRunner.run(SuiteRunner.java:286) >> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) >> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) >> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) >> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) >> at org.testng.TestNG.runSuites(TestNG.java:1069) >> at org.testng.TestNG.run(TestNG.java:1037) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) >> at >> com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) >> at >> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) >> at java.base/java.lang.reflect.Method.invoke(Method.java:577) >> at >> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) >> at java.base/java.lang.Thread.run(Thread.java:828) >> Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 >> 19:10:31 IST 2022' could not be parsed at index 0 >> at >> java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) >> at >> java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) >> at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) >> ... 30 more >> >> (in the exception above a locale with language `he` is being used) >> >> The root cause of this failure lies (only) within the tests - the >> `DateTime
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v5]
comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. > Additionally, while I was at it, I updated the `PropertiesStoreTest` to > automate the use of multiple different locales to reproduce this issue > (automatically) and verify the fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: HashSet::new instead of new HashSet() in test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7558/files - new: https://git.openjdk.java.net/jdk/pull/7558/files/1d14f1c7..915f12e1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7558=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7558=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7558/head:pull/7558 PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
On Thu, 24 Feb 2022 17:15:16 GMT, Naoto Sato wrote: >> Jaikiran Pai has updated the pull request incrementally with four additional >> commits since the last revision: >> >> - use Roger's suggestion of using Stream and Collection based APIs to avoid >> code duplication in the data provider method of the test >> - no need for system.out.println since testng add the chosen params to the >> output logs >> - review comments: >> - upper case static final fields in test >> - use DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN, Locale.ROOT) >> - remove @modules declaration on the jtreg test > > test/jdk/java/util/Properties/PropertiesStoreTest.java line 112: > >> 110: locales.add(Locale.getDefault()); // always test the default >> locale >> 111: locales.add(Locale.US); // guaranteed to be present >> 112: locales.add(Locale.ROOT); // guaranteed to be present > > Can we assume the returned `Set` is mutable? `Collectors.toSet()` > javadoc reads no guarantee for mutability. That's a very good point. I've updated the PR to now explicitly use a mutable `Set` instead of using `Collectors.toSet()` - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v4]
comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. > Additionally, while I was at it, I updated the `PropertiesStoreTest` to > automate the use of multiple different locales to reproduce this issue > (automatically) and verify the fix. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: use Collections.toCollection() instead of Collectors.toSet() to allow for mutability guarantees - Changes: - all: https://git.openjdk.java.net/jdk/pull/7558/files - new: https://git.openjdk.java.net/jdk/pull/7558/files/97c9afd5..1d14f1c7 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7558=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7558=02-03 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7558/head:pull/7558 PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]
On Wed, 23 Feb 2022 18:37:03 GMT, Roger Riggs wrote:
>> Jaikiran Pai has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - implement review comments
>> - copyright years
>
> test/jdk/java/util/Properties/PropertiesStoreTest.java line 61:
>
>> 59: private static final DateTimeFormatter formatter =
>> DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN)
>> 60: .withLocale(Locale.ROOT);
>> 61: private static final Locale prevLocale = Locale.getDefault();
>
> By convention, static final field names are uppercase. It make the code
> using them easier to read.
Makes sense. Fixed in the latest version of the PR.
> test/jdk/java/util/Properties/PropertiesStoreTest.java line 112:
>
>> 110: if (!locale.getLanguage().isEmpty() &&
>> !locale.getLanguage().equals("en")) {
>> 111: nonEnglishLocale = locale;
>> 112: System.out.println("Selected non-english locale: " +
>> nonEnglishLocale + " for tests");
>
> TestNG includes the arguments in the output when the test is run.
Removed the System.out.println statement from the latest version of the PR.
> test/jdk/java/util/Properties/PropertiesStoreTest.java line 116:
>
>> 114: }
>> 115: }
>> 116: if (nonEnglishLocale == null) {
>
> Alternatively, create a Set, to avoid duplicates, adding the
> candidates as they are discovered
> and finally convert the Set to an Object[][]. It may be a bit easier to
> maintain.
>
> private static Object[][] provideLocales() {
> // pick a non-english locale for testing
> Set locales = Arrays.stream(Locale.getAvailableLocales())
> .filter(l -> !l.getLanguage().isEmpty() &&
> !l.getLanguage().equals("en"))
> .limit(1)
> .collect(Collectors.toSet());
> locales.add(Locale.getDefault());
> locales.add(Locale.US);
> locales.add(Locale.ROOT);
>
> return locales.stream()
> .map(m -> new Locale[] {m})
> .toArray(n -> new Object[n][0]);
> }
Thank you Roger for this suggestion. This indeed is cleaner. I've updated the
PR to use this suggested code. The tests continue to pass with the latest round
of changes.
-
PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]
On Wed, 23 Feb 2022 18:22:53 GMT, Naoto Sato wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - implement review comments >> - copyright years > > test/jdk/java/util/Properties/PropertiesStoreTest.java line 51: > >> 49: * @summary tests the order in which the Properties.store() method >> writes out the properties >> 50: * @bug 8231640 8282023 >> 51: * @modules jdk.localedata > > OK, you can remove `@modules jdk.localedata` so that other tests would run. > (it'd be unusual setup, but still possible with jlink) Thank you Naoto. I've removed this in the latest version of the PR. > test/jdk/java/util/Properties/PropertiesStoreTest.java line 60: > >> 58: // it internally calls the Date.toString() which always writes in a >> locale insensitive manner >> 59: private static final DateTimeFormatter formatter = >> DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN) >> 60: .withLocale(Locale.ROOT); > > Should have noticed before, but you can call the convenient overload of > `ofPattern(DATE_FORMAT_PATTERN, Locale.ROOT)` here. Applies to the other test > too. That makes sense. I hadn't noticed that API before. I've updated the PR with this suggested change. - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v3]
comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. > Additionally, while I was at it, I updated the `PropertiesStoreTest` to > automate the use of multiple different locales to reproduce this issue > (automatically) and verify the fix. Jaikiran Pai has updated the pull request incrementally with four additional commits since the last revision: - use Roger's suggestion of using Stream and Collection based APIs to avoid code duplication in the data provider method of the test - no need for system.out.println since testng add the chosen params to the output logs - review comments: - upper case static final fields in test - use DateTimeFormatter.ofPattern(DATE_FORMAT_PATTERN, Locale.ROOT) - remove @modules declaration on the jtreg test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7558/files - new: https://git.openjdk.java.net/jdk/pull/7558/files/c5dd7f79..97c9afd5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7558=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7558=01-02 Stats: 36 lines in 2 files changed: 1 ins; 12 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/7558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7558/head:pull/7558 PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]
On Tue, 22 Feb 2022 17:31:14 GMT, Naoto Sato wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - implement review comments >> - copyright years > > test/jdk/java/util/Properties/PropertiesStoreTest.java line 50: > >> 48: * @test >> 49: * @summary tests the order in which the Properties.store() method >> writes out the properties >> 50: * @bug 8231640 > > Add the bug id, also `@modules jdk.localedata` needs to be added. I have added this to the updated PR. But just to understand why this is needed, I had a look at the jtreg tag specification doc, which states: > > a test will not be run if the system being tested does not contain all of the > specified modules So with this `@modules` added, it will no longer run tests where the `jdk.localedata` isn't present. Given that this `PropertiesStoreTest` tests a few other things other than the locale insensitive parsing of the date comment, do you think adding this `@modules` would now skip some of the other testing in this test unnecessarily? Furthermore, since the `provideLocales()` data provider method in this test has necessary checks (via `getAvailableLocales()`) to only use the non-guaranteed locales if they are available, do you think this `@modules` is still needed? - PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale [v2]
comment written out is > locale insensitive and as such when parsing using `DateTimeFormatter` a > neutral locale is appropriate on the `DateTimeFormatter` instance. This PR > thus changes the tests to use `Locale.ROOT` while parsing this date comment. > Additionally, while I was at it, I updated the `PropertiesStoreTest` to > automate the use of multiple different locales to reproduce this issue > (automatically) and verify the fix. Jaikiran Pai has updated the pull request incrementally with two additional commits since the last revision: - implement review comments - copyright years - Changes: - all: https://git.openjdk.java.net/jdk/pull/7558/files - new: https://git.openjdk.java.net/jdk/pull/7558/files/da0e4fbd..c5dd7f79 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7558=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7558=00-01 Stats: 20 lines in 2 files changed: 6 ins; 7 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7558.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7558/head:pull/7558 PR: https://git.openjdk.java.net/jdk/pull/7558
Re: RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale
On Mon, 21 Feb 2022 14:09:50 GMT, Jaikiran Pai wrote: > Can I please get a review of this test only change which fixes the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8282023? > > As noted in that JBS issue these tests fail when the default locale under > which those tests are run isn't `en_US`. Both these tests were introduced as > part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, > these test do the following: > - Use Properties.store(...) APIs to write out a properties file. This > internally ends up writing a date comment via a call to > `java.util.Date.toString()` method. > - The test then runs a few checks to make sure that the written out `Date` is > an expected one. To run these checks it uses the > `java.time.format.DateTimeFormatter` to parse the date comment out of the > properties file. > > All this works fine when the default locale is (for example) `en_US`. > However, when the default locale is (for example `en_CA` or even `hi_IN`) the > tests fail with an exception in the latter step above while parsing the date > comment using the `DateTimeFormatter` instance. The exception looks like: > > > Using locale: he for Properties#store(OutputStream) test > test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure > java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST > 2022 > at org.testng.Assert.fail(Assert.java:87) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) > at > PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) > at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) > at > org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) > at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) > at > org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) > at > org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) > at > org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) > at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) > at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) > at org.testng.TestRunner.privateRun(TestRunner.java:764) > at org.testng.TestRunner.run(TestRunner.java:585) > at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) > at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) > at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) > at org.testng.SuiteRunner.run(SuiteRunner.java:286) > at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) > at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) > at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) > at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) > at org.testng.TestNG.runSuites(TestNG.java:1069) > at org.testng.TestNG.run(TestNG.java:1037) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) > at > com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) > at > java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) > at java.base/java.lang.reflect.Method.invoke(Method.java:577) > at > com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) > at java.base/java.lang.Thread.run(Thread.java:828) > Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 > IST 2022' could not be parsed at index 0 > at > java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) > at > java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) > at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) > ... 30 more > > (in the exception above a locale with language `he` is being used) > > The root cause of this failure lies (only) within the tests - the > `DateTimeFormatter` used in the tests is locale sensitive and uses the > current (`Locale.getDefault()`) locale by default for parsing the date text. > This parsing fails because, although `Date.toString()` javadoc states nothing > about locales, it does mention the exact characters that will be used to >
Integrated: 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle
On Mon, 21 Feb 2022 12:42:37 GMT, Jaikiran Pai wrote: > Can I please get a review of this trivial change to the javadoc of > `DateTimeFormatter.getDecimalStyle()` method which fixes the typo noted in > https://bugs.openjdk.java.net/browse/JDK-8282190? This pull request has now been integrated. Changeset: e0b49629 Author: Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/e0b49629e95c98aabe8b75ec2f7528e7fb6dcffc Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle Reviewed-by: dfuchs, rriggs, lancea, iris - PR: https://git.openjdk.java.net/jdk/pull/7556
Re: RFR: 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle
On Mon, 21 Feb 2022 12:42:37 GMT, Jaikiran Pai wrote: > Can I please get a review of this trivial change to the javadoc of > `DateTimeFormatter.getDecimalStyle()` method which fixes the typo noted in > https://bugs.openjdk.java.net/browse/JDK-8282190? Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/7556
RFR: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale
Can I please get a review of this test only change which fixes the issue noted in https://bugs.openjdk.java.net/browse/JDK-8282023? As noted in that JBS issue these tests fail when the default locale under which those tests are run isn't `en_US`. Both these tests were introduced as part of https://bugs.openjdk.java.net/browse/JDK-8231640. At a high level, these test do the following: - Use Properties.store(...) APIs to write out a properties file. This internally ends up writing a date comment via a call to `java.util.Date.toString()` method. - The test then runs a few checks to make sure that the written out `Date` is an expected one. To run these checks it uses the `java.time.format.DateTimeFormatter` to parse the date comment out of the properties file. All this works fine when the default locale is (for example) `en_US`. However, when the default locale is (for example `en_CA` or even `hi_IN`) the tests fail with an exception in the latter step above while parsing the date comment using the `DateTimeFormatter` instance. The exception looks like: Using locale: he for Properties#store(OutputStream) test test PropertiesStoreTest.testStoreOutputStreamDateComment(he): failure java.lang.AssertionError: Unexpected date comment: Mon Feb 21 19:10:31 IST 2022 at org.testng.Assert.fail(Assert.java:87) at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:255) at PropertiesStoreTest.testStoreOutputStreamDateComment(PropertiesStoreTest.java:223) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) at java.base/java.lang.reflect.Method.invoke(Method.java:577) at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132) at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599) at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174) at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46) at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822) at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147) at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146) at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128) at java.base/java.util.ArrayList.forEach(ArrayList.java:1511) at org.testng.TestRunner.privateRun(TestRunner.java:764) at org.testng.TestRunner.run(TestRunner.java:585) at org.testng.SuiteRunner.runTest(SuiteRunner.java:384) at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378) at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337) at org.testng.SuiteRunner.run(SuiteRunner.java:286) at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53) at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96) at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218) at org.testng.TestNG.runSuitesLocally(TestNG.java:1140) at org.testng.TestNG.runSuites(TestNG.java:1069) at org.testng.TestNG.run(TestNG.java:1037) at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:94) at com.sun.javatest.regtest.agent.TestNGRunner.main(TestNGRunner.java:54) at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104) at java.base/java.lang.reflect.Method.invoke(Method.java:577) at com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127) at java.base/java.lang.Thread.run(Thread.java:828) Caused by: java.time.format.DateTimeParseException: Text 'Mon Feb 21 19:10:31 IST 2022' could not be parsed at index 0 at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2106) at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1934) at PropertiesStoreTest.testDateComment(PropertiesStoreTest.java:253) ... 30 more (in the exception above a locale with language `he` is being used) The root cause of this failure lies (only) within the tests - the `DateTimeFormatter` used in the tests is locale sensitive and uses the current (`Locale.getDefault()`) locale by default for parsing the date text. This parsing fails because, although `Date.toString()` javadoc states nothing about locales, it does mention the exact characters that will be used to write out the date comment. In other words, this date comment written out is locale insensitive and as such when parsing using `DateTimeFormatter` a neutral locale is appropriate on the `DateTimeFormatter` instance. This PR thus changes the tests to use `Locale.ROOT` while parsing this date comment. Additionally, while I was at it, I updated the `PropertiesStoreTest` to automate the use of multiple
RFR: 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle
Can I please get a review of this trivial change to the javadoc of `DateTimeFormatter.getDecimalStyle()` method which fixes the typo noted in https://bugs.openjdk.java.net/browse/JDK-8282190? - Commit messages: - 8282190: Typo in javadoc of java.time.format.DateTimeFormatter#getDecimalStyle Changes: https://git.openjdk.java.net/jdk/pull/7556/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7556=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282190 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7556.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7556/head:pull/7556 PR: https://git.openjdk.java.net/jdk/pull/7556
Integrated: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
On Mon, 14 Feb 2022 05:27:39 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8281634? > > The commit introduces the missing `err.invalid.filters` key in the jdeps > resource bundle. I have run a simple check to make sure this resource bundle > doesn't miss any other `err.` keys. From a simple search, following are the > unique `err.` keys used in the code of jdeps (under > `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): > > err.exception.message > err.invalid.options > err.multirelease.version.associated > err.missing.arg > err.multirelease.jar.malformed > err.option.already.specified > err.missing.dependences > err.module.not.found > err.invalid.path > err.genmoduleinfo.not.jarfile > err.invalid.arg.for.option > err.multirelease.option.notfound > err.filter.not.specified > err.unknown.option > err.command.set > err.invalid.filters > err.genmoduleinfo.unnamed.package > err.option.after.class > > Apart from the `err.invalid.filters` key which this PR is fixing, none of the > other keys are missing from the resource bundle. I haven't updated the > resource bundles for Japanese and Chinese languages because I don't know > their equivalent values and looking at the commit history of those files, it > appears that those changes are done as separate tasks (should a JBS issue be > raised for this?) > > The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been > updated to reproduce the issue and verify this fix. > > An important detail about the update to the test is that while working on the > update to this test, I realized that the current implementation in the test > isn't fully functional. As a result, this test is currently, incorrectly > considered as passing. Specifically, the test was missing a `assertTrue` call > against the ouput/error content generated by the run of the `jdeps` tool. > This PR adds that assertion. > Once that assertion is added, it started showing up 3 genuine failures. These > failures are within that test code: > - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be > a typo when this section in the test was added. The commit history of the > source of jdeps tool shows that this option was always `-jdkinternals`. This > PR fixes that part in the test. > - The test expects an error message "-R cannot be used with --inverse option" > when `-R` and `--inverse` are used together. However, at some point the > source of jdeps tool was changed (as part of > https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error > message. That changes appears to have missed changing this test case error > message and since this test has been falsely passing, it never got caught. > This PR now fixes that issue by expecting the correct error message. > - The test was expecting an error message "--list-deps and > --list-reduced-deps options are specified" when "--list-deps" was used along > with "--summary". This appears to be a copy/paste error in the test case and > wasn't caught because the test was falsely passing. This too has been fixed > in this PR. > > With the fixes in this test, the test now reproduces the original issue and > verifies the fix. I realize that this PR has changes in the test that aren't > directly related to the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are > necessary to get the test functional. However if a separate JBS issue needs > to be opened to track those changes, please do let me know and I'll address > these test changes in a separate PR. This pull request has now been integrated. Changeset: d4cd8dfe Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/d4cd8dfedbe220fb3b9a68650aba90536e9b12ee Stats: 14 lines in 2 files changed: 5 ins; 0 del; 9 mod 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters Reviewed-by: dfuchs, naoto, mchung - PR: https://git.openjdk.java.net/jdk/pull/7455
Re: RFR: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
On Mon, 14 Feb 2022 05:27:39 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8281634? > > The commit introduces the missing `err.invalid.filters` key in the jdeps > resource bundle. I have run a simple check to make sure this resource bundle > doesn't miss any other `err.` keys. From a simple search, following are the > unique `err.` keys used in the code of jdeps (under > `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): > > err.exception.message > err.invalid.options > err.multirelease.version.associated > err.missing.arg > err.multirelease.jar.malformed > err.option.already.specified > err.missing.dependences > err.module.not.found > err.invalid.path > err.genmoduleinfo.not.jarfile > err.invalid.arg.for.option > err.multirelease.option.notfound > err.filter.not.specified > err.unknown.option > err.command.set > err.invalid.filters > err.genmoduleinfo.unnamed.package > err.option.after.class > > Apart from the `err.invalid.filters` key which this PR is fixing, none of the > other keys are missing from the resource bundle. I haven't updated the > resource bundles for Japanese and Chinese languages because I don't know > their equivalent values and looking at the commit history of those files, it > appears that those changes are done as separate tasks (should a JBS issue be > raised for this?) > > The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been > updated to reproduce the issue and verify this fix. > > An important detail about the update to the test is that while working on the > update to this test, I realized that the current implementation in the test > isn't fully functional. As a result, this test is currently, incorrectly > considered as passing. Specifically, the test was missing a `assertTrue` call > against the ouput/error content generated by the run of the `jdeps` tool. > This PR adds that assertion. > Once that assertion is added, it started showing up 3 genuine failures. These > failures are within that test code: > - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be > a typo when this section in the test was added. The commit history of the > source of jdeps tool shows that this option was always `-jdkinternals`. This > PR fixes that part in the test. > - The test expects an error message "-R cannot be used with --inverse option" > when `-R` and `--inverse` are used together. However, at some point the > source of jdeps tool was changed (as part of > https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error > message. That changes appears to have missed changing this test case error > message and since this test has been falsely passing, it never got caught. > This PR now fixes that issue by expecting the correct error message. > - The test was expecting an error message "--list-deps and > --list-reduced-deps options are specified" when "--list-deps" was used along > with "--summary". This appears to be a copy/paste error in the test case and > wasn't caught because the test was falsely passing. This too has been fixed > in this PR. > > With the fixes in this test, the test now reproduces the original issue and > verifies the fix. I realize that this PR has changes in the test that aren't > directly related to the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are > necessary to get the test functional. However if a separate JBS issue needs > to be opened to track those changes, please do let me know and I'll address > these test changes in a separate PR. Thank you for the reviews, Mandy and Naoto. - PR: https://git.openjdk.java.net/jdk/pull/7455
Re: RFR: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
On Mon, 14 Feb 2022 05:27:39 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > noted in https://bugs.openjdk.java.net/browse/JDK-8281634? > > The commit introduces the missing `err.invalid.filters` key in the jdeps > resource bundle. I have run a simple check to make sure this resource bundle > doesn't miss any other `err.` keys. From a simple search, following are the > unique `err.` keys used in the code of jdeps (under > `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): > > err.exception.message > err.invalid.options > err.multirelease.version.associated > err.missing.arg > err.multirelease.jar.malformed > err.option.already.specified > err.missing.dependences > err.module.not.found > err.invalid.path > err.genmoduleinfo.not.jarfile > err.invalid.arg.for.option > err.multirelease.option.notfound > err.filter.not.specified > err.unknown.option > err.command.set > err.invalid.filters > err.genmoduleinfo.unnamed.package > err.option.after.class > > Apart from the `err.invalid.filters` key which this PR is fixing, none of the > other keys are missing from the resource bundle. I haven't updated the > resource bundles for Japanese and Chinese languages because I don't know > their equivalent values and looking at the commit history of those files, it > appears that those changes are done as separate tasks (should a JBS issue be > raised for this?) > > The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been > updated to reproduce the issue and verify this fix. > > An important detail about the update to the test is that while working on the > update to this test, I realized that the current implementation in the test > isn't fully functional. As a result, this test is currently, incorrectly > considered as passing. Specifically, the test was missing a `assertTrue` call > against the ouput/error content generated by the run of the `jdeps` tool. > This PR adds that assertion. > Once that assertion is added, it started showing up 3 genuine failures. These > failures are within that test code: > - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be > a typo when this section in the test was added. The commit history of the > source of jdeps tool shows that this option was always `-jdkinternals`. This > PR fixes that part in the test. > - The test expects an error message "-R cannot be used with --inverse option" > when `-R` and `--inverse` are used together. However, at some point the > source of jdeps tool was changed (as part of > https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error > message. That changes appears to have missed changing this test case error > message and since this test has been falsely passing, it never got caught. > This PR now fixes that issue by expecting the correct error message. > - The test was expecting an error message "--list-deps and > --list-reduced-deps options are specified" when "--list-deps" was used along > with "--summary". This appears to be a copy/paste error in the test case and > wasn't caught because the test was falsely passing. This too has been fixed > in this PR. > > With the fixes in this test, the test now reproduces the original issue and > verifies the fix. I realize that this PR has changes in the test that aren't > directly related to the issue raised in > https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are > necessary to get the test functional. However if a separate JBS issue needs > to be opened to track those changes, please do let me know and I'll address > these test changes in a separate PR. Thank you for your review, Daniel. > Maybe wait one day or two before integrating to give Mandy a chance to chime > in. Certainly. - PR: https://git.openjdk.java.net/jdk/pull/7455
RFR: 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters
Can I please get a review for this change which proposes to fix the issue noted in https://bugs.openjdk.java.net/browse/JDK-8281634? The commit introduces the missing `err.invalid.filters` key in the jdeps resource bundle. I have run a simple check to make sure this resource bundle doesn't miss any other `err.` keys. From a simple search, following are the unique `err.` keys used in the code of jdeps (under `src/jdk.jdeps/share/classes/com/sun/tools/jdeps` directory): err.exception.message err.invalid.options err.multirelease.version.associated err.missing.arg err.multirelease.jar.malformed err.option.already.specified err.missing.dependences err.module.not.found err.invalid.path err.genmoduleinfo.not.jarfile err.invalid.arg.for.option err.multirelease.option.notfound err.filter.not.specified err.unknown.option err.command.set err.invalid.filters err.genmoduleinfo.unnamed.package err.option.after.class Apart from the `err.invalid.filters` key which this PR is fixing, none of the other keys are missing from the resource bundle. I haven't updated the resource bundles for Japanese and Chinese languages because I don't know their equivalent values and looking at the commit history of those files, it appears that those changes are done as separate tasks (should a JBS issue be raised for this?) The existing `test/langtools/tools/jdeps/Options.java` jtreg test has been updated to reproduce the issue and verify this fix. An important detail about the update to the test is that while working on the update to this test, I realized that the current implementation in the test isn't fully functional. As a result, this test is currently, incorrectly considered as passing. Specifically, the test was missing a `assertTrue` call against the ouput/error content generated by the run of the `jdeps` tool. This PR adds that assertion. Once that assertion is added, it started showing up 3 genuine failures. These failures are within that test code: - The test uses `-jdkinternal` instead of `-jdkinternals`. This appears to be a typo when this section in the test was added. The commit history of the source of jdeps tool shows that this option was always `-jdkinternals`. This PR fixes that part in the test. - The test expects an error message "-R cannot be used with --inverse option" when `-R` and `--inverse` are used together. However, at some point the source of jdeps tool was changed (as part of https://bugs.openjdk.java.net/browse/JDK-8213909) to return a different error message. That changes appears to have missed changing this test case error message and since this test has been falsely passing, it never got caught. This PR now fixes that issue by expecting the correct error message. - The test was expecting an error message "--list-deps and --list-reduced-deps options are specified" when "--list-deps" was used along with "--summary". This appears to be a copy/paste error in the test case and wasn't caught because the test was falsely passing. This too has been fixed in this PR. With the fixes in this test, the test now reproduces the original issue and verifies the fix. I realize that this PR has changes in the test that aren't directly related to the issue raised in https://bugs.openjdk.java.net/browse/JDK-8281634, but those changes are necessary to get the test functional. However if a separate JBS issue needs to be opened to track those changes, please do let me know and I'll address these test changes in a separate PR. - Commit messages: - 8281634: jdeps: java.lang.InternalError: Missing message: err.invalid.filters Changes: https://git.openjdk.java.net/jdk/pull/7455/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7455=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8281634 Stats: 14 lines in 2 files changed: 5 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7455.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7455/head:pull/7455 PR: https://git.openjdk.java.net/jdk/pull/7455
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input [v2]
On Thu, 10 Feb 2022 22:08:27 GMT, Joe Darcy wrote: >> src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java >> line 197: >> >>> 195: // Predicate above covers enum constants, including >>> 196: // those with specialized class bodies. >>> 197: return toSourceString((Enum) value); >> >> Hello Joe, would it be better to use the new syntax for `instanceof` here to >> avoid the subsequent cast? >> >> >> else if (value instanceof Enum v) >> >> return toSourceString(v); > >> Hello Joe, would it be better to use the new syntax for `instanceof` here to >> avoid the subsequent cast? >> >> ``` >> else if (value instanceof Enum v) >> >> return toSourceString(v); >> ``` > > Fair point; updated in subsequent push. Thanks. Thank you for this change, Joe. Looks fine to me. - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: RFR: JDK-8281462: Annotation toString output for enum not reusable for source input
On Thu, 10 Feb 2022 05:49:47 GMT, Joe Darcy wrote: > Two changes to the toString output for annotations to give better source > fidelity: > > 1) For enum constants, call their name method rather than their toString > method. An enum class can override the toString method to print something > other than the name. > > 2) Switch from using binary names (names with "$" for nested types) to > canonical names (names with "." with nested types) > > Various existing regression tests are updated to accommodate the changes. > > Please also review the CSR: > https://bugs.openjdk.java.net/browse/JDK-8281568 src/java.base/share/classes/sun/reflect/annotation/AnnotationInvocationHandler.java line 197: > 195: // Predicate above covers enum constants, including > 196: // those with specialized class bodies. > 197: return toSourceString((Enum) value); Hello Joe, would it be better to use the new syntax for `instanceof` here to avoid the subsequent cast? else if (value instanceof Enum v) return toSourceString(v); - PR: https://git.openjdk.java.net/jdk/pull/7418
Re: Seeking inputs on 8224794: ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows
Thank you Daniel for your inputs. I'll stop any further investigation/work on this one and update the JBS issue with the details of the investigation so far. -Jaikiran On 04/02/22 10:55 pm, Daniel Fuchs wrote: Hi Jaikiran, Thanks for working on this and apologies for the long silence. I believe your analysis of the issue is very valuable. Unless there is some clever trick we could do to allow to unlink the file from the file system before deleting it, so that the file path can be reused, it seems indeed that using FILE_SHARE_DELETE doesn't buy us much benefit. It is a pity, but IMO it was well worth the investigation. Unless others on this list disagree, or can suggest other tricks, I would suggest shelving this work for now. best regards, -- daniel On 18/12/2021 06:00, Jaikiran Pai wrote: Would there be interest in moving this forward? Enabling that FILE_SHARE_DELETE option has opened up some questions that I noted in my previous mail below. So in order to move forward I would need some inputs. If we don't want to do this at this time, I'll close the draft PR that's currently open https://github.com/openjdk/jdk/pull/6477. -Jaikiran
Re: Source file launch with security manager enabled fails
On 03/02/22 7:07 pm, Alan Bateman wrote:
I think it would be useful to hear from Jon Gibbons or someone else
working on javac first. It would be a bit unusual to run the compiler
with a security manager and I thought it was deliberate to not grant
permissions to jdk.compiler in the default policy. Also the source
code launcher is aimed at the early stages of learning Java where
there shouldn't be advanced options or exotic execution modes.
To add some context on where I ran into this - I was experimenting with
security manager itself to see how the SocketChannel.bind() API behaves
when security manager was enabled. Something like this trivial code:
import java.nio.channels.*;
import java.net.*;
public class SecManager {
public static void main(final String[] args) throws Exception {
SocketChannel sc = SocketChannel.open();
System.out.println("Opened socket channel " + sc);
sc.bind(new InetSocketAddress("127.0.0.1", 23452));
System.out.println("Bound socket channel " + sc);
sc.close();
System.out.println("Closed socket channel " + sc);
}
}
I decided to use source launcher mode here and since I was experimenting
with security manager itself, I had to enable security manager.
I had a look at the JEP-330 https://openjdk.java.net/jeps/330 where this
feature was introduced but couldn't see a mention of whether
using/enabling security manager with this feature was allowed/supported.
-Jaikiran
Re: Source file launch with security manager enabled fails
Thank you Sean for the confirmation. I just filed
https://bugs.openjdk.java.net/browse/JDK-8281217 to track this.
-Jaikiran
On 03/02/22 6:55 pm, Sean Mullan wrote:
I only took a quick look, but it looks like a bug. The jdk.compiler
module needs to be granted that permission in the default.policy file.
Please file a bug, or if you like I can file one on your behalf.
Thanks,
Sean
On 2/3/22 8:01 AM, Jaikiran Pai wrote:
I'm unsure if core-libs is the right place for this or compiler-dev,
sending this to core-libs for now.
Please consider this trivial Java source file:
public class HelloWorld {
public static void main(final String[] args) throws Exception {
System.out.println("Hello World");
}
}
Running this in source file launcher mode as follows:
java HelloWorld.java
returns the expected result. However when running in source file
launcher mode *and* with security manager enabled, it throws the
following exception:
java -Djava.security.manager=default HelloWorld.java
WARNING: A command line option has enabled the Security Manager
WARNING: The Security Manager is deprecated and will be removed in a
future release
Exception in thread "main" java.security.AccessControlException: access
denied ("java.lang.RuntimePermission"
"accessClassInPackage.jdk.internal.misc")
at
java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:485)
at
java.base/java.security.AccessController.checkPermission(AccessController.java:1068)
at
java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:416)
at
java.base/java.lang.SecurityManager.checkPackageAccess(SecurityManager.java:1332)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:184)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
at
jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
This happens in Java 17 as well as latest master branch. I haven't
checked older releases but I guess it's reproducible there too.
Are users expected to use an explicit policy file to add this permission
in source file launch mode or is this missing an internal doPrivileged
call in the JDK?
As an additional input, compiling this file first using javac and then
launching it in traditional mode with security manager enabled works
fine:
javac HelloWorld.java
java -Djava.security.manager=default HelloWorld
WARNING: A command line option has enabled the Security Manager
WARNING: The Security Manager is deprecated and will be removed in a
future release
Hello World
-Jaikiran
Source file launch with security manager enabled fails
I'm unsure if core-libs is the right place for this or compiler-dev,
sending this to core-libs for now.
Please consider this trivial Java source file:
public class HelloWorld {
public static void main(final String[] args) throws Exception {
System.out.println("Hello World");
}
}
Running this in source file launcher mode as follows:
java HelloWorld.java
returns the expected result. However when running in source file
launcher mode *and* with security manager enabled, it throws the
following exception:
java -Djava.security.manager=default HelloWorld.java
WARNING: A command line option has enabled the Security Manager
WARNING: The Security Manager is deprecated and will be removed in a
future release
Exception in thread "main" java.security.AccessControlException: access
denied ("java.lang.RuntimePermission"
"accessClassInPackage.jdk.internal.misc")
at
java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:485)
at
java.base/java.security.AccessController.checkPermission(AccessController.java:1068)
at
java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:416)
at
java.base/java.lang.SecurityManager.checkPackageAccess(SecurityManager.java:1332)
at
java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:184)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
at jdk.compiler/com.sun.tools.javac.launcher.Main.main(Main.java:132)
This happens in Java 17 as well as latest master branch. I haven't
checked older releases but I guess it's reproducible there too.
Are users expected to use an explicit policy file to add this permission
in source file launch mode or is this missing an internal doPrivileged
call in the JDK?
As an additional input, compiling this file first using javac and then
launching it in traditional mode with security manager enabled works fine:
javac HelloWorld.java
java -Djava.security.manager=default HelloWorld
WARNING: A command line option has enabled the Security Manager
WARNING: The Security Manager is deprecated and will be removed in a
future release
Hello World
-Jaikiran
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.java.net/browse/JDK-8003417? > > The issue notes that this is applicable for `WeakHashMap` which have `null` > keys. However, the issue is even applicable for `WeakHashMap` instances which > don't have `null` keys, as reproduced and shown by the newly added jtreg test > case in this PR. > > The root cause of the issue is that once the iterator is used to iterate till > the end and the `remove()` is called, then the > `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the > key to remove from the map, instead of the key of the last returned entry. > The commit in this PR fixes that part. > > A new jtreg test has been added which reproduces the issue as well as > verifies the fix. > `tier1` testing and this new test have passed after this change. However, I > guess this will require a JCK run to be run too, perhaps? If so, I will need > help from someone who has access to them to have this run against those > please. > Seems like we ought to be able to fix this one. Agreed. I got caught up in a few unrelated things, so haven't been able to get back to this to take into account some of your and Roger's inputs. I'll get back to this shortly. - PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin
On Wed, 12 Jan 2022 13:45:00 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8279921? > > The change here builds upon the debugging enhancement that was done in > https://github.com/openjdk/jdk/pull/6696 to try and narrow down the problem > behind intermittent failures on macos ARM systems. The previous change caught > only `IllegalArgumentException` to dump the class file, but as seen in a > recent failure, the exception type itself isn't guaranteed to be the same > due to the nature of the issue. So, this commit catches `Exception` to dump > the class file. Additionally, the change here also prints out the underlying > `ResourcePoolEntry` type to see if that provides any additional hints on why > the class content could end being corrupt. Thank you for the review Mandy. I ran the jdk:tier3 locally to make sure nothing regressed and it went fine: == Test summary == TEST TOTAL PASS FAIL ERROR jtreg:test/jdk:tier3 1284 1284 0 0 == TEST SUCCESS - PR: https://git.openjdk.java.net/jdk/pull/7049
Integrated: 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin
On Wed, 12 Jan 2022 13:45:00 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which addresses > https://bugs.openjdk.java.net/browse/JDK-8279921? > > The change here builds upon the debugging enhancement that was done in > https://github.com/openjdk/jdk/pull/6696 to try and narrow down the problem > behind intermittent failures on macos ARM systems. The previous change caught > only `IllegalArgumentException` to dump the class file, but as seen in a > recent failure, the exception type itself isn't guaranteed to be the same > due to the nature of the issue. So, this commit catches `Exception` to dump > the class file. Additionally, the change here also prints out the underlying > `ResourcePoolEntry` type to see if that provides any additional hints on why > the class content could end being corrupt. This pull request has now been integrated. Changeset: e683d4ac Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/e683d4ac8d9ee3b0078c5e87a2b3e7d36d7344fc Stats: 21 lines in 2 files changed: 16 ins; 0 del; 5 mod 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin Reviewed-by: mchung - PR: https://git.openjdk.java.net/jdk/pull/7049
RFR: 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin
Can I please get a review for this change which addresses https://bugs.openjdk.java.net/browse/JDK-8279921? The change here builds upon the debugging enhancement that was done in https://github.com/openjdk/jdk/pull/6696 to try and narrow down the problem behind intermittent failures on macos ARM systems. The previous change caught only `IllegalArgumentException` to dump the class file, but as seen in a recent failure, the exception type itself isn't guaranteed to be the same due to the nature of the issue. So, this commit catches `Exception` to dump the class file. Additionally, the change here also prints out the underlying `ResourcePoolEntry` type to see if that provides any additional hints on why the class content could end being corrupt. - Commit messages: - 8279921: Dump the .class file in jlink debug mode for any failure during transform() of a plugin Changes: https://git.openjdk.java.net/jdk/pull/7049/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7049=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8279921 Stats: 21 lines in 2 files changed: 16 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/7049.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7049/head:pull/7049 PR: https://git.openjdk.java.net/jdk/pull/7049
Re: Seeking inputs on 8224794: ZipFile/JarFile should open zip/JAR file with FILE_SHARE_DELETE sharing mode on Windows
Would there be interest in moving this forward? Enabling that FILE_SHARE_DELETE option has opened up some questions that I noted in my previous mail below. So in order to move forward I would need some inputs. If we don't want to do this at this time, I'll close the draft PR that's currently open https://github.com/openjdk/jdk/pull/6477. -Jaikiran On 19/11/21 5:10 pm, Jaikiran Pai wrote: I have been experimenting with https://bugs.openjdk.java.net/browse/JDK-8224794 and have reached a stage with my changes where I would like some inputs before I move ahead or raise a PR for review. As noted in that JBS issue, the idea is to open the underlying file using FILE_SHARE_DELETE when using java.uitl.zip.ZipFile and java.util.jar.JarFile. From what I can infer from that JBS issue, the motivation behind this seems to be to allow deleting that underlying file when that file is currently opened and in use by the ZipFile/JarFile instance. The linked github issues seem to suggest the same. IMO, it's a reasonable expectation, given that such a deletion works on *nix platform. It's on Windows where such a deletion fails with "The process cannot access the file because it is being used by another process" (the other process in many cases will be the current JVM instance itself). So trying to get this to work on Windows by setting the FILE_SHARE_DELETE share access mode seems reasonable. Coming to the implementation part, I've an initial draft version here https://github.com/openjdk/jdk/pull/6477. It sets the FILE_SHARE_DELETE share access mode on the file when constructed by ZipFile/JarFile. It "works", in the sense that if you issue a delete against this underlying file (path) after its been opened by ZipFile/JarFile instance, you no longer see the error mentioned above and the delete completes without any exception. However, my experiments with this change have raised a bunch of questions related to this: The crucial part of the "DeleteFile" API in Windows is that, as stated in its documentation[1]: "The DeleteFile function marks a file for deletion on close. Therefore, the file deletion does not occur until the last handle to the file is closed. Subsequent calls to CreateFile to open the file fail with ERROR_ACCESS_DENIED." So imagine the following pseudo-code: String path = "C:/foo.jar" 1. JarFile jar = new JarFile(path); // create a jar instance 2. var deleted = new File(path).delete(); // delete the underlying file 3. var exists = new File(path).exists(); // check file existence 4. FileOutputStream fos = new FileOutputStream(new File(path)); // open a FileOutputStream for /tmp/foo.jar 5. java.nio.file.Files.deleteIfExists(Path.of(path)); When this is run (on Windows) *without* the change that introduces FILE_SHARE_DELETE, I notice that line#2 returns "false" for File.delete() (since the file is in use by the JarFile instance) and then line#3 returns "true" since it still exists and line#4 succeeds and creates a new FileOutputStream and line#5 fails with an exception stating "java.nio.file.FileSystemException: foo.jar: The process cannot access the file because it is being used by another process " Now when the same code is run (on Windows) *with* the proposed FILE_SHARE_DELETE changes, I notice that line#2 returns "true" (i.e. the file is considered deleted) and line#3 returns "true" (i.e. it's considered the file still exists). So essentially the file is only marked for deletion and hasn't been deleted at the OS level, since the JarFile instance still has an handle open. Then line#4 unlike previously, now throws an exception and fails to create the FileOutputStream instance. The failure says "java.nio.file.AccessDeniedException: foo.jar". This exception matches the documentation from that Windows DeleteFile API. line#5 too fails with this same AccessDeniedException. So what this means is, although we have managed to allow the deletion to happen (i.e. the first call to delete does mark it for deletion at OS level), IMO, it still doesn't improve much and raises other difference in behaviour of APIs as seen above, unless the underlying file handle held by ZipFile/JarFile is closed. So that brings me to the next related issue. For this change to be really useful, the file handle (which is an instance of RandomAccessFile) held by ZipFile/JarFile needs to be closed. If a user application explicitly creates an instance of ZipFile/JarFile, it is expected that they close it themselves. However, the most prominent use of the JarFile creation is within the JDK code where it uses the sun.net.www.protocol.jar.JarURLConnection to construct the JarFile instances, typically from the URLClassLoader. By default these JarFile instances that are created by the sun.net.www.protocol.jar.JarURLConnection are cached and the close() on those instan
Re: RFR: 8278087: Deserialization filter and filter factory property error reporting under specified [v2]
On Mon, 6 Dec 2021 16:59:41 GMT, Roger Riggs wrote: >> The effects of invalid values of `jdk.serialFilter` and >> `jdk.serialFilterFactory` properties are >> incompletely specified. The behavior for invalid values of the properties is >> different and >> use an unconventional exception type, `ExceptionInInitializerError` and >> leave the `OIF.Config` class >> uninitialized. >> >> The exceptions in the `ObjectInputFilter.Config` class initialization caused >> by invalid values of the two properties, >> either by system properties supplied on the command line or security >> properties are logged. >> The `Config` class marks either or both the filter and filter factory values >> as unusable >> and remembers the exception message. >> >> Subsequent calls to the methods that get or set the filter or filter factory >> or create >> an `ObjectInputStream` throw `java.lang.IllegalStateException` with the >> remembered exception message. >> Constructing an `ObjectInputStream` calls both `Config.getSerialFilter` and >> `Config.getSerialFilterFactory`. >> The nature of the invalid property is reported as an `IllegalStateException` >> on first use. >> >> This PR supercedes https://github.com/openjdk/jdk/pull/6508 Document that >> setting an invalid property jdk.serialFilter disables deserialization > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments to consistently identify security property names > and use the correct bug number in the test. Thank you Roger for the updates. This looks fine to me. - PR: https://git.openjdk.java.net/jdk/pull/6645
Re: RFR: 8278087: Deserialization filter and filter factory property error reporting under specified
On Mon, 6 Dec 2021 04:25:37 GMT, Jaikiran Pai wrote:
>> The effects of invalid values of `jdk.serialFilter` and
>> `jdk.serialFilterFactory` properties are
>> incompletely specified. The behavior for invalid values of the properties is
>> different and
>> use an unconventional exception type, `ExceptionInInitializerError` and
>> leave the `OIF.Config` class
>> uninitialized.
>>
>> The exceptions in the `ObjectInputFilter.Config` class initialization caused
>> by invalid values of the two properties,
>> either by system properties supplied on the command line or security
>> properties are logged.
>> The `Config` class marks either or both the filter and filter factory values
>> as unusable
>> and remembers the exception message.
>>
>> Subsequent calls to the methods that get or set the filter or filter factory
>> or create
>> an `ObjectInputStream` throw `java.lang.IllegalStateException` with the
>> remembered exception message.
>> Constructing an `ObjectInputStream` calls both `Config.getSerialFilter` and
>> `Config.getSerialFilterFactory`.
>> The nature of the invalid property is reported as an `IllegalStateException`
>> on first use.
>>
>> This PR supercedes https://github.com/openjdk/jdk/pull/6508 Document that
>> setting an invalid property jdk.serialFilter disables deserialization
>
> src/java.base/share/classes/java/io/ObjectInputFilter.java line 526:
>
>> 524: * {@code jdk.serialFilter} is defined then it is used to configure
>> the filter.
>> 525: * The filter is created as if {@link #createFilter(String)
>> createFilter} is called,
>> 526: * if the filter string is invalid the initialization fails and
>> subsequent attempts to
>
> Hello Roger,
>> if the filter string is invalid the initialization fails
>
> Should this instead be "if the filter string is invalid the initialization of
> {@code Config} class fails ..."
Please ignore this above comment. Clearly, the initialization of `Config` class
doesn't fail, but the initialization of the JVM wide serial filter fails. I
still had the older semantics of `Config` class in mind when I submitted that
above comment.
-
PR: https://git.openjdk.java.net/jdk/pull/6645
Re: RFR: 8278087: Deserialization filter and filter factory property error reporting under specified
On Wed, 1 Dec 2021 18:19:05 GMT, Roger Riggs wrote:
> The effects of invalid values of `jdk.serialFilter` and
> `jdk.serialFilterFactory` properties are
> incompletely specified. The behavior for invalid values of the properties is
> different and
> use an unconventional exception type, `ExceptionInInitializerError` and leave
> the `OIF.Config` class
> uninitialized.
>
> The exceptions in the `ObjectInputFilter.Config` class initialization caused
> by invalid values of the two properties,
> either by system properties supplied on the command line or security
> properties are logged.
> The `Config` class marks either or both the filter and filter factory values
> as unusable
> and remembers the exception message.
>
> Subsequent calls to the methods that get or set the filter or filter factory
> or create
> an `ObjectInputStream` throw `java.lang.IllegalStateException` with the
> remembered exception message.
> Constructing an `ObjectInputStream` calls both `Config.getSerialFilter` and
> `Config.getSerialFilterFactory`.
> The nature of the invalid property is reported as an `IllegalStateException`
> on first use.
>
> This PR supercedes https://github.com/openjdk/jdk/pull/6508 Document that
> setting an invalid property jdk.serialFilter disables deserialization
src/java.base/share/classes/java/io/ObjectInputFilter.java line 526:
> 524: * {@code jdk.serialFilter} is defined then it is used to configure
> the filter.
> 525: * The filter is created as if {@link #createFilter(String)
> createFilter} is called,
> 526: * if the filter string is invalid the initialization fails and
> subsequent attempts to
Hello Roger,
> if the filter string is invalid the initialization fails
Should this instead be "if the filter string is invalid the initialization of
{@code Config} class fails ..."
src/java.base/share/classes/java/io/ObjectInputFilter.java line 527:
> 525: * The filter is created as if {@link #createFilter(String)
> createFilter} is called,
> 526: * if the filter string is invalid the initialization fails and
> subsequent attempts to
> 527: * {@linkplain Config#getSerialFilter() get the filter}, {@link
> Config#setSerialFilter set a filter},
> {@link Config#setSerialFilter set a filter}
Should this instead be "{@link Config#setSerialFilter(ObjectInputFilter) set a
filter}" i.e. include the param as part of the `@link`, like in other places?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 532:
> 530: * invalid serial filter.
> 531: * If the system property {@code jdk.serialFilter} or the {@link
> java.security.Security}
> 532: * property is not set the filter can be set with
> or the {@link java.security.Security} property is not set the filter can be
> set ...
Is it intentional that the name of security property is left out here? Perhaps
this should be:
`or the {@link java.security.Security} property {@code jdk.serialFilter} is
not set the filter `
or even:
`or the {@link java.security.Security} property of the same name is not set the
filter`
src/java.base/share/classes/java/io/ObjectInputFilter.java line 751:
> 749: if (serialFilter != null) {
> 750: throw new IllegalStateException("Serial filter can
> only be set once");
> 751: } else if (invalidFilterMessage != null) {
The `invalidFilterMessage` is a `static` `final` `String` which gets
initialized during the static initialization of the class and as such doesn't
get changed after that. Do you think this lock on `serialFilterLock` is
necessary for this check or it can be moved outside this `synchronized` block?
src/java.base/share/classes/java/io/ObjectInputFilter.java line 859:
> 857: /*
> 858: * Return message for an invalid filter factory configuration
> saved from the static init.
> 859: * It can be called before the static initializer is complete
> and has set the message/null.
More of a curiosity than a review comment - Is that because the
`Config.getSerialFilterFactory()/setSerialFilterFactory(...)` could be called
from the constructor of the user configured factory class, which gets invoked
when static initialization is in progress for the `Config` class?
test/jdk/java/io/Serializable/serialFilter/InvalidGlobalFilterTest.java line 38:
> 36: /*
> 37: * @test
> 38: * @bug 8269336
Would this need an update to include the new bug id of this PR?
-
PR: https://git.openjdk.java.net/jdk/pull/6645
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Thu, 2 Dec 2021 19:04:11 GMT, Roger Riggs wrote: > fyi, with this change the JCK did not flag a failure. Thank you Roger for running those tests. - PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Wed, 1 Dec 2021 12:21:00 GMT, Athijegannathan Sundararajan wrote: >> Can I please get a review of this change which adds `jlink.debug=true` >> system property while launching `jpackage` tests? >> >> The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't >> take into account the part where the `jpackage` tool gets launched as a >> `ToolProvider` from some of these tests. This resulted in a large number of >> tests failing (across different OS) in `tier2` with errors like: >> >> >> Error: Invalid Option: [-J-Djlink.debug=true] >> >> >> In this current PR, the changed code now takes into account the possibility >> of `jpackage` being launched as a `ToolProvider` and in such cases doesn't >> add this option. To achieve this, the adding of this argument is delayed >> till when the actual execution is about to happen and thus it's now done in >> the `adjustArgumentsBeforeExecution()` method of the jpackage test framework. >> >> With this change, I have now run the `jdk:tier2` locally on a macos instance >> and the tests have all passed: >> >> >> Test results: passed: 3,821; failed: 3 >> Report written to >> jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html >> Results written to >> jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 >> Error: Some tests failed or other problems occurred. >> Finished running test 'jtreg:test/jdk:tier2' >> Test report is stored in >> build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 >> >> == >> Test summary >> == >>TEST TOTAL PASS FAIL ERROR >> jtreg:test/jdk:tier2 3824 3821 3 0 << >> == >> >> The 3 failing tests are unrelated to this change and belong to the >> `java/nio/channels/DatagramChannel/` test package. >> Furthermore, I've looked into the generated logs of the following tests to >> verify that the `-J-Djlink.debug=true` does get passed in relevant tests and >> doesn't in those that failed previously in `tier2`: >> >> test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java >> test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java >> test/jdk/tools/jpackage/share/ArgumentsTest.java >> >> A sample from one of the logs where this system property is expected to be >> passed along: >> >>> TRACE: exec: Execute >>> [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input >>> ./test/input --dest ./test/output --name "Name With Space" --type pkg >>> --main-jar hello.jar --main-class Hello -J-Djlink.debug=true >>> --verbose](15); inherit I/O... >> >> >> I would still like to request someone with access to CI or other OSes (like >> Windows and Linux) to help test `tier2` against this PR. > > LGTM Thank you for the review @sundararajana - PR: https://git.openjdk.java.net/jdk/pull/6534
Integrated: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Wed, 24 Nov 2021 08:54:08 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which adds `jlink.debug=true` system > property while launching `jpackage` tests? > > The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't > take into account the part where the `jpackage` tool gets launched as a > `ToolProvider` from some of these tests. This resulted in a large number of > tests failing (across different OS) in `tier2` with errors like: > > > Error: Invalid Option: [-J-Djlink.debug=true] > > > In this current PR, the changed code now takes into account the possibility > of `jpackage` being launched as a `ToolProvider` and in such cases doesn't > add this option. To achieve this, the adding of this argument is delayed till > when the actual execution is about to happen and thus it's now done in the > `adjustArgumentsBeforeExecution()` method of the jpackage test framework. > > With this change, I have now run the `jdk:tier2` locally on a macos instance > and the tests have all passed: > > > Test results: passed: 3,821; failed: 3 > Report written to > jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html > Results written to > jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 > Error: Some tests failed or other problems occurred. > Finished running test 'jtreg:test/jdk:tier2' > Test report is stored in > build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >>> jtreg:test/jdk:tier2 3824 3821 3 0 << > == > > The 3 failing tests are unrelated to this change and belong to the > `java/nio/channels/DatagramChannel/` test package. > Furthermore, I've looked into the generated logs of the following tests to > verify that the `-J-Djlink.debug=true` does get passed in relevant tests and > doesn't in those that failed previously in `tier2`: > > test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java > test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java > test/jdk/tools/jpackage/share/ArgumentsTest.java > > A sample from one of the logs where this system property is expected to be > passed along: > >> TRACE: exec: Execute >> [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input >> ./test/input --dest ./test/output --name "Name With Space" --type pkg >> --main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); >> inherit I/O... > > > I would still like to request someone with access to CI or other OSes (like > Windows and Linux) to help test `tier2` against this PR. This pull request has now been integrated. Changeset: 09522db5 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/09522db5aa9503131381bbb4fe3f2eae829645ce Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures Reviewed-by: sundar - PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Wed, 24 Nov 2021 08:54:08 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which adds `jlink.debug=true` system > property while launching `jpackage` tests? > > The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't > take into account the part where the `jpackage` tool gets launched as a > `ToolProvider` from some of these tests. This resulted in a large number of > tests failing (across different OS) in `tier2` with errors like: > > > Error: Invalid Option: [-J-Djlink.debug=true] > > > In this current PR, the changed code now takes into account the possibility > of `jpackage` being launched as a `ToolProvider` and in such cases doesn't > add this option. To achieve this, the adding of this argument is delayed till > when the actual execution is about to happen and thus it's now done in the > `adjustArgumentsBeforeExecution()` method of the jpackage test framework. > > With this change, I have now run the `jdk:tier2` locally on a macos instance > and the tests have all passed: > > > Test results: passed: 3,821; failed: 3 > Report written to > jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html > Results written to > jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 > Error: Some tests failed or other problems occurred. > Finished running test 'jtreg:test/jdk:tier2' > Test report is stored in > build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 > > == > Test summary > == >TEST TOTAL PASS FAIL ERROR > >>> jtreg:test/jdk:tier2 3824 3821 3 0 << > == > > The 3 failing tests are unrelated to this change and belong to the > `java/nio/channels/DatagramChannel/` test package. > Furthermore, I've looked into the generated logs of the following tests to > verify that the `-J-Djlink.debug=true` does get passed in relevant tests and > doesn't in those that failed previously in `tier2`: > > test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java > test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java > test/jdk/tools/jpackage/share/ArgumentsTest.java > > A sample from one of the logs where this system property is expected to be > passed along: > >> TRACE: exec: Execute >> [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input >> ./test/input --dest ./test/output --name "Name With Space" --type pkg >> --main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); >> inherit I/O... > > > I would still like to request someone with access to CI or other OSes (like > Windows and Linux) to help test `tier2` against this PR. Any help reviewing this please? - PR: https://git.openjdk.java.net/jdk/pull/6534
Re: RFR: 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime
On Tue, 30 Nov 2021 14:28:05 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which fixes a typo in the javadoc > of `java.util.zip.ZipEntry.setTime()` method? Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/6615
Integrated: 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime
On Tue, 30 Nov 2021 14:28:05 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which fixes a typo in the javadoc > of `java.util.zip.ZipEntry.setTime()` method? This pull request has now been integrated. Changeset: 0a01baaf Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/0a01baaf2dd31a0fe2bc8b1327fb072cc3909eeb Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime Reviewed-by: alanb, iris, lancea - PR: https://git.openjdk.java.net/jdk/pull/6615
Re: RFR: 8276766: Enable jar and jmod to produce deterministic timestamped content [v9]
Hello Andrew,
On 30/11/21 7:32 pm, Andrew Leonard wrote:
On Mon, 29 Nov 2021 16:22:30 GMT, Andrew Leonard wrote:
Add a new --source-date (epoch seconds) option to jar and jmod to
allow specification of time to use for created/updated jar/jmod entries. This then
allows the ability to make the content deterministic.
However, it looks like this behavior to not set extended mtime within the
xdostime range has just been changed by a recent PR:
https://github.com/openjdk/jdk/pull/5486/files which has changed jartool.Main
using zipEntry.setTime(time) to use zipEntry.setLastModifiedTime(time), the
later sets mtime always regardless of xdostime.
When I changed the implementation in sun.tools.jar.Main to use
setLastModifiedTime(FileTime) instead of the previous setTime(long), I
hadn't paid close attention to the implementation of these methods in
java.util.zip.ZipEntry. Now that you brought this up, I had a more
closer look at their implementation.
So it looks like the implementation of setTime(long) conditionally sets
the extended timestamp fields in optional extra data of the zip entry.
That condition checks to see if the time value being set is before 1980
or after 2099 and if it passes this condition, it goes and sets the
extended timestamp fields. So in practice, for jar creation/updation in
sun.tools.jar.Main, the previous code using setTime(long) wouldn't have
set the extended timestamp fields, because the current year(s) aren't
before 1980 or after 2099. The implementation of
java.util.zip.ZipEntry.setLastModifiedTime(FileTime) on the other hand
has no such conditional checks and (as noted in the javadoc of that
method) goes ahead and sets the time value in the extended timestamp
fields of that zip entry.
So yes, the change that went in to
https://github.com/openjdk/jdk/pull/5486 did introduce this subtle
change in the generated zip/jar entries. Having said that, the
setTime(long) on java.util.zip.ZipEntry makes no mention of these
conditional checks. It currently states:
* Sets the last modification time of the entry.
*
* If the entry is output to a ZIP file or ZIP file formatted
* output stream the last modification time set by this method will
* be stored into the {@code date and time fields} of the zip file
* entry and encoded in standard {@code MS-DOS date and time format}.
* The {@link java.util.TimeZone#getDefault() default TimeZone} is
* used to convert the epoch time to the MS-DOS data and time.
So it doesn't even make a mention of setting extended timestamp fields,
let along setting them on a particular condition. So I'm unsure whether
switching to setLastModifiedTime(FileTime) instead of setTime(long) was
a bad thing.
The implications of https://bugs.openjdk.java.net/browse/JDK-8073497 might also
apply to FileTime unnecessary initialization slowing VM startup, however if
FileTime is already regularly referenced during startup, then it wont.. If this
is the case then way forward (1) would be ok...
@AlanBateman was that an intentional change? @jaikiran
I had a look at that JBS issue now. From what I understand in its
description and goal, the idea was to prevent initialization of
java.util.Date utilities very early on during the startup. I had a quick
look at the code in ZipEntry and how it behaves when it constructs a
ZipEntry instance out of the zip file data. With the change in
https://github.com/openjdk/jdk/pull/5486, the "mtime" (which represents
extended timestamp field) will be set in the zip file entry, so there
would be an attempt to create a FileTime out of it. The call that does
that appears to be "unixTimeToFileTime" in ZipEntry and as far as I can
see it doesn't end up using any java.util.Date classes. Of course, I
haven't yet looked or experimented to verify and be absolutely sure
about it, but from a cursory check it doesn't look like this would
contribute to pulling in java.util.Date utilities.
-Jaikiran
RFR: 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime
Can I please get a review for this change which fixes a typo in the javadoc of `java.util.zip.ZipEntry.setTime()` method? - Commit messages: - 8277986: Typo in javadoc of java.util.zip.ZipEntry#setTime Changes: https://git.openjdk.java.net/jdk/pull/6615/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6615=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277986 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6615.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6615/head:pull/6615 PR: https://git.openjdk.java.net/jdk/pull/6615
Re: RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
On Sat, 20 Nov 2021 10:08:41 GMT, Jaikiran Pai wrote: > Can I please get a review for this change which proposes to fix the issue > reported in https://bugs.openjdk.java.net/browse/JDK-8003417? > > The issue notes that this is applicable for `WeakHashMap` which have `null` > keys. However, the issue is even applicable for `WeakHashMap` instances which > don't have `null` keys, as reproduced and shown by the newly added jtreg test > case in this PR. > > The root cause of the issue is that once the iterator is used to iterate till > the end and the `remove()` is called, then the > `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the > key to remove from the map, instead of the key of the last returned entry. > The commit in this PR fixes that part. > > A new jtreg test has been added which reproduces the issue as well as > verifies the fix. > `tier1` testing and this new test have passed after this change. However, I > guess this will require a JCK run to be run too, perhaps? If so, I will need > help from someone who has access to them to have this run against those > please. Any reviews for this change, please? - PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
On 26/11/21 7:32 am, Mandy Chung wrote: On Thu, 25 Nov 2021 02:22:20 GMT, Jaikiran Pai wrote: Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: minor fix to avoid casting Hello Mandy, one small issue - `ClassFileReader`, `DependencyFinder` and `VersionHelper` are misisng a copyright year update for this change. @jaikiran Sorry, I missed your comment about the copyright year update. I will update them in other jdeps fix. That sounds fine, Mandy. Thank you. -Jaikiran
Re: RFR: 8277165: jdeps --multi-release --print-module-deps fails if module-info.class in different versioned directories [v2]
On Wed, 24 Nov 2021 17:06:41 GMT, Mandy Chung wrote: >> jdeps intends to report an error if there are multiple versions of the same >> class being parsed. module-info.class should be excluded from such >> detection. >> >> This patch also fixes a data race in `VersionHelper::set` and also unwraps >> the `ExecutionException` when FutureTask of parsing the classes throws an >> exception to report `MultiReleaseException` properly. > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > minor fix to avoid casting Hello Mandy, one small issue - `ClassFileReader`, `DependencyFinder` and `VersionHelper` are misisng a copyright year update for this change. - PR: https://git.openjdk.java.net/jdk/pull/6530
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
On Wed, 24 Nov 2021 15:04:35 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with three additional > commits since the last revision: > > - review suggestion - format code in ModuleInfoEntry to use 4 space > indentation > - review suggestion - change javadoc to mention module-info.class instead of > module descriptor > - review suggestion - use if/else instead of ternary operator Thank you everyone for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/5486
Integrated: 8258117: jar tool sets the time stamp of module-info.class entries to the current time
On Mon, 13 Sep 2021 05:35:23 GMT, Jaikiran Pai wrote: > The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. This pull request has now been integrated. Changeset: a81e4fc0 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/a81e4fc07b654a3cc954921981d9d3c0cfd8bcec Stats: 285 lines in 2 files changed: 263 ins; 0 del; 22 mod 8258117: jar tool sets the time stamp of module-info.class entries to the current time Reviewed-by: lancea, ihse, alanb - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v9]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request incrementally with three additional commits since the last revision: - review suggestion - format code in ModuleInfoEntry to use 4 space indentation - review suggestion - change javadoc to mention module-info.class instead of module descriptor - review suggestion - use if/else instead of ternary operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/6f1c1b62..bdc741ca Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=07-08 Stats: 12 lines in 1 file changed: 4 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Hello Andy,
On 24/11/21 7:23 pm, Andy Herrick wrote:
never mind my previous email, addArgument("--java-option ...") ,
would be for argument to the java launching the app.
you are right to use -J-Djlink.debug=true . It must be interpreted in
the standard launcher used for jpackage, because the -J-D arg never
gets to the jpackage java code and the System Property "jlink.debug"
is already set to true by then.
Thank you for the clarification and verifying the value. I did a similar
(manual) check and like you say, the system property does rightly make
it to the JLinkTask.
-Jaikiran
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v8]
On Wed, 24 Nov 2021 14:06:29 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 13 commits: > > - Merge latest from master branch > - use proper FileTime centric APIs > - review suggestion - use FileTime instead of Long to prevent any potential > NPEs during unboxing > - review suggestion - split into multiple statements to make it easily > readable > - review suggestion - Use Path.of instead of Paths.get in testcase > - review suggestion - store e.getValue() and reuse the stored value > - Merge latest from master branch > - use testng asserts > - Remove "final" usage from test > - convert test to testng > - ... and 3 more: > https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 Thank you Lance and Magnus for the latest reviews and the testing. A recent commit to master in this area meant that there was a merge conflict in this PR. I have now resolved that and pushed the merge to this PR (no other changes). The test continues to pass. Alan, do these latest round of changes around `FileTime` usage that you requested, look fine? - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v8]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 13 commits: - Merge latest from master branch - use proper FileTime centric APIs - review suggestion - use FileTime instead of Long to prevent any potential NPEs during unboxing - review suggestion - split into multiple statements to make it easily readable - review suggestion - Use Path.of instead of Paths.get in testcase - review suggestion - store e.getValue() and reuse the stored value - Merge latest from master branch - use testng asserts - Remove "final" usage from test - convert test to testng - ... and 3 more: https://git.openjdk.java.net/jdk/compare/cf7adae6...6f1c1b62 - Changes: https://git.openjdk.java.net/jdk/pull/5486/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5486=07 Stats: 278 lines in 2 files changed: 259 ins; 0 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
RFR: 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Can I please get a review of this change which adds `jlink.debug=true` system property while launching `jpackage` tests? The previous fix for this in https://github.com/openjdk/jdk/pull/6491 didn't take into account the part where the `jpackage` tool gets launched as a `ToolProvider` from some of these tests. This resulted in a large number of tests failing (across different OS) in `tier2` with errors like: Error: Invalid Option: [-J-Djlink.debug=true] In this current PR, the changed code now takes into account the possibility of `jpackage` being launched as a `ToolProvider` and in such cases doesn't add this option. To achieve this, the adding of this argument is delayed till when the actual execution is about to happen and thus it's now done in the `adjustArgumentsBeforeExecution()` method of the jpackage test framework. With this change, I have now run the `jdk:tier2` locally on a macos instance and the tests have all passed: Test results: passed: 3,821; failed: 3 Report written to jdk/build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2/html/report.html Results written to jdk/build/macosx-x86_64-server-release/test-support/jtreg_test_jdk_tier2 Error: Some tests failed or other problems occurred. Finished running test 'jtreg:test/jdk:tier2' Test report is stored in build/macosx-x86_64-server-release/test-results/jtreg_test_jdk_tier2 == Test summary == TEST TOTAL PASS FAIL ERROR >> jtreg:test/jdk:tier2 3824 3821 3 0 << == The 3 failing tests are unrelated to this change and belong to the `java/nio/channels/DatagramChannel/` test package. Furthermore, I've looked into the generated logs of the following tests to verify that the `-J-Djlink.debug=true` does get passed in relevant tests and doesn't in those that failed previously in `tier2`: test/jdk/tools/jpackage/share/MultiLauncherTwoPhaseTest.java test/jdk/tools/jpackage/macosx/NameWithSpaceTest.java test/jdk/tools/jpackage/share/ArgumentsTest.java A sample from one of the logs where this system property is expected to be passed along: > TRACE: exec: Execute > [jdk/build/macosx-x86_64-server-release/images/jdk/bin/jpackage --input > ./test/input --dest ./test/output --name "Name With Space" --type pkg > --main-jar hello.jar --main-class Hello -J-Djlink.debug=true --verbose](15); > inherit I/O... I would still like to request someone with access to CI or other OSes (like Windows and Linux) to help test `tier2` against this PR. - Commit messages: - 8277647: [REDO] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures Changes: https://git.openjdk.java.net/jdk/pull/6534/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6534=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277647 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6534.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6534/head:pull/6534 PR: https://git.openjdk.java.net/jdk/pull/6534
Re: Integrated: 8277649 [BACKOUT] JDK-8277507 Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Tue, 23 Nov 2021 15:08:43 GMT, Daniel D. Daugherty wrote: > This reverts commit 12f08ba4d47cb70a0629b17bc3639ce170309f21. > > The fix for JDK-8277507 is causing 38 failures per Tier2 job set. Sorry, I should really have had run that original PR through `tier2` from someone before merging it. - PR: https://git.openjdk.java.net/jdk/pull/6523
Integrated: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Sun, 21 Nov 2021 12:39:17 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to add more > diagnostics when a failure occurs in the jlink tool during the jpackage tests? > > As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 > failures have been reported in jpackage tests (across different test cases) > with the same underlying cause coming from the jlink tool. Since this failure > isn't specific to one or two tests and seems to be keep happening across > these tests in `test/jdk/tools/jpackage/`, I decided to set this system > property from one central location in the `HelloApp` which gets used by these > tests. These failures have only been reported on macos (and specifically > aarch64), but the commit here doesn't do any OS name checks, to keep this > change simple. Furthermore, looking at the > `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property will > _not_ generate any additional logs unless there's an exception thrown by the > `jlink` tool, in which case it prints the exception stacktrace. So enabling > this by default won't end up increasing the log output or flooding the logs. > > With this change, I have run the 3 tests noted in those issues locally to > make sure this doesn't break anything else. I have also verified manually > that enabling this system property does indeed get propagated to the > `JlinkTask` and checked the logs to see that the command line does pass it: > >> >> [17:51:07.123] TRACE: exec: Execute >> [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input >> --dest ./test/output --name "Name With Space" --type pkg >> -J-Djlink.debug=true --main-jar hello.jar --main-class Hello --verbose](15); >> inherit I/O... >> [17:51:07.364] Building PKG package for Name With Space. >> [17:51:19.191] Command [PID: -1]: >> jlink --output >> /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name >> With Space.app/Contents/runtime/Contents/Home --module-path >> macosx-x86_64-server-release/images/jdk/jmods --add-modules >> java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,jd k.incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata --strip-native-commands --strip-debug --no-man-pages --no-header-files >> [17:51:19.192] Output: >> WARNING: Using incubator modules: jdk.incubator.foreign, >> jdk.incubator.vector >> >> [17:51:19.192] Returned: 0 >> > > These tests get run in `tier2` and I have no way of running the entire > `tier2` locally or relying of GitHub actions which only runs `tier1`. If this > change looks OK and is approved, then I would like to request for help > running the entire tests against this PR before merging it. This pull request has now been integrated. Changeset: 12f08ba4 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/12f08ba4d47cb70a0629b17bc3639ce170309f21 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures Reviewed-by: almatvee - PR: https://git.openjdk.java.net/jdk/pull/6491
Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Tue, 23 Nov 2021 04:57:57 GMT, Alexander Matveev wrote: >> Can I please get a review of this change which proposes to add more >> diagnostics when a failure occurs in the jlink tool during the jpackage >> tests? >> >> As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 >> failures have been reported in jpackage tests (across different test cases) >> with the same underlying cause coming from the jlink tool. Since this >> failure isn't specific to one or two tests and seems to be keep happening >> across these tests in `test/jdk/tools/jpackage/`, I decided to set this >> system property from one central location in the `HelloApp` which gets used >> by these tests. These failures have only been reported on macos (and >> specifically aarch64), but the commit here doesn't do any OS name checks, to >> keep this change simple. Furthermore, looking at the >> `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property >> will _not_ generate any additional logs unless there's an exception thrown >> by the `jlink` tool, in which case it prints the exception stacktrace. So >> enabling this by default won't end up increasing the log output or flooding >> the logs. >> >> With this change, I have run the 3 tests noted in those issues locally to >> make sure this doesn't break anything else. I have also verified manually >> that enabling this system property does indeed get propagated to the >> `JlinkTask` and checked the logs to see that the command line does pass it: >> >>> >>> [17:51:07.123] TRACE: exec: Execute >>> [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input >>> --dest ./test/output --name "Name With Space" --type pkg >>> -J-Djlink.debug=true --main-jar hello.jar --main-class Hello >>> --verbose](15); inherit I/O... >>> [17:51:07.364] Building PKG package for Name With Space. >>> [17:51:19.191] Command [PID: -1]: >>> jlink --output >>> /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name >>> With Space.app/Contents/runtime/Contents/Home --module-path >>> macosx-x86_64-server-release/images/jdk/jmods --add-modules >>> java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,j dk.incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata --strip-native-commands --strip-debug --no-man-pages --no-header-files >>> [17:51:19.192] Output: >>> WARNING: Using incubator modules: jdk.incubator.foreign, >>> jdk.incubator.vector >>> >>> [17:51:19.192] Returned: 0 >>> >> >> These tests get run in `tier2` and I have no way of running the entire >> `tier2` locally or relying of GitHub actions which only runs `tier1`. If >> this change looks OK and is approved, then I would like to request for help >> running the entire tests against this PR before merging it. > > Marked as reviewed by almatvee (Reviewer). Thank you for the review @sashamatveev. For a moment I was thinking whether I should withdraw this PR because your manual run already narrowed down the exception stacktrace. But I will go ahead with the merge so as to allow any future failures to generate this root cause information and see if it consistently fails due to the same error. - PR: https://git.openjdk.java.net/jdk/pull/6491
Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Mon, 22 Nov 2021 08:14:20 GMT, Alan Bateman wrote: > It might be that jlink is throwing IAE for cases where another exception is > more appropriate, but it does suggests something intermittent in the jpackage > tests to trigger it. Issue https://bugs.openjdk.java.net/browse/JDK-8277058 now contains a comment which includes the underlying exception stacktrace when that particular test was run using -Djlink.debug=true manually. - PR: https://git.openjdk.java.net/jdk/pull/6491
Re: RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
On Sun, 21 Nov 2021 12:39:17 GMT, Jaikiran Pai wrote: > Can I please get a review of this change which proposes to add more > diagnostics when a failure occurs in the jlink tool during the jpackage tests? > > As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 > failures have been reported in jpackage tests (across different test cases) > with the same underlying cause coming from the jlink tool. Since this failure > isn't specific to one or two tests and seems to be keep happening across > these tests in `test/jdk/tools/jpackage/`, I decided to set this system > property from one central location in the `HelloApp` which gets used by these > tests. These failures have only been reported on macos (and specifically > aarch64), but the commit here doesn't do any OS name checks, to keep this > change simple. Furthermore, looking at the > `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property will > _not_ generate any additional logs unless there's an exception thrown by the > `jlink` tool, in which case it prints the exception stacktrace. So enabling > this by default won't end up increasing the log output or flooding the logs. > > With this change, I have run the 3 tests noted in those issues locally to > make sure this doesn't break anything else. I have also verified manually > that enabling this system property does indeed get propagated to the > `JlinkTask` and checked the logs to see that the command line does pass it: > >> >> [17:51:07.123] TRACE: exec: Execute >> [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input >> --dest ./test/output --name "Name With Space" --type pkg >> -J-Djlink.debug=true --main-jar hello.jar --main-class Hello --verbose](15); >> inherit I/O... >> [17:51:07.364] Building PKG package for Name With Space. >> [17:51:19.191] Command [PID: -1]: >> jlink --output >> /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name >> With Space.app/Contents/runtime/Contents/Home --module-path >> macosx-x86_64-server-release/images/jdk/jmods --add-modules >> java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,jd k.incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata --strip-native-commands --strip-debug --no-man-pages --no-header-files >> [17:51:19.192] Output: >> WARNING: Using incubator modules: jdk.incubator.foreign, >> jdk.incubator.vector >> >> [17:51:19.192] Returned: 0 >> > > These tests get run in `tier2` and I have no way of running the entire > `tier2` locally or relying of GitHub actions which only runs `tier1`. If this > change looks OK and is approved, then I would like to request for help > running the entire tests against this PR before merging it. Thank you Alan. I'll wait for someone from jpackage area to take a look at this change. - PR: https://git.openjdk.java.net/jdk/pull/6491
Re: RFR: 8277322: Document that setting an invalid property `jdk.serialFilter` disables deserialization
On Mon, 22 Nov 2021 19:57:25 GMT, Roger Riggs wrote:
> The effects of an invalid `jdk.serialFilter` property are not completely
> documented. If the value of the system property jdk.serialFilter is invalid,
> deserialization should not be possible and it should be clear in the
> specification.
>
> Specify an implementation specific exception is thrown in the case where
> deserialization is invoked after reporting the invalid jdk.serialFilter.
src/java.base/share/classes/java/io/ObjectInputFilter.java line 530:
> 528: * and the initialization fails; subsequent attempts to use the
> configuration or
> 529: * serialization will fail with an implementation specific exception.
> 530: * If the system property {@code jdk.serialFilter} is not set on the
> command line
Hello Roger,
Thank you for rearranging these lines. It reads much more clearly. One tiny
final question - this new line now states `If the system property {@code
jdk.serialFilter} is not set on the command line it can be set with `.
However, this property if not set on the command line could have instead been
set as a `java.security.Security` property (in a file). The javadoc does
mention this a few lines back. So do you think this new line should be reworded
to something like `If the filter is neither set as a system property on the
command line nor as a security property then it can be set with...`
-
PR: https://git.openjdk.java.net/jdk/pull/6508
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v7]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: use proper FileTime centric APIs - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/8ff75835..9ab5ad8a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=05-06 Stats: 12 lines in 1 file changed: 1 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
On Mon, 22 Nov 2021 09:09:37 GMT, Jaikiran Pai wrote:
>> test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java
>> line 60:
>>
>>> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4";
>>> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo";
>>> 60: private static final Path MODULE_CLASSES_DIR =
>>> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();
>>
>> You can use Path.of here.
>
> Done. Replaced this and one other place in this test to use `Path.of`.
Test continues to pass with all these new changes.
-
PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
On Mon, 22 Nov 2021 08:25:38 GMT, Alan Bateman wrote:
>> Jaikiran Pai 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 seven additional
>> commits since the last revision:
>>
>> - Merge latest from master branch
>> - use testng asserts
>> - Remove "final" usage from test
>> - convert test to testng
>> - Merge latest from master branch
>> - Merge latest from master branch
>> - 8258117: jar tool sets the time stamp of module-info.class entries to the
>> current time
>
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 806:
>
>> 804: Long lastModified = f.lastModified() == 0 ? null :
>> f.lastModified();
>> 805: moduleInfos.putIfAbsent(name,
>> 806: new StreamedModuleInfoEntry(name,
>> Files.readAllBytes(f.toPath()), lastModified));
>
> I'd prefer to see this split into two two statements as there is just too
> much going on one the one line.
Done. I have updated the PR to split this line into more than 1 statement.
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 1785:
>
>> 1783: private final String name;
>> 1784: private final byte[] bytes;
>> 1785: private final Long lastModifiedTime;
>
> It might be better to use a FileTime here, otherwise we risk a NPE when
> unboxing.
Sounds good. I've updated the PR to replace this to use `FileTime`.
> src/jdk.jartool/share/classes/sun/tools/jar/Main.java line 2108:
>
>> 2106: byte[] extended = extendedInfoBytes(md, bytes, packages);
>> 2107: // replace the entry value with the extended bytes
>> 2108: e.setValue(new
>> StreamedModuleInfoEntry(e.getValue().name(), extended,
>> e.getValue().getLastModifiedTime()));
>
> There are 3 calls to getValue and each one I need to remember that e.getValue
> is a ModuleInfoEntry. If you add ModuleInfo entry = e.getValue() then it
> might help the readability.
That makes sense. The updated PR now has this change.
> test/jdk/tools/jar/modularJar/JarToolModuleDescriptorReproducibilityTest.java
> line 60:
>
>> 58: private static final String UPDATED_MODULE_VERSION = "1.2.4";
>> 59: private static final String MAIN_CLASS = "jdk.test.foo.Foo";
>> 60: private static final Path MODULE_CLASSES_DIR =
>> Paths.get("8258117-module-classes", MODULE_NAME).toAbsolutePath();
>
> You can use Path.of here.
Done. Replaced this and one other place in this test to use `Path.of`.
-
PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v6]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai has updated the pull request incrementally with four additional commits since the last revision: - review suggestion - use FileTime instead of Long to prevent any potential NPEs during unboxing - review suggestion - split into multiple statements to make it easily readable - review suggestion - Use Path.of instead of Paths.get in testcase - review suggestion - store e.getValue() and reuse the stored value - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/945fde6f..8ff75835 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=04-05 Stats: 21 lines in 2 files changed: 3 ins; 1 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
RFR: 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures
Can I please get a review of this change which proposes to add more diagnostics when a failure occurs in the jlink tool during the jpackage tests? As noted in https://bugs.openjdk.java.net/browse/JDK-8277507, so far 3 failures have been reported in jpackage tests (across different test cases) with the same underlying cause coming from the jlink tool. Since this failure isn't specific to one or two tests and seems to be keep happening across these tests in `test/jdk/tools/jpackage/`, I decided to set this system property from one central location in the `HelloApp` which gets used by these tests. These failures have only been reported on macos (and specifically aarch64), but the commit here doesn't do any OS name checks, to keep this change simple. Furthermore, looking at the `jdk.tools.jlink.internal.JlinkTask` code, enabling this system property will _not_ generate any additional logs unless there's an exception thrown by the `jlink` tool, in which case it prints the exception stacktrace. So enabling this by default won't end up increasing the log output or flooding the logs. With this change, I have run the 3 tests noted in those issues locally to make sure this doesn't break anything else. I have also verified manually that enabling this system property does indeed get propagated to the `JlinkTask` and checked the logs to see that the command line does pass it: > > [17:51:07.123] TRACE: exec: Execute > [macosx-x86_64-server-release/images/jdk/bin/jpackage --input ./test/input > --dest ./test/output --name "Name With Space" --type pkg -J-Djlink.debug=true > --main-jar hello.jar --main-class Hello --verbose](15); inherit I/O... > [17:51:07.364] Building PKG package for Name With Space. > [17:51:19.191] Command [PID: -1]: > jlink --output > /var/folders/7v/cnkwrnx154926cr3289w4rd8gp/T/jdk.jpackage13608930262477285540/images/image-3713034571561734902/Name > With Space.app/Contents/runtime/Contents/Home --module-path > macosx-x86_64-server-release/images/jdk/jmods --add-modules > java.rmi,jdk.management.jfr,jdk.jdi,jdk.charsets,jdk.xml.dom,java.xml,java.datatransfer,jdk.jstatd,jdk.httpserver,java.desktop,java.security.sasl,jdk.zipfs,java.base,jdk.crypto.ec,jdk.javadoc,jdk.management.agent,jdk.jshell,jdk.editpad,jdk.jsobject,java.sql.rowset,jdk.sctp,jdk.unsupported,jdk.jlink,java.smartcardio,java.security.jgss,jdk.nio.mapmode,java.compiler,jdk.dynalink,jdk.unsupported.desktop,jdk.accessibility,jdk.security.jgss,jdk.incubator.vector,java.sql,java.xml.crypto,java.logging,java.transaction.xa,jdk.jfr,jdk.crypto.cryptoki,jdk.net,jdk.random,java.naming,jdk.internal.ed,java.prefs,java.net.http,jdk.compiler,jdk.internal.opt,jdk.naming.rmi,jdk.jconsole,jdk.attach,jdk.internal.le,java.management,jdk.jdwp.agent,jdk .incubator.foreign,jdk.internal.jvmstat,java.instrument,jdk.management,jdk.security.auth,java.scripting,jdk.jdeps,jdk.jartool,jdk.jpackage,java.management.rmi,jdk.naming.dns,jdk.localedata --strip-native-commands --strip-debug --no-man-pages --no-header-files > [17:51:19.192] Output: > WARNING: Using incubator modules: jdk.incubator.foreign, > jdk.incubator.vector > > [17:51:19.192] Returned: 0 > These tests get run in `tier2` and I have no way of running the entire `tier2` locally or relying of GitHub actions which only runs `tier1`. If this change looks OK and is approved, then I would like to request for help running the entire tests against this PR before merging it. - Commit messages: - 8277507: Add jlink.debug system property while launching jpackage tests to help diagonize recent intermittent failures Changes: https://git.openjdk.java.net/jdk/pull/6491/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6491=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277507 Stats: 4 lines in 1 file changed: 4 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6491.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6491/head:pull/6491 PR: https://git.openjdk.java.net/jdk/pull/6491
Re: RFR: JDK-8277451: java.lang.reflect.Field::set on static field with invalid argument type should throw IAE
On Sat, 20 Nov 2021 19:13:43 GMT, Mandy Chung wrote:
> java.lang.reflect.Field::set on static field with invalid argument type
> should throw IAE. But this regression is introduced by JEP 416 throwing NPE
> instead.
>
> `ensureObj` is called as the first check of the `Field::set` method to ensure
> the receiver object is checked first before the argument. For a Field
> instance with write-access, the method handle invocation will check the
> receiver. Therefore for `Field::setXXX` methods to set a primitive value,
> `ensureObj` is only called if it's a read-only Field instance to ensure
> IllegalArgumentException is thrown first before IllegalAccessException to
> keep the existing behavior to avoid duplicated receiver check.
src/java.base/share/classes/jdk/internal/reflect/MethodHandleFieldAccessorImpl.java
line 72:
> 70: */
> 71: protected IllegalArgumentException
> newGetIllegalArgumentException(Object o) {
> 72: return new IllegalArgumentException(getMessage(true, o != null ?
> o.getClass().getName() : ""));
Hello Mandy,
With this change the `getMessage` may get passed an empty string, so it would
end up printing something like `Can not get ... field on`. Do you
think the `getMessage` implementation should be tweaked not to print the "on"
if the `attemptedType` is empty?
-
PR: https://git.openjdk.java.net/jdk/pull/6490
RFR: 8003417: WeakHashMap$HashIterator removes wrong entry
Can I please get a review for this change which proposes to fix the issue reported in https://bugs.openjdk.java.net/browse/JDK-8003417? The issue notes that this is applicable for `WeakHashMap` which have `null` keys. However, the issue is even applicable for `WeakHashMap` instances which don't have `null` keys, as reproduced and shown by the newly added jtreg test case in this PR. The root cause of the issue is that once the iterator is used to iterate till the end and the `remove()` is called, then the `WeakHashMap$HashIterator#remove()` implementation used to pass `null` as the key to remove from the map, instead of the key of the last returned entry. The commit in this PR fixes that part. A new jtreg test has been added which reproduces the issue as well as verifies the fix. `tier1` testing and this new test have passed after this change. However, I guess this will require a JCK run to be run too, perhaps? If so, I will need help from someone who has access to them to have this run against those please. - Commit messages: - 8003417: WeakHashMap$HashIterator removes wrong entry Changes: https://git.openjdk.java.net/jdk/pull/6488/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6488=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8003417 Stats: 164 lines in 2 files changed: 162 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6488.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6488/head:pull/6488 PR: https://git.openjdk.java.net/jdk/pull/6488
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v4]
On Fri, 5 Nov 2021 13:52:49 GMT, Jaikiran Pai wrote: >> The commit here is a potential fix for the issue noted in >> https://bugs.openjdk.java.net/browse/JDK-8258117. >> >> The change here repurposes an existing internal interface `ModuleInfoEntry` >> to keep track of the last modified timestamp of a `module-info.class` >> descriptor. >> >> This commit uses the timestamp of the `module-info.class` on the filesystem >> to set the time on the `JarEntry`. There are a couple of cases to consider >> here: >> >> 1. When creating a jar (using `--create`), we use the source >> `module-info.class` from the filesystem and then add extended info to it >> (attributes like packages, module version etc...). In such cases, this patch >> will use the lastmodified timestamp from the filesystem of >> `module-info.class` even though we might end up updating/extending/modifying >> (for example by adding a module version) its content while storing it as a >> `JarEntry`. >> >> 2. When updating a jar (using `--update`), this patch will use the >> lastmodified timestamp of `module-info.class` either from the filesystem or >> from the source jar's entry (depending on whether a new `module-info.class` >> is being passed to the command). Here too, it's possible that we might end >> up changing/modifying/extending the `module-info.class` (for example, >> changing the module version to a new version) that gets written into the >> updated jar file, but this patch _won't_ use `System.currentTimeMillis()` >> even in such cases. >> >> If we do have to track actual changes that might happen to >> `module-info.class` while extending its info (in `extendedInfoBytes()`) and >> then decide whether to use current system time as last modified time, then >> this will require a bigger change and also a discussion on what kind of >> extending of module-info.class content will require a change to the >> lastmodifiedtime of that entry. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - use testng asserts > - Remove "final" usage from test Hello Lance, > Hi Jaikiran, > > I am OK with moving forward. Thank you for the review. > You might give it a couple more days before you push in the event we get > additional feedback (realizing the PR has been open for a while) > > Thank you for your efforts and patience on this. Certainly. I won't integrate this until this next Tuesday. In the meantime, given that this has been open for a while now and new commits have made it to master, I will go ahead and merge the latest master changes just to be sure nothing surprisingly breaks. - PR: https://git.openjdk.java.net/jdk/pull/5486
Re: RFR: 8258117: jar tool sets the time stamp of module-info.class entries to the current time [v5]
> The commit here is a potential fix for the issue noted in > https://bugs.openjdk.java.net/browse/JDK-8258117. > > The change here repurposes an existing internal interface `ModuleInfoEntry` > to keep track of the last modified timestamp of a `module-info.class` > descriptor. > > This commit uses the timestamp of the `module-info.class` on the filesystem > to set the time on the `JarEntry`. There are a couple of cases to consider > here: > > 1. When creating a jar (using `--create`), we use the source > `module-info.class` from the filesystem and then add extended info to it > (attributes like packages, module version etc...). In such cases, this patch > will use the lastmodified timestamp from the filesystem of > `module-info.class` even though we might end up updating/extending/modifying > (for example by adding a module version) its content while storing it as a > `JarEntry`. > > 2. When updating a jar (using `--update`), this patch will use the > lastmodified timestamp of `module-info.class` either from the filesystem or > from the source jar's entry (depending on whether a new `module-info.class` > is being passed to the command). Here too, it's possible that we might end up > changing/modifying/extending the `module-info.class` (for example, changing > the module version to a new version) that gets written into the updated jar > file, but this patch _won't_ use `System.currentTimeMillis()` even in such > cases. > > If we do have to track actual changes that might happen to > `module-info.class` while extending its info (in `extendedInfoBytes()`) and > then decide whether to use current system time as last modified time, then > this will require a bigger change and also a discussion on what kind of > extending of module-info.class content will require a change to the > lastmodifiedtime of that entry. Jaikiran Pai 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 seven additional commits since the last revision: - Merge latest from master branch - use testng asserts - Remove "final" usage from test - convert test to testng - Merge latest from master branch - Merge latest from master branch - 8258117: jar tool sets the time stamp of module-info.class entries to the current time - Changes: - all: https://git.openjdk.java.net/jdk/pull/5486/files - new: https://git.openjdk.java.net/jdk/pull/5486/files/2c0246f9..945fde6f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5486=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5486=03-04 Stats: 48509 lines in 1041 files changed: 34004 ins; 7147 del; 7358 mod Patch: https://git.openjdk.java.net/jdk/pull/5486.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5486/head:pull/5486 PR: https://git.openjdk.java.net/jdk/pull/5486
