Re: Discussion: easier Stream closing
Hello, Brian! Thanks for your thoughts. > The consumeAndClose approach is clever, in that it adds one API point that > works for all streams, rather than having to add a new API point for every > factory of a closeable stream; on the other hand, it is dramatically less > discoverable, and so requires more education to get people to use it (and, as > you say, has the exception problem.) I think this could be solved via IDE/static analysis features. It looks quite easy to statically rewrite the try-catch statement to consumeAndClose. And, of course, static analyzers may recommend consumeAndClose when no resource management is used at all on the stream that is known to hold the resource. I filed https://bugs.openjdk.java.net/browse/JDK-8274412 to move this forward.
Integrated: 8231640: (prop) Canonical property storage
On Sat, 4 Sep 2021 15:25:59 GMT, Jaikiran Pai wrote: > The commit in this PR implements the proposal for enhancement that was > discussed in the core-libs-dev mailing list recently[1], for > https://bugs.openjdk.java.net/browse/JDK-8231640 > > At a high level - the `store()` APIs in `Properties` have been modified to > now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env > variable is set, then instead of writing out the current date time as a date > comment, the `store()` APIs instead will use the value set for this env > variable to parse it to a `Date` and write out the string form of such a > date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date > format and `Locale.ROOT` to format and write out such a date. This should > provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. Furthermore, > intentionally, no changes in the date format of the "current date" have been > done. > > These modified `store()` APIs work in the presence of the `SecurityManager` > too. The caller is expected to have a read permission on the > `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that > permission, then the implementation of these `store()` APIs will write out > the "current date" and will ignore any value that has been set for the > `SOURCE_DATE_EPOCH` env variable. This should allow for backward > compatibility of existing applications, where, when they run under a > `SecurityManager` and perhaps with an existing restrictive policy file, the > presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the `store()` > APIs. > > The modified `store()` APIs will also ignore any value for > `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such cases, > the `store()` APIs will write out the "current date" and ignore the value set > for this environment variable. No exceptions will be thrown for such invalid > values. This is an additional backward compatibility precaution to prevent > any rogue value for `SOURCE_DATE_EPOCH` from breaking applications. > > An additional change in the implementation of these `store()` APIs and > unrelated to the date comment, is that these APIs will now write out the > property keys in a deterministic order. The keys will be written out in the > natural ordering as specified by `java.lang.String#compareTo()` API. > > The combination of the ordering of the property keys when written out and the > usage of `SOURCE_DATE_EPOCH` environment value to determine the date comment > should together allow for reproducibility of the output generated by these > `store()` APIs. > > New jtreg test classes have been introduced to verify these changes. The > primary focus of `PropertiesStoreTest` is the ordering aspects of the > property keys that are written out. On the other hand > `StoreReproducibilityTest` focuses on the reproducibility of the output > generated by these APIs. The `StoreReproducibilityTest` runs these tests > both in the presence and absence of `SecurityManager`. Plus, in the presence > of SecurityManager, it tests both the scenarios where the caller is granted > the requisite permission and in other case not granted that permission. > > These new tests and existing tests under `test/jdk/java/util/Properties/` > pass with these changes. > > [1] > https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html > [2] https://reproducible-builds.org/specs/source-date-epoch/ This pull request has now been integrated. Changeset: af50772d Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/af50772d39a063652895e79d474da6ebb992cae0 Stats: 837 lines in 4 files changed: 822 ins; 0 del; 15 mod 8231640: (prop) Canonical property storage Reviewed-by: rriggs, smarks, dfuchs, ihse - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8231640: (prop) Canonical property storage [v22]
On Wed, 22 Sep 2021 10:27:45 GMT, Jaikiran Pai wrote: >> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert "Implement Mark's suggestion in CSR to include the > java.properties.date in the list of system properties listed in > System::getProperties()" > >Additional inputs since this specific commit was introduced have leaned > towards not listing this property in System::getProperties() > >This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. > - reduce the line length of code comment It was a pleasure working on this one due to the timely and valuable reviews, inputs and suggestions. Thanks to everyone for helping in getting this done. - PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Sat, 25 Sep 2021 03:38:24 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-8273790? >> >> As noted in that issue, trying to class load >> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` >> concurrently in separate threads can lead to a deadlock because of the >> cyclic nature of the code in their static initialization. More specifically, >> consider this case: >> >> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. >> - This gives T1 the implicit class init lock on `CalendarSystem`. >> - Consider thread T2 which at the same time initiates a classload on >> `sun.util.calendar.Gregorian` class. >> - This gives T2 a implicit class init lock on `Gregorian`. >> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` >> since it wants to create a (singleton) instance of `Gregorian` and assign it >> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class >> init lock on `Gregorian`, T1 ends up waiting >> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` >> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, >> T2 starts travelling up the class hierarchy and asks for a lock on >> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up >> waiting on T1 which is waiting on T2. That triggers this deadlock. >> >> The linked JBS issue has a thread dump which shows this in action. >> >> The commit here delays the instance creation of `Gregorian` by moving that >> instance creation logic from the static initialization of the >> `CalendarSystem` class, to the first call to >> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` >> from needing a lock on `Gregorian` during its static init (of course, unless >> some code in this static init flow calls >> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square >> one. I have verified, both manually and through the jtreg test, that the >> code in question doesn't have such calls) >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. The test in addition to loading these 2 classes in question, also >> additionally loads a few other classes concurrently. These classes have >> specific static initialization which leads the calls to >> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. >> Including these classes in the tests ensures that this deadlock hasn't >> "moved" to a different location. I have run multiple runs (approximately 25) >> of this test with the fix and I haven't seen it deadlock anymore. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor test adjustments as suggested by Naoto > - use a holder instead of volatile, as suggested by Roger Thank you Roger, Naoto and Yi Yang for the reviews. - PR: https://git.openjdk.java.net/jdk/pull/5683
Integrated: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem
On Fri, 24 Sep 2021 14:36:07 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-8273790? > > As noted in that issue, trying to class load > `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` > concurrently in separate threads can lead to a deadlock because of the cyclic > nature of the code in their static initialization. More specifically, > consider this case: > > - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. > - This gives T1 the implicit class init lock on `CalendarSystem`. > - Consider thread T2 which at the same time initiates a classload on > `sun.util.calendar.Gregorian` class. > - This gives T2 a implicit class init lock on `Gregorian`. > - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` > since it wants to create a (singleton) instance of `Gregorian` and assign it > to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class > init lock on `Gregorian`, T1 ends up waiting > - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` > itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, > T2 starts travelling up the class hierarchy and asks for a lock on > `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up > waiting on T1 which is waiting on T2. That triggers this deadlock. > > The linked JBS issue has a thread dump which shows this in action. > > The commit here delays the instance creation of `Gregorian` by moving that > instance creation logic from the static initialization of the > `CalendarSystem` class, to the first call to > `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` > from needing a lock on `Gregorian` during its static init (of course, unless > some code in this static init flow calls > `CalendarSystem#getGregorianCalendar()`, in which case it is back to square > one. I have verified, both manually and through the jtreg test, that the code > in question doesn't have such calls) > > A new jtreg test has been introduced to reproduce the issue and verify the > fix. The test in addition to loading these 2 classes in question, also > additionally loads a few other classes concurrently. These classes have > specific static initialization which leads the calls to > `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. > Including these classes in the tests ensures that this deadlock hasn't > "moved" to a different location. I have run multiple runs (approximately 25) > of this test with the fix and I haven't seen it deadlock anymore. This pull request has now been integrated. Changeset: ddc26274 Author:Jaikiran Pai URL: https://git.openjdk.java.net/jdk/commit/ddc262746aea99050b9a6484f51c7ddb8f4bc991 Stats: 183 lines in 2 files changed: 179 ins; 0 del; 4 mod 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem Reviewed-by: naoto, yyang, rriggs - PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang wrote:
>> 4890732: GZIPOutputStream doesn't support optional GZIP fields
>
> Lin Zang has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 17 commits:
>
> - Merge branch 'master' into gzip-field
> - delete trailing spaces
> - refine code
> - Merge branch 'master' into gzip-field
> - change since version to 18
> - Merge branch 'master' into gzip-field
> - Merge branch 'master' into gzip-field
> - Add api in GZIPInputStream to get header data
> - Merge remote-tracking branch 'upstream/master' into gzip-field
> - remove trailing spaces
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 65:
> 63: * Add extra field in GZIP file header.
> 64: * This method verifies the extra fileds layout per RFC-1952.
> 65: * See comments of {@code verifyExtraFieldLayout}
You should specify the layout here than refer to a private method, as private
methods don't appear in generated online javadocs
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 186:
> 184: * @since 18
> 185: */
> 186: public byte[] generateBytes(byte cm,
Is there any reason to leave this public? If this is just an implementation
detail, this should be kept private as public apis are maintenance burdens that
are risky to change (hence csrs)
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 247:
> 245: *+=+
> 246: */
> 247: byte[] filenameBytes = filename.getBytes("ISO-8859-1");
Should use
[`StandardCharsets.ISO_8859_1`](https://github.com/openjdk/jdk/blob/8876eae42993d4425ba9886dde94b08f6101a257/src/java.base/share/classes/java/nio/charset/StandardCharsets.java#L55)
than string names. Using the charset object is significantly faster than
looking up charsets by string names, see
https://bugs.openjdk.java.net/browse/JDK-8272120
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 328:
> 326: String fileComment,
> 327: int headerCRC16,
> 328: byte[] headerBytes) {
Need to override the getter for byte array fields to return copies than
original arrays to prevent modifications. For the extraFieldBytes one, the call
to copy needs a null check too.
src/java.base/share/classes/java/util/zip/GZIPHeaderBuilder.java line 359:
> 357: private static final int FEXTRA = 4;// Extra field
> 358: private static final int FNAME = 8;// File name
> 359: private static final int FCOMMENT = 16; // File comment
Since the `flags` record component is public, These probably should be left
public as well to benefit users, especially given checking the flags can
replace null checks for `extraFieldBytes`, `filename`, and `fileContent`.
-
PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 4890732: GZIPOutputStream doesn't support optional GZIP fields [v12]
On Mon, 6 Sep 2021 07:20:11 GMT, Lin Zang wrote: >> 4890732: GZIPOutputStream doesn't support optional GZIP fields > > Lin Zang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 17 commits: > > - Merge branch 'master' into gzip-field > - delete trailing spaces > - refine code > - Merge branch 'master' into gzip-field > - change since version to 18 > - Merge branch 'master' into gzip-field > - Merge branch 'master' into gzip-field > - Add api in GZIPInputStream to get header data > - Merge remote-tracking branch 'upstream/master' into gzip-field > - remove trailing spaces > - ... and 7 more: > https://git.openjdk.java.net/jdk/compare/c640fe42...196caedb Dear All， This PR has been pending there for quite a long time. I am wondering maybe this PR is not so interesting? I would like to leave this PR open for a while more, and if no new update, I would let it close automatically by timeout. Thanks! Lin - PR: https://git.openjdk.java.net/jdk/pull/3072
Re: RFR: 8269559: AArch64: Implement string_compare intrinsic in SVE [v2]
On Mon, 23 Aug 2021 21:48:01 GMT, TatWai Chong wrote: >> This patch implements string_compare intrinsic in SVE. >> It supports all LL, LU, UL and UU comparisons. >> >> As we haven't found an existing benchmark to measure performance impact, >> we created a benchmark derived from the test [1] for this evaluation. >> This benchmark is attached to this patch. >> >> Besides, remove the unused temporary register `vtmp3` from the existing >> match rules for StrCmp. >> >> The result below shows all varients can be benefited largely. >> Command: make exploded-test TEST="micro:StringCompareToDifferentLength" >> >> Benchmark(size) Mode Cnt Score Speedup Units >> compareToLL 24 avgt 10 1.0x ms/op >> compareToLL 36 avgt 10 1.0x ms/op >> compareToLL 72 avgt 10 1.0x ms/op >> compareToLL 128 avgt 10 1.4x ms/op >> compareToLL 256 avgt 10 1.8x ms/op >> compareToLL 512 avgt 10 2.7x ms/op >> compareToLU 24 avgt 10 1.6x ms/op >> compareToLU 36 avgt 10 1.8x ms/op >> compareToLU 72 avgt 10 2.3x ms/op >> compareToLU 128 avgt 10 3.8x ms/op >> compareToLU 256 avgt 10 4.7x ms/op >> compareToLU 512 avgt 10 6.3x ms/op >> compareToUL 24 avgt 10 1.6x ms/op >> compareToUL 36 avgt 10 1.7x ms/op >> compareToUL 72 avgt 10 2.2x ms/op >> compareToUL 128 avgt 10 3.3x ms/op >> compareToUL 256 avgt 10 4.4x ms/op >> compareToUL 512 avgt 10 6.1x ms/op >> compareToUU 24 avgt 10 1.0x ms/op >> compareToUU 36 avgt 10 1.0x ms/op >> compareToUU 72 avgt 10 1.4x ms/op >> compareToUU 128 avgt 10 2.2x ms/op >> compareToUU 256 avgt 10 2.6x ms/op >> compareToUU 512 avgt 10 3.7x ms/op >> >> [1] >> https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/intrinsics/string/TestStringCompareToDifferentLength.java > > TatWai Chong has updated the pull request incrementally with one additional > commit since the last revision: > > Restore the removal of vtmp3 (=V2) as it is still used by the non-SVE > compare-long-strings stub. > > And remove the assertion in `string_compare` since it won't help as the > registers > used in the stub are fixed. @theRealAph Hello, Andrew. I'm wondering are you interested in reviewing this patch? I believe your feedback and comment are useful to it. - PR: https://git.openjdk.java.net/jdk/pull/5129
Re: RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code [v2]
> macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set > the name of the application in the system menu bar. > > Because this set shortly after the VM is running, it causes a thread safety > issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 > > Since the AWT already looks for the system property > "apple.awt.application.name" for this same purpose, > we can set that instead of the env. var and avoid the threading issue. > > This trades the JAVA_MAIN_CLASS_ pollution of the environment for > setting a system property which is visible to apps as well but it seems a > reasonable trade-off since we already (implicitly) support this variable > anyway in preference to the env. var. Phil Race has updated the pull request incrementally with one additional commit since the last revision: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - Changes: - all: https://git.openjdk.java.net/jdk/pull/5724/files - new: https://git.openjdk.java.net/jdk/pull/5724/files/232bfae4..93108c59 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5724=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5724=00-01 Stats: 5 lines in 1 file changed: 0 ins; 4 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
Integrated: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. This pull request has now been integrated. Changeset: c880b87a Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/c880b87a205cc9611fe88cb29f506293dfebf946 Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod 8274367: Re-indent stack-trace examples for Throwable.printStackTrace Reviewed-by: mchung, iris, darcy, bpb - PR: https://git.openjdk.java.net/jdk/pull/5715
Re: RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5715
Re: RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. Marked as reviewed by darcy (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5715
RFR: 8217501: Matcher.hitEnd returns false for incomplete surrogate pairs
Fixing a bug where character matcher doesn't mark hitEnd as true if it's a code point with more than one character. - Commit messages: - 8217501: Matcher.hitEnd returns false for incomplete surrogate pairs Changes: https://git.openjdk.java.net/jdk/pull/5725/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5725=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8217501 Stats: 26 lines in 2 files changed: 26 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5725.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5725/head:pull/5725 PR: https://git.openjdk.java.net/jdk/pull/5725
Re: RFR: 8231640: (prop) Canonical property storage [v22]
On Wed, 22 Sep 2021 10:27:45 GMT, Jaikiran Pai wrote: >> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert "Implement Mark's suggestion in CSR to include the > java.properties.date in the list of system properties listed in > System::getProperties()" > >Additional inputs since this specific commit was introduced have leaned > towards not listing this property in System::getProperties() > >This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. > - reduce the line length of code comment I agree with Roger. It's hard to understand the amount of work you have put into this when you only look at the small, resulting patch. Thank you for pulling this through! - PR: https://git.openjdk.java.net/jdk/pull/5372
RFR: 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code
macOS launcher code sets JAVA_MAIN_CLASS_ which is read by AWT to set the name of the application in the system menu bar. Because this set shortly after the VM is running, it causes a thread safety issue described in https://bugs.openjdk.java.net/browse/JDK-8270549 Since the AWT already looks for the system property "apple.awt.application.name" for this same purpose, we can set that instead of the env. var and avoid the threading issue. This trades the JAVA_MAIN_CLASS_ pollution of the environment for setting a system property which is visible to apps as well but it seems a reasonable trade-off since we already (implicitly) support this variable anyway in preference to the env. var. - Commit messages: - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code - 8274397: [macOS] Stop setting env. var JAVA_MAIN_CLASS_ in launcher code Changes: https://git.openjdk.java.net/jdk/pull/5724/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5724=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274397 Stats: 75 lines in 2 files changed: 33 ins; 33 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/5724.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5724/head:pull/5724 PR: https://git.openjdk.java.net/jdk/pull/5724
Re: Discussion: easier Stream closing
In Java 8, I think we were reluctant to lean on the idiom of "pass me a
lambda and I'll pass it the confined data"), because Java developers
were already struggling to understand lambdas. But now that we're
mostly over that hurdle, API points that accept Consumer are
a powerful way to gain confinement (as we've seen in StackWalker, and
also in the explorations of ScopeLocal in Loom.) So I have no objection
to this idiom.
I have some concern that putting these side-by-side with existing
Files.walk(...) methods will be a source of confusion, creating two
different ways to do the same thing.
I would rather not add anything new to BaseStream; it was a mistake to
expose at all, and I'd rather see it deprecated than add more to it.
However, adding it individually to the Stream implementations is
equivalent, and doesn't have this problem.
The consumeAndClose approach is clever, in that it adds one API point
that works for all streams, rather than having to add a new API point
for every factory of a closeable stream; on the other hand, it is
dramatically less discoverable, and so requires more education to get
people to use it (and, as you say, has the exception problem.)
On 9/26/2021 5:27 AM, Tagir Valeev wrote:
Hello!
With current NIO file API, a very simple problem to get the list of
all files in the directory requires some ceremony:
List paths;
try (Stream stream = Files.list(Path.of("/etc"))) {
paths = stream.toList();
}
If we skip try-with-resources, we may experience OS file handles leak,
so it's desired to keep it. Yet, it requires doing boring stuff. In
this sense, classic new File("/etc").list() was somewhat more
convenient (despite its awful error handling).
I like how this problem is solved in StackWalker API [1]: it limits
the lifetime of the Stream by requiring a user-specified function to
consume it. After the function is applied, the stream is closed
automatically. We could add a similar overload to the Files API. As an
additional feature, we could also translate all UncheckedIOException's
back to IOException for more uniform exception processing:
/**
* @param dir The path to the directory
* @param function function to apply to the stream of directory files
* @param type of the result
* @return result of the function
* @throws IOException if an I/O error occurs when opening the directory, or
* UncheckedIOException is thrown by the supplied function.
*/
public static T list(Path dir, Function, ?
extends T> function) throws IOException {
try (Stream stream = Files.list(dir)) {
return function.apply(stream);
}
catch (UncheckedIOException exception) {
throw exception.getCause();
}
}
In this case, getting the List of all files in the directory will be
as simple as
List paths = Files.list(Path.of("/etc"), Stream::toList);
This doesn't limit the flexibility. For example, if we need only file
names instead of full paths, we can do this:
List paths = Files.list(Path.of("/etc"), s ->
s.map(Path::getFileName).toList());
Alternatively, we could enhance the BaseStream interface in a similar
way. It won't allow us to translate exceptions, but could be useful
for every stream that must be closed after consumption:
// in BaseStream:
/**
* Apply a given function to this stream, then close the stream.
* No further operation on the stream will be possible after that.
*
* @param function function to apply
* @param type of the function result
* @return result of the function
* @see #close()
*/
default R consumeAndClose(Function function) {
try(@SuppressWarnings("unchecked") S s = (S) this) {
return function.apply(s);
}
}
The usage would be a little bit longer but still more pleasant than
explicit try-with-resources:
List list = Files.list(Path.of("/etc")).consumeAndClose(Stream::toList);
On the other hand, in this case, we are free to put intermediate
operations outside of consumeAndClose, so we won't need to nest
everything inside the function. Only terminal operation should be
placed inside the consumeAndClose. E.g., if we need file names only,
like above, we can do this:
List list =
Files.list(Path.of("/etc")).map(Path::getFileName).consumeAndClose(Stream::toList);
What do you think?
[1]
https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#walk(java.util.function.Function)
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Mon, 27 Sep 2021 17:21:09 GMT, Mandy Chung wrote: > This reminds to double check [1] and realized that `java.lang.reflect` is not > in the list of trusted packages for `@Stable`. I'll add that in JEP 416. > > [1] > https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L219 Gotcha, I misremembered it as-if all packages under java/lang were already considered to be in that list. - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Mon, 27 Sep 2021 19:59:00 GMT, Mandy Chung wrote: > One thing I'm puzzling is why the performance result is better but > `java.lang.reflect` is not in the TNSFF package list. I think for deciding if a field can be treated as constant, `@Stable` has equal weight as a final in a TNSFF package, see https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L287 Adding java/lang/reflect to the trusted package list seem reasonable, though. - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Mon, 27 Sep 2021 11:23:43 GMT, Claes Redestad wrote: > A stray thought is why not most fields in `Field`/`Method`/`Constructor` are > `final`, as my IDE suggests. I can't find any hotspot code that injects > values to these fields. Experimentally changing most fields in `Field` to > final seem to pass at least all the java/lang/reflect tests. Since this is a > trusted package `@Stable` should then be pointless. This is a good observation. I took another look.The only place to set those fields are in `Reflection:new_field/new_method/new_constructor`. Making the fields that set by the Method/Field/Constructor constructor be final is a good idea! Those final fields don't need to be `@Stable`. One thing I'm puzzling is why the performance result is better but `java.lang.reflect` is not in the TNSFF package list. - PR: https://git.openjdk.java.net/jdk/pull/5694
Integrated: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
On Mon, 27 Sep 2021 18:51:37 GMT, Joe Darcy wrote: > Follow-up change to JDK-8231442, augmentations to javac's Xlint:serial > checking are out for review (#5709) and java.sql.rowset would need some > changes to pass under the expanded checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. This pull request has now been integrated. Changeset: 5b660f33 Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/5b660f3347917201902d501f371530a97c768977 Stats: 18 lines in 7 files changed: 13 ins; 0 del; 5 mod 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset Reviewed-by: bpb, lancea - PR: https://git.openjdk.java.net/jdk/pull/5719
Re: RFR: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset [v2]
> Follow-up change to JDK-8231442, augmentations to javac's Xlint:serial > checking are out for review (#5709) and java.sql.rowset would need some > changes to pass under the expanded checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update copyrights. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5719/files - new: https://git.openjdk.java.net/jdk/pull/5719/files/70747989..f0faf468 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5719=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5719=00-01 Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/5719.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5719/head:pull/5719 PR: https://git.openjdk.java.net/jdk/pull/5719
RFR: 8274394: Use Optional.isEmpty instead of !Optional.isPresent in jdk.jlink
I propose to replace usages of !Optional.isPresent() with Optional.isEmpty method. It's makes code a bit easier to read. Noticing negation before long chain of method calls is hard. - Commit messages: - [PATCH] Use Optional.isEmpty instead of !Optional.isPresent in jdk.jlink Changes: https://git.openjdk.java.net/jdk/pull/5707/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5707=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274394 Stats: 8 lines in 5 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/5707.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5707/head:pull/5707 PR: https://git.openjdk.java.net/jdk/pull/5707
Re: RFR: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
On Mon, 27 Sep 2021 18:51:37 GMT, Joe Darcy wrote: > Follow-up change to JDK-8231442, augmentations to javac's Xlint:serial > checking are out for review (#5709) and java.sql.rowset would need some > changes to pass under the expanded checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5719
Re: RFR: 8266936: Add a finalization JFR event [v14]
On Sat, 25 Sep 2021 14:22:28 GMT, Markus Grönlund wrote: >> Greetings, >> >> Object.finalize() was deprecated in JDK9. There is an ongoing effort to >> replace and mitigate Object.finalize() uses in the JDK libraries; please see >> https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. >> >> We also like to assist users in replacing and mitigating uses in non-JDK >> code. >> >> Hence, this changeset adds a periodic JFR event to help identify which >> classes are overriding Object.finalize(). >> >> Thanks >> Markus > > Markus Grönlund has updated the pull request incrementally with one > additional commit since the last revision: > > symbols-unix I reviewed the change in java.base that looks good. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
On Mon, 27 Sep 2021 18:51:37 GMT, Joe Darcy wrote: > Follow-up change to JDK-8231442, augmentations to javac's Xlint:serial > checking are out for review (#5709) and java.sql.rowset would need some > changes to pass under the expanded checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5719
Re: RFR: 8274391: Suppress more warnings on non-serializable non-transient instance fields in java.util.concurrent
On Mon, 27 Sep 2021 18:40:10 GMT, Joe Darcy wrote: > Follow-up change to JDK-8232230, augmentations to javac's Xlint:serial > checking are out for review (https://github.com/openjdk/jdk/pull/5709) and > java.util.concurrent would need some changes to pass under the expanded > checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. > > In ForkJoinTask, the SuppressWarnings annotation previously applied in > JDK-8232230, seems to have been misplaced to a different field; this change > corrects it. Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5718
Re: RFR: 8274391: Suppress more warnings on non-serializable non-transient instance fields in java.util.concurrent
On Mon, 27 Sep 2021 18:40:10 GMT, Joe Darcy wrote: > Follow-up change to JDK-8232230, augmentations to javac's Xlint:serial > checking are out for review (https://github.com/openjdk/jdk/pull/5709) and > java.util.concurrent would need some changes to pass under the expanded > checks. > > The changes are to suppress warnings where non-transient fields in > serializable types are not declared with a type statically known to be > serializable. That isn't necessarily a correctness issues, but it does merit > further scrutiny. > > In ForkJoinTask, the SuppressWarnings annotation previously applied in > JDK-8232230, seems to have been misplaced to a different field; this change > corrects it. Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5718
RFR: 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset
Follow-up change to JDK-8231442, augmentations to javac's Xlint:serial checking are out for review (#5709) and java.sql.rowset would need some changes to pass under the expanded checks. The changes are to suppress warnings where non-transient fields in serializable types are not declared with a type statically known to be serializable. That isn't necessarily a correctness issues, but it does merit further scrutiny. - Commit messages: - 8274392: Suppress more warnings on non-serializable non-transient instance fields in java.sql.rowset Changes: https://git.openjdk.java.net/jdk/pull/5719/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5719=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274392 Stats: 13 lines in 7 files changed: 13 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5719.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5719/head:pull/5719 PR: https://git.openjdk.java.net/jdk/pull/5719
RFR: 8274391: Suppress more warnings on non-serializable non-transient instance fields in java.util.concurrent
Follow-up change to JDK-8232230, augmentations to javac's Xlint:serial checking are out for review (https://github.com/openjdk/jdk/pull/5709) and java.util.concurrent would need some changes to pass under the expanded checks. The changes are to suppress warnings where non-transient fields in serializable types are not declared with a type statically known to be serializable. That isn't necessarily a correctness issues, but it does merit further scrutiny. In ForkJoinTask, the SuppressWarnings annotation previously applied in JDK-8232230, seems to have been misplaced to a different field; this change corrects it. - Commit messages: - 8274391: Suppress more warnings on non-serializable non-transient instance fields in java.util.concurrent Changes: https://git.openjdk.java.net/jdk/pull/5718/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5718=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274391 Stats: 3 lines in 2 files changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5718.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5718/head:pull/5718 PR: https://git.openjdk.java.net/jdk/pull/5718
Re: RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5715
Re: RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
On Mon, 27 Sep 2021 12:04:48 GMT, Pavel Rappo wrote: > This PR fixes indentation in the examples in the doc comment for > java.lang.Throwable#printStackTrace(). > > 1. Indentation in stack-trace output is significant. The cause of an > exception is output on the same level of indentation as that of the > exception. A suppressed exception is output at a deeper level of indentation > than that of the containing exception. The last example for > Throwable.printStackTrace violates this. > > 2. Indentation in examples for Throwable.printStackTrace that relate to > suppressed exceptions is inconsistent with that of cause exceptions. Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5715
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Sat, 25 Sep 2021 10:15:11 GMT, Peter Levart wrote: > This patch improves reflective access speed as shown by the included > benchmarks: > > https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a > > ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection > with Method Handle) perform better in some circumstances. Looks good to me. Very nice performance improvement. One minor comment: I think the change in `UnsafeFieldAccessorImpl.java` and `UnsafeStaticFieldAccessorImpl.java` isn't necessary since they're final fields. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Mon, 27 Sep 2021 11:23:43 GMT, Claes Redestad wrote: > This looks good, assuming Mandy is OK with extracting and adding to the > microbenchmarks from JEP 416 I'm okay to include these microbenchmarks in this patch. I will merge the change with JEP 416. > A stray thought is why not most fields in `Field`/`Method`/`Constructor` are > `final`, as my IDE suggests. I can't find any hotspot code that injects > values to these fields. See `Reflection::new_method` and `Reflection::new_field`. > Experimentally changing most fields in `Field` to final seem to pass at least > all the java/lang/reflect tests. Since this is a trusted package `@Stable` > should then be pointless. This reminds to double check [1] and realized that `java.lang.reflect` is not in the list of trusted packages for `@Stable`. I'll add that in JEP 416. [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/share/ci/ciField.cpp#L219 - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Sat, 25 Sep 2021 10:15:11 GMT, Peter Levart wrote: > This patch improves reflective access speed as shown by the included > benchmarks: > > https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a > > ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection > with Method Handle) perform better in some circumstances. Adding to Peter's description - separating this patch from JEP 416 will give some bake time on this change of the existing core reflection implementation. When JEP 416 is integrated, it will switch the new implementation and the changes in `*AccessorImpl` will not be exercised. In addition, this will make the backport easier if desirable. - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader [v2]
On Fri, 24 Sep 2021 11:50:21 GMT, Сергей Цыпанов
wrote:
>> Currently on each invocation of `URLClassPath.FileLoader.getResource()`
>> `normalizedBase` URL is recalculated using same final `baseURL` from parent
>> class. This can be avoided giving significant improvement in memory
>> consumption. Consider the benchmark:
>>
>> @State(Scope.Benchmark)
>> @BenchmarkMode(Mode.AverageTime)
>> @OutputTimeUnit(TimeUnit.NANOSECONDS)
>> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
>> public class URLClassPathBenchmark {
>> private final ClassLoader classLoader = getClass().getClassLoader();
>>
>> @Benchmark
>> public URL getResource() {
>> return
>> classLoader.getResource("file:./config/application.properties");
>> }
>> }
>>
>> it demonstrates improvement brought in by this patch:
>>
>> before
>> Benchmark Mode
>> Cnt Score Error Units
>> URLClassPathBenchmark.getResource avgt
>> 50 2840,746 ± 22,206 ns/op
>> URLClassPathBenchmark.getResource:·gc.alloc.rateavgt
>> 50 645,313 ± 8,072 MB/sec
>> URLClassPathBenchmark.getResource:·gc.alloc.rate.norm avgt
>> 50 2403,258 ± 17,639B/op
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Eden_Space avgt
>> 50 656,624 ± 116,090 MB/sec
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Eden_Space.norm avgt
>> 50 2450,175 ± 440,011B/op
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Survivor_Space avgt
>> 50 0,123 ± 0,149 MB/sec
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Survivor_Space.norm avgt
>> 50 0,459 ± 0,556B/op
>> URLClassPathBenchmark.getResource:·gc.count avgt
>> 5067,000counts
>> URLClassPathBenchmark.getResource:·gc.time avgt
>> 50 117,000ms
>>
>> after
>> Benchmark Mode
>> Cnt ScoreError Units
>> URLClassPathBenchmark.getResource avgt
>> 100 2596,719 ± 9,786 ns/op
>> URLClassPathBenchmark.getResource:·gc.alloc.rateavgt
>> 100 448,780 ± 1,684 MB/sec
>> URLClassPathBenchmark.getResource:·gc.alloc.rate.norm avgt
>> 100 1528,040 ± 0,005B/op
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Eden_Space avgt
>> 100 479,905 ± 23,369 MB/sec
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Eden_Space.norm avgt
>> 100 1634,314 ± 79,821B/op
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Survivor_Space avgt
>> 100 0,101 ± 0,097 MB/sec
>> URLClassPathBenchmark.getResource:·gc.churn.G1_Survivor_Space.norm avgt
>> 100 0,345 ± 0,329B/op
>> URLClassPathBenchmark.getResource:·gc.count avgt
>> 10098,000 counts
>> URLClassPathBenchmark.getResource:·gc.time avgt
>> 100 218,000 ms
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8274276: Make URLClassPath.Loader.getBaseURL() final
OK - LGTM. I have run tier1 and tier2 on your changes in our CI and nothing
unexpected came up.
-
Marked as reviewed by dfuchs (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5677
Integrated: JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable
On Fri, 24 Sep 2021 23:07:33 GMT, Mandy Chung wrote: > GenGraphs tool generates the module graph. It currently supports the > configuration via javadoc-graphs.properties. However, > `make/jdk/src/classes/build/tools/jigsaw/javadoc-graphs.properties` only > documents two properties. It should be updated to cover all configurable > properties. > > There are a couple other properties not configurable such as nodesep and node > margin. This extends the configuration to allow to set additional properties. > > This also fixes `requiresMandatedColor` in javadoc-graphs.properties to light > gray to match the default configuration in the implementation, i.e. the color > of the edge to java.base. It seems a bug that was unnoticed until Alex and > Iris spotted it. This pull request has now been integrated. Changeset: daaa47e2 Author:Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/daaa47e2005cfa1d72f94a32e7756255f24c4d1f Stats: 167 lines in 3 files changed: 100 ins; 33 del; 34 mod 8274311: Make build.tools.jigsaw.GenGraphs more configurable Reviewed-by: alanb, iris - PR: https://git.openjdk.java.net/jdk/pull/5690
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Sat, 25 Sep 2021 03:38:24 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-8273790? >> >> As noted in that issue, trying to class load >> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` >> concurrently in separate threads can lead to a deadlock because of the >> cyclic nature of the code in their static initialization. More specifically, >> consider this case: >> >> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. >> - This gives T1 the implicit class init lock on `CalendarSystem`. >> - Consider thread T2 which at the same time initiates a classload on >> `sun.util.calendar.Gregorian` class. >> - This gives T2 a implicit class init lock on `Gregorian`. >> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` >> since it wants to create a (singleton) instance of `Gregorian` and assign it >> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class >> init lock on `Gregorian`, T1 ends up waiting >> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` >> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, >> T2 starts travelling up the class hierarchy and asks for a lock on >> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up >> waiting on T1 which is waiting on T2. That triggers this deadlock. >> >> The linked JBS issue has a thread dump which shows this in action. >> >> The commit here delays the instance creation of `Gregorian` by moving that >> instance creation logic from the static initialization of the >> `CalendarSystem` class, to the first call to >> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` >> from needing a lock on `Gregorian` during its static init (of course, unless >> some code in this static init flow calls >> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square >> one. I have verified, both manually and through the jtreg test, that the >> code in question doesn't have such calls) >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. The test in addition to loading these 2 classes in question, also >> additionally loads a few other classes concurrently. These classes have >> specific static initialization which leads the calls to >> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. >> Including these classes in the tests ensures that this deadlock hasn't >> "moved" to a different location. I have run multiple runs (approximately 25) >> of this test with the fix and I haven't seen it deadlock anymore. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor test adjustments as suggested by Naoto > - use a holder instead of volatile, as suggested by Roger - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Sat, 25 Sep 2021 03:38:24 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-8273790? >> >> As noted in that issue, trying to class load >> `sun.util.calendar.CalendarSystem` and `sun.util.calendar.Gregorian` >> concurrently in separate threads can lead to a deadlock because of the >> cyclic nature of the code in their static initialization. More specifically, >> consider this case: >> >> - Thread T1 initiates a classload on the `sun.util.calendar.CalendarSystem`. >> - This gives T1 the implicit class init lock on `CalendarSystem`. >> - Consider thread T2 which at the same time initiates a classload on >> `sun.util.calendar.Gregorian` class. >> - This gives T2 a implicit class init lock on `Gregorian`. >> - T1, still holding a lock on `CalendarSystem` attempts to load `Gregorian` >> since it wants to create a (singleton) instance of `Gregorian` and assign it >> to the `static final GREGORIAN_INSTANCE` member. Since T2 is holding a class >> init lock on `Gregorian`, T1 ends up waiting >> - T2 on the other hand is still loading the `Gregorian` class. `Gregorian` >> itself "is a" `CalendarSystem`, so during this loading of `Gregorian` class, >> T2 starts travelling up the class hierarchy and asks for a lock on >> `CalendarSystem`. However T1 is holding this lock and as a result T2 ends up >> waiting on T1 which is waiting on T2. That triggers this deadlock. >> >> The linked JBS issue has a thread dump which shows this in action. >> >> The commit here delays the instance creation of `Gregorian` by moving that >> instance creation logic from the static initialization of the >> `CalendarSystem` class, to the first call to >> `CalendarSystem#getGregorianCalendar()`. This prevents the `CalendarSystem` >> from needing a lock on `Gregorian` during its static init (of course, unless >> some code in this static init flow calls >> `CalendarSystem#getGregorianCalendar()`, in which case it is back to square >> one. I have verified, both manually and through the jtreg test, that the >> code in question doesn't have such calls) >> >> A new jtreg test has been introduced to reproduce the issue and verify the >> fix. The test in addition to loading these 2 classes in question, also >> additionally loads a few other classes concurrently. These classes have >> specific static initialization which leads the calls to >> `CalendarSystem#getGregorianCalendar()` or `CalendarSystem#forName()`. >> Including these classes in the tests ensures that this deadlock hasn't >> "moved" to a different location. I have run multiple runs (approximately 25) >> of this test with the fix and I haven't seen it deadlock anymore. > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Minor test adjustments as suggested by Naoto > - use a holder instead of volatile, as suggested by Roger Looks good. Thank you for the fix! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v4]
> Sorting: > > - adopt radix sort for sequential and parallel sorts on int/long/float/double > arrays (almost random and length > 6K) > - fix tryMergeRuns() to better handle case when the last run is a single > element > - minor javadoc and comment changes > > Testing: > - add new data inputs in tests for sorting > - add min/max/infinity values to float/double testing > - add tests for radix sort iaroslavski has updated the pull request incrementally with one additional commit since the last revision: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) Simplified mixed insertion sort - Changes: - all: https://git.openjdk.java.net/jdk/pull/3938/files - new: https://git.openjdk.java.net/jdk/pull/3938/files/6003fb69..adcc6942 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3938=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3938=02-03 Stats: 204 lines in 1 file changed: 52 ins; 96 del; 56 mod Patch: https://git.openjdk.java.net/jdk/pull/3938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3938/head:pull/3938 PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8231640: (prop) Canonical property storage [v22]
On Wed, 22 Sep 2021 10:27:45 GMT, Jaikiran Pai wrote: >> The commit in this PR implements the proposal for enhancement that was >> discussed in the core-libs-dev mailing list recently[1], for >> https://bugs.openjdk.java.net/browse/JDK-8231640 >> >> At a high level - the `store()` APIs in `Properties` have been modified to >> now look for the `SOURCE_DATE_EPOCH` environment variable[2]. If that env >> variable is set, then instead of writing out the current date time as a date >> comment, the `store()` APIs instead will use the value set for this env >> variable to parse it to a `Date` and write out the string form of such a >> date. The implementation here uses the `d MMM HH:mm:ss 'GMT'` date >> format and `Locale.ROOT` to format and write out such a date. This should >> provide reproducibility whenever the `SOURCE_DATE_EPOCH` is set. >> Furthermore, intentionally, no changes in the date format of the "current >> date" have been done. >> >> These modified `store()` APIs work in the presence of the `SecurityManager` >> too. The caller is expected to have a read permission on the >> `SOURCE_DATE_EPOCH` environment variable. If the caller doesn't have that >> permission, then the implementation of these `store()` APIs will write out >> the "current date" and will ignore any value that has been set for the >> `SOURCE_DATE_EPOCH` env variable. This should allow for backward >> compatibility of existing applications, where, when they run under a >> `SecurityManager` and perhaps with an existing restrictive policy file, the >> presence of `SOURCE_DATE_EPOCH` shouldn't impact their calls to the >> `store()` APIs. >> >> The modified `store()` APIs will also ignore any value for >> `SOURCE_DATE_EPOCH` that cannot be parsed to an `long` value. In such >> cases, the `store()` APIs will write out the "current date" and ignore the >> value set for this environment variable. No exceptions will be thrown for >> such invalid values. This is an additional backward compatibility precaution >> to prevent any rogue value for `SOURCE_DATE_EPOCH` from breaking >> applications. >> >> An additional change in the implementation of these `store()` APIs and >> unrelated to the date comment, is that these APIs will now write out the >> property keys in a deterministic order. The keys will be written out in the >> natural ordering as specified by `java.lang.String#compareTo()` API. >> >> The combination of the ordering of the property keys when written out and >> the usage of `SOURCE_DATE_EPOCH` environment value to determine the date >> comment should together allow for reproducibility of the output generated by >> these `store()` APIs. >> >> New jtreg test classes have been introduced to verify these changes. The >> primary focus of `PropertiesStoreTest` is the ordering aspects of the >> property keys that are written out. On the other hand >> `StoreReproducibilityTest` focuses on the reproducibility of the output >> generated by these APIs. The `StoreReproducibilityTest` runs these tests >> both in the presence and absence of `SecurityManager`. Plus, in the presence >> of SecurityManager, it tests both the scenarios where the caller is granted >> the requisite permission and in other case not granted that permission. >> >> These new tests and existing tests under `test/jdk/java/util/Properties/` >> pass with these changes. >> >> [1] >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-August/080758.html >> [2] https://reproducible-builds.org/specs/source-date-epoch/ > > Jaikiran Pai has updated the pull request incrementally with two additional > commits since the last revision: > > - Revert "Implement Mark's suggestion in CSR to include the > java.properties.date in the list of system properties listed in > System::getProperties()" > >Additional inputs since this specific commit was introduced have leaned > towards not listing this property in System::getProperties() > >This reverts commit 458c1fd37cf1e8e8bd0a60a171d173ce5da12b7c. > - reduce the line length of code comment Looks good, thanks for the initiative, the followup on the comments, and finding the sweet spot in the goals, compatibility, and implementation constraints. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5372
Re: RFR: 8274333: Redundant null comparison after Pattern.split
On Sun, 26 Sep 2021 15:10:52 GMT, Andrey Turbanov wrote: > In couple of classes, result part of arrays of Pattern.split is compared with > `null`. Pattern.split (and hence String.split) never returns `null` in array > elements. Such comparisons are redundant. The AlgorithmDecomposer change looks fine to me. - Marked as reviewed by mullan (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5708
Re: RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
On Mon, 27 Sep 2021 01:00:18 GMT, Joe Darcy wrote:
> This is an initial PR for expanded lint warnings done under two bugs:
>
> 8202056: Expand serial warning to check for bad overloads of serial-related
> methods and ineffectual fields
> 8160675: Issue lint warning for non-serializable non-transient instance
> fields in serializable type
>
> to get feedback on the general approach and test strategy before further
> polishing the implementation.
>
> The implementation initially started as an annotation processor I wrote
> several years ago. The refined version being incorporated into Attr has been
> refactored, had its checks expanded, and been partially ported to idiomatic
> javac coding style rather than using the javax.lang.model API from annotation
> processing.
>
> Subsequent versions of this PR are expected to move the implementation closer
> to idiomatic javac, in particular to use javac flags rather than
> javax.lang.model.Modifier's. Additional resources keys will be defined for
> the serialization-related fields and methods not having the expected
> modifiers, types, etc. The resource keys for the existing checks related to
> serialVersionUID and reused.
>
> Please also review the corresponding CSRs:
>
> https://bugs.openjdk.java.net/browse/JDK-8274335
> https://bugs.openjdk.java.net/browse/JDK-8274336
>
> Informative serialization-related warning messages must take into account
> whether a class, interface, annotation, record, and enum is being analyzed.
> Enum classes and record classes have special handling in serialization. This
> implementation under review has been augmented with checks for interface
> types recommended by Chris Hegarty in an attachment on 8202056.
>
> The JDK build has the Xlint:serial check enabled. The build did not pass with
> the augmented checks. For most modules, this PR contains the library changes
> necessary for the build to pass. I will start separate PRs in those library
> areas to get the needed SuppressWarning("serial") or other changes in place.
> For one module, I temporarily disabled the Xlint:serial check.
>
> In terms of performance, I have not done benchmarks of the JDK build with and
> without these changes, but informally the build seems to take about as long
> as before.
Build change looks ok.
-
Marked as reviewed by erikj (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5709
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Add Matcher predicate to avoid changing shared code as non-x86 platforms implements support for the _encodeAsciiArray intrinsic - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/12ab6ff5..9800a99a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=01-02 Stats: 17 lines in 6 files changed: 14 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
On Mon, 27 Sep 2021 11:59:54 GMT, Claes Redestad wrote: > > Should we remove the "iso" part from the method/class names? > > I'm open to suggestions, but I've not been able to think of anything better. > `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the > ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only > variant is technically encoding chars to valid ISO-8859-1. Okay, that's fine with me. - PR: https://git.openjdk.java.net/jdk/pull/5621
RFR: 8274367: Re-indent stack-trace examples for Throwable.printStackTrace
This PR fixes indentation in the examples in the doc comment for java.lang.Throwable#printStackTrace(). 1. Indentation in stack-trace output is significant. The cause of an exception is output on the same level of indentation as that of the exception. A suppressed exception is output at a deeper level of indentation than that of the containing exception. The last example for Throwable.printStackTrace violates this. 2. Indentation in examples for Throwable.printStackTrace that relate to suppressed exceptions is inconsistent with that of cause exceptions. - Commit messages: - Initial commit Changes: https://git.openjdk.java.net/jdk/pull/5715/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5715=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274367 Stats: 21 lines in 1 file changed: 0 ins; 0 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/5715.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5715/head:pull/5715 PR: https://git.openjdk.java.net/jdk/pull/5715
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v3]
On Mon, 27 Sep 2021 09:34:21 GMT, Volker Simonis wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add Matcher predicate to avoid changing shared code as non-x86 platforms >> implements support for the _encodeAsciiArray intrinsic > > src/hotspot/share/opto/c2compiler.cpp line 222: > >> 220: #if !defined(X86) >> 221: return false; // not yet implemented >> 222: #endif > > It might be a little more work, but I think it's cleaner to move the decision > whether the intrinisc is supported into the Matcher like for most other > intrinsics and keep this code here platform independent. Otherwise we will > get an increasing cascade of ifdefs as people start implementing this for > other platforms. Not too much work. I recently introduced platform-specific `matcher_*.hpp` files, so since then adding a boolean constant is easy (no need to muck with the .ad files). - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v2]
On Mon, 27 Sep 2021 06:36:50 GMT, Tobias Hartmann wrote: > Should we remove the "iso" part from the method/class names? I'm open to suggestions, but I've not been able to think of anything better. `encodeISOOrASCII` doesn't seem helpful and since ASCII is a subset of the ISO-8859-1 encoding referred to by the "iso" moniker then the ASCII-only variant is technically encoding chars to valid ISO-8859-1. - PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86 [v2]
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Tobias review - Changes: - all: https://git.openjdk.java.net/jdk/pull/5621/files - new: https://git.openjdk.java.net/jdk/pull/5621/files/8edc228f..12ab6ff5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5621=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5621=00-01 Stats: 10 lines in 3 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/5621.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5621/head:pull/5621 PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable
On Sat, 25 Sep 2021 10:15:11 GMT, Peter Levart wrote: > This patch improves reflective access speed as shown by the included > benchmarks: > > https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a > > ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection > with Method Handle) perform better in some circumstances. This looks good, assuming Mandy is OK with extracting and adding to the microbenchmarks from JEP 416 A stray thought is why not most fields in `Field`/`Method`/`Constructor` are `final`, as my IDE suggests. I can't find any hotspot code that injects values to these fields. Experimentally changing most fields in `Field` to final seem to pass at least all the java/lang/reflect tests. Since this is a trusted package `@Stable` should then be pointless. - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote: > This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to > work also for ASCII encoding, which makes for example the `UTF_8$Encoder` > perform on par with (or outperform) similarly getting charset encoded bytes > from a String. The former took a small performance hit in JDK 9, and the > latter improved greatly in the same release. > > Extending the `EncodeIsoArray` intrinsics on other platforms should be > possible, but I'm unfamiliar with the macro assembler in general and unlike > the x86 intrinsic they don't use a simple vectorized mask to implement the > latin-1 check. For example aarch64 seem to filter out the low bytes and then > check if there's any bits set in the high bytes. Clever, but very different > to the 0xFF80 2-byte mask that an ASCII test wants. src/hotspot/share/opto/c2compiler.cpp line 222: > 220: #if !defined(X86) > 221: return false; // not yet implemented > 222: #endif It might be a little more work, but I think it's cleaner to move the decision whether the intrinisc is supported into the Matcher like for most other intrinsics and keep this code here platform independent. Otherwise we will get an increasing cascade of ifdefs as people start implementing this for other platforms. - PR: https://git.openjdk.java.net/jdk/pull/5621
RFR: 8202056: Expand serial warning to check for bad overloads of serial-related methods and ineffectual fields
This is an initial PR for expanded lint warnings done under two bugs:
8202056: Expand serial warning to check for bad overloads of serial-related
methods and ineffectual fields
8160675: Issue lint warning for non-serializable non-transient instance fields
in serializable type
to get feedback on the general approach and test strategy before further
polishing the implementation.
The implementation initially started as an annotation processor I wrote several
years ago. The refined version being incorporated into Attr has been
refactored, had its checks expanded, and been partially ported to idiomatic
javac coding style rather than using the javax.lang.model API from annotation
processing.
Subsequent versions of this PR are expected to move the implementation closer
to idiomatic javac, in particular to use javac flags rather than
javax.lang.model.Modifier's. Additional resources keys will be defined for the
serialization-related fields and methods not having the expected modifiers,
types, etc. The resource keys for the existing checks related to
serialVersionUID and reused.
Please also review the corresponding CSRs:
https://bugs.openjdk.java.net/browse/JDK-8274335
https://bugs.openjdk.java.net/browse/JDK-8274336
Informative serialization-related warning messages must take into account
whether a class, interface, annotation, record, and enum is being analyzed.
Enum classes and record classes have special handling in serialization. This
implementation under review has been augmented with checks for interface types
recommended by Chris Hegarty in an attachment on 8202056.
The JDK build has the Xlint:serial check enabled. The build did not pass with
the augmented checks. For most modules, this PR contains the library changes
necessary for the build to pass. I will start separate PRs in those library
areas to get the needed SuppressWarning("serial") or other changes in place.
For one module, I temporarily disabled the Xlint:serial check.
In terms of performance, I have not done benchmarks of the JDK build with and
without these changes, but informally the build seems to take about as long as
before.
-
Commit messages:
- Appease jcheck
- Implement checks chegar recommended for interfaces.
- Update comment.
- Add tests for instance field checks.
- Clean build with instance field checks in place.
- Merge branch 'master' into JDK-8202056
- Put Externalizable checks last.
- Add checks for constructor access in Serializable classes.
- Add no-arg ctor check for Externalizable classes.
- Improve Externalization warnings.
- ... and 19 more: https://git.openjdk.java.net/jdk/compare/5756385c...d498ff5f
Changes: https://git.openjdk.java.net/jdk/pull/5709/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5709=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8202056
Stats: 1519 lines in 79 files changed: 1511 ins; 1 del; 7 mod
Patch: https://git.openjdk.java.net/jdk/pull/5709.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5709/head:pull/5709
PR: https://git.openjdk.java.net/jdk/pull/5709
RFR: 8274333: Redundant null comparison after Pattern.split
In couple of classes, result part of arrays of Pattern.split is compared with `null`. Pattern.split (and hence String.split) never returns `null` in array elements. Such comparisons are redundant. - Commit messages: - [PATCH] Remove redundant 'null' comparison after Pattern.split Changes: https://git.openjdk.java.net/jdk/pull/5708/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5708=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274333 Stats: 15 lines in 2 files changed: 4 ins; 5 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5708.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5708/head:pull/5708 PR: https://git.openjdk.java.net/jdk/pull/5708
Re: RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
On Fri, 24 Sep 2021 11:28:09 GMT, Masanori Yano wrote: > Could you please review the 8250678 bug fixes? > > The `parse` method of ModuleDescriptor.Version class behaves incorrectly when > the input string contains consecutive delimiters. > > The `parse` method treats strings as three sections, but the parsing method > differs between the method for the version sections and the ones for others. > In version sections, the `parse` method takes a single character in a loop > and determines whether it is a delimiter. In pre and build sections, the > parse method takes two characters in a loop and determines whether the second > character is the delimiter. Therefore, if the string contains a sequence of > delimiters in pre or build section, the `parse` method treats character at > the odd-numbered position as a token and the one at even-numbered as a > delimiter. > > A string containing consecutive delimiters is an incorrect version string, > but this behavior does not follow the API specification. > https://download.java.net/java/early_access/jdk18/docs/api/java.base/java/lang/module/ModuleDescriptor.Version.html > > Therefore, I propose to fix the parsing method of pre and build section in > the same way as the version. I thin this is okay, just look unusual to have repeated punctuation creating the case where a component is empty. @mbreinhold may wish to comment on this PR. - PR: https://git.openjdk.java.net/jdk/pull/5679
Re: RFR: 8233674: JarURLConnection.getJarFile throws Exception if some process is holding the file
On Mon, 27 Sep 2021 07:40:26 GMT, Masanori Yano wrote: > It’s a good idea to ask the Microsoft folks about that, but I don't know the > way to ask. Could you tell me how to do it? > > As you say, CreateFile function is used in other parts of the JDK, but it > have a significant impact to fix all of that. Therefore, I fixed the problem > about opening zip files and jar files which is reported in the JBS. I think we need to find out if the issue you are working around is a bug/issue with the virus checker or that Microsoft recommends that all applications should require to work these issues. As regards the patch then it is incomplete. If we are forced to put a workaround into the JDK code then I think it will have to do everywhere, not just for zip/JAR files. - PR: https://git.openjdk.java.net/jdk/pull/5460
Re: RFR: 8233674: JarURLConnection.getJarFile throws Exception if some process is holding the file
On Fri, 10 Sep 2021 09:23:52 GMT, Masanori Yano wrote: > Could you please review the 8233674 bug fixes? > This problem is caused by the antivirus software opening the file for a short > time, so CreateFile() should be retried. It’s a good idea to ask the Microsoft folks about that, but I don't know the way to ask. Could you tell me how to do it? As you say, CreateFile function is used in other parts of the JDK, but it have a significant impact to fix all of that. Therefore, I fixed the problem about opening zip files and jar files which is reported in the JBS. - PR: https://git.openjdk.java.net/jdk/pull/5460
Re: RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
On Tue, 21 Sep 2021 21:58:48 GMT, Claes Redestad wrote:
> This patch extends the `ISO_8859_1.implEncodeISOArray` intrinsic on x86 to
> work also for ASCII encoding, which makes for example the `UTF_8$Encoder`
> perform on par with (or outperform) similarly getting charset encoded bytes
> from a String. The former took a small performance hit in JDK 9, and the
> latter improved greatly in the same release.
>
> Extending the `EncodeIsoArray` intrinsics on other platforms should be
> possible, but I'm unfamiliar with the macro assembler in general and unlike
> the x86 intrinsic they don't use a simple vectorized mask to implement the
> latin-1 check. For example aarch64 seem to filter out the low bytes and then
> check if there's any bits set in the high bytes. Clever, but very different
> to the 0xFF80 2-byte mask that an ASCII test wants.
Very nice. The changes look good to me, just added some minor comments.
Should we remove the "iso" part from the method/class names?
src/hotspot/cpu/x86/x86_32.ad line 12218:
> 12216: instruct encode_ascii_array(eSIRegP src, eDIRegP dst, eDXRegI len,
> 12217: regD tmp1, regD tmp2, regD tmp3, regD tmp4,
> 12218: eCXRegI tmp5, eAXRegI result, eFlagsReg cr)
> %{
Indentation is wrong.
src/hotspot/cpu/x86/x86_32.ad line 12223:
> 12221: effect(TEMP tmp1, TEMP tmp2, TEMP tmp3, TEMP tmp4, USE_KILL src,
> USE_KILL dst, USE_KILL len, KILL tmp5, KILL cr);
> 1:
> 12223: format %{ "Encode array $src,$dst,$len -> $result// KILL ECX,
> EDX, $tmp1, $tmp2, $tmp3, $tmp4, ESI, EDI " %}
You might want to change the opto assembly comment to "Encode ascii array" (and
to "Encode iso array" above). Same on 64-bit.
src/hotspot/share/opto/intrinsicnode.hpp line 171:
> 169:
> 170:
> //--EncodeISOArray
> 171: // encode char[] to byte[] in ISO_8859_1
Comment should be adjusted to `... in ISO_8859_1 or ASCII`.
-
Marked as reviewed by thartmann (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5621
