Re: RFR: 8266936: Add a finalization JFR event [v11]
On Thu, 23 Sep 2021 22:35:26 GMT, Coleen Phillimore wrote: >> Markus Grönlund has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - remove rehashing and rely on default grow_hint for table resize >> - mtStatistics > > src/hotspot/share/jfr/periodic/jfrFinalizerStatisticsEvent.cpp line 26: > >> 24: >> 25: #include "precompiled.hpp" >> 26: #if INCLUDE_MANAGEMENT > > With precompiled headers turned off, this gets a compilation error: > error: "INCLUDE_MANAGEMENT" is not > defined, evaluates to 0 [-Werror=undef] Thanks, I missed submitting a few no-precompiled header builds. Fixed now. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v12]
> 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: no precompiled headers and mtServiceability nmt classification - Changes: - all: https://git.openjdk.java.net/jdk/pull/4731/files - new: https://git.openjdk.java.net/jdk/pull/4731/files/62592daf..5759c605 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=10-11 Stats: 9 lines in 4 files changed: 4 ins; 2 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
RFR: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader
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 50
67,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 100
98,000 counts
URLClassPathBenchmark.getResource:·gc.time avgt 100
218,000 ms
-
Commit messages:
- 8274276: Cache normalizedBase URL in URLClassPath.FileLoader
Changes: https://git.openjdk.java.net/jdk/pull/5677/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5677&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8274276
Stats: 4 lines in 1 file changed: 2 ins; 1 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5677.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5677/head:pull/5677
PR: https://git.openjdk.java.net/jdk/pull/5677
Re: RFR: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader
On Fri, 24 Sep 2021 10:35:54 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 50
>67,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 100
>98,000 counts
> URLClassPathBenchmark.getResource:·gc.time avgt 100
> 218,000 ms
This is calling an overidable method within a constructor, which might have
unforeseen side effects. Can you make `Loader::getBaseURL` final? That would
remove the concern.
-
PR: https://git.openjdk.java.net/jdk/pull/5677
RFR: 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently
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. - Commit messages: - 8250678: ModuleDescriptor.Version parsing treats empty segments inconsistently Changes: https://git.openjdk.java.net/jdk/pull/5679/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5679&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8250678 Stats: 26 lines in 2 files changed: 11 ins; 14 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5679.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5679/head:pull/5679 PR: https://git.openjdk.java.net/jdk/pull/5679
Re: RFR: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader [v2]
> 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 50
>67,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 100
>98,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
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/5677/files
- new: https://git.openjdk.java.net/jdk/pull/5677/files/dffc3301..937e6aee
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5677&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5677&range=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/5677.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/5677/head:pull/5677
PR: https://git.openjdk.java.net/jdk/pull/5677
Re: RFR: 8274276: Cache normalizedBase URL in URLClassPath.FileLoader [v2]
On Fri, 24 Sep 2021 11:14:00 GMT, Daniel Fuchs wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8274276: Make URLClassPath.Loader.getBaseURL() final > > This is calling an overidable method within a constructor, which might have > unforeseen side effects. Can you make `Loader::getBaseURL` final? That would > remove the concern. @dfuch Sure, done! - PR: https://git.openjdk.java.net/jdk/pull/5677
Integrated: JDK-8274087: Windows DLL path not set correctly.
On Thu, 23 Sep 2021 20:03:29 GMT, Andy Herrick wrote: > JDK-8274087: Windows DLL path not set correctly. This pull request has now been integrated. Changeset: f36a2bbd Author:Andy Herrick URL: https://git.openjdk.java.net/jdk/commit/f36a2bbd15d94d4371c2117ce08b7f04a0d59da4 Stats: 5 lines in 1 file changed: 2 ins; 2 del; 1 mod 8274087: Windows DLL path not set correctly. Reviewed-by: almatvee, asemenyuk - PR: https://git.openjdk.java.net/jdk/pull/5663
Integrated: 8274234: Remove unnecessary boxing via primitive wrapper valueOf(String) methods in java.sql.rowset
On Thu, 23 Sep 2021 19:47:51 GMT, Andrey Turbanov wrote: > Usages of methods Integer.valueOf, Byte.valueOf, Short.valueOf, > Float.valueOf, Double.valueOf, Long.valueOf often can be simplified by using > their pair methods parseInt/parseLong/parseShort/parseByte/parseFloat. This pull request has now been integrated. Changeset: f214d6e8 Author:Andrey Turbanov Committer: Lance Andersen URL: https://git.openjdk.java.net/jdk/commit/f214d6e8736a620c8e1b87c30587aa09774c Stats: 7 lines in 2 files changed: 0 ins; 1 del; 6 mod 8274234: Remove unnecessary boxing via primitive wrapper valueOf(String) methods in java.sql.rowset Reviewed-by: lancea, bpb - PR: https://git.openjdk.java.net/jdk/pull/5662
Re: RFR: 8253702: BigSur version number reported as 10.16, should be 11.nn [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set Hi Marc, cc @sormuras, not necessary, it was already reported & fixed: https://bugs.openjdk.java.net/browse/JDK-8269850 😄 Cheers Christoph - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur version number reported as 10.16, should be 11.nn [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set With macOS 11.6 this fix no longer works. @sormuras will create a proper bug report. 😉 - PR: https://git.openjdk.java.net/jdk/pull/2530
Re: RFR: 8253702: BigSur version number reported as 10.16, should be 11.nn [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set Thanks! I searched the bug tracker but couldn't find it. Will that be backported to JDK 11 and/or 17? - PR: https://git.openjdk.java.net/jdk/pull/2530
RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem
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. - Commit messages: - 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem Changes: https://git.openjdk.java.net/jdk/pull/5683/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5683&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273790 Stats: 185 lines in 2 files changed: 181 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5683/head:pull/5683 PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 8253702: BigSur version number reported as 10.16, should be 11.nn [v3]
On Thu, 11 Feb 2021 19:23:55 GMT, Roger Riggs wrote: >> On Mac Os X, the OSVersionTest detected a difference in the version number >> reported in the os.version property >> and the version number provided by `sw_vers -productVersion`. >> >> When the java runtime is built with XCode 11.3, the os.version is reported >> as 10.16 >> though the current version numbering is 11.nnn. >> >> The workaround is to derive the os.version number from the >> ProductBuildVersion. >> When the toolchain is updated to XCode 12.nnn it can be removed. >> The workaround is enabled only when the environment variable >> SYSTEM_VERSION_COMPAT is unset. >> When the SYSTEM_VERSION_COMPAT is set in the environment the version number >> is reported as reported by Mac OS X. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > optimize case of 10.16 and SYSTEM_VERSION_COMPAT is set Yup: https://github.com/openjdk/jdk17u/pull/85 https://github.com/openjdk/jdk11u/pull/12 For OpenJDK 11u I hope to get it into the October Update. Can't speak for 17u as this is maintained by Oracle. Maybe @sormuras can help here :) - PR: https://git.openjdk.java.net/jdk/pull/2530
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. The current version (cef05f4) copies the ISO_8859_1.implEncodeISOArray intrinsic and adapts it to work on ASCII encoding, which makes the UTF_8$Encoder perform on par with (or outperform) encoding from a String. Using microbenchmarks provided by @carterkozak here: https://github.com/carterkozak/stringbuilder-encoding-performance Baseline: Benchmark (charsetName) (message) (timesToAppend) Mode Cnt ScoreError Units EncoderBenchmarks.charsetEncoder UTF-8 This is a simple ASCII message3 avgt8 270.237 ± 10.504 ns/op EncoderBenchmarks.charsetEncoder UTF-8 This is a message with unicode 😊3 avgt8 568.353 ± 2.331 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a simple ASCII message3 avgt8 324.889 ± 17.466 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a message with unicode 😊3 avgt8 633.720 ± 22.703 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a simple ASCII message3 avgt8 1132.436 ± 30.661 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a message with unicode 😊3 avgt8 1379.207 ± 66.982 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a simple ASCII message3 avgt891.253 ± 3.848 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a message with unicode 😊3 avgt8 519.489 ± 12.516 ns/op Patch: Benchmark (charsetName) (message) (timesToAppend) Mode Cnt Score Error Units EncoderBenchmarks.charsetEncoder UTF-8 This is a simple ASCII message3 avgt482.535 ± 20.310 ns/op EncoderBenchmarks.charsetEncoder UTF-8 This is a message with unicode 😊3 avgt4 522.679 ± 13.456 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a simple ASCII message3 avgt4 127.831 ± 32.612 ns/op EncoderBenchmarks.charsetEncoderWithAllocation UTF-8 This is a message with unicode 😊3 avgt4 549.343 ± 59.899 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a simple ASCII message3 avgt4 1182.835 ± 153.735 ns/op EncoderBenchmarks.charsetEncoderWithAllocationWrappingBuilder UTF-8 This is a message with unicode 😊3 avgt4 1416.407 ± 130.551 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a simple ASCII message3 avgt497.770 ± 15.742 ns/op EncoderBenchmarks.toStringGetBytes UTF-8 This is a message with unicode 😊3 avgt4 516.351 ± 58.580 ns/op This can probably be simplified further, say by adding a flag to the intrinsic of whether we're encoding ASCII only or ISO-8859-1. It also needs to be implemented and tested on all architectures. (edit: accidentally edit rather than quote-reply, restored original comment) On the JDK-included `CharsetEncodeDecode.encode` microbenchmark, I get these numbers in the baseline (18-b09): Benchmark (size) (type) Mode CntScore Error Units CharsetEncodeDecode.encode 16384UTF-8 avgt 30 39.962 ± 1.703 us/op CharsetEncodeDecode.encode 16384 BIG5 avgt 30 153.282 ± 4.521 us/op CharsetEncodeDecode.encode 16384 ISO-8859-15 avgt 30 192.040 ± 4.543 us/op CharsetEncodeDecode.enco
RFR: 8274242: Implement fast-path for ASCII-compatible CharsetEncoders on x86
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. - Commit messages: - Freshen up CharsetEncodeDecode micro (still only tests ASCII), optimize ASCII-compatible SingleByte (e.g. ISO-8859-15) - Add @bug id to test - Move and adapt defunct Test6896617 test to test more Charsets without using internal APIs; remove adhoc performance tests - Exclude encodeAsciiArray intrinsic on all non-X86 platforms - Fix indentation - Remove EncodeAsciiArray node and instead extend EncodeISOArray with an is_ascii predicate - Merge MacroAssembler methods - Implement intrinsified ASCII fast-path by copying and adapting encodeISOArray intrinsic (currently x86 only) - Enhance UTF_8.Encoder by using StringUTF16.compress for ASCII Changes: https://git.openjdk.java.net/jdk/pull/5621/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5621&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274242 Stats: 763 lines in 20 files changed: 375 ins; 355 del; 33 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
On Thu, 23 Sep 2021 00:03:55 GMT, Claes Redestad wrote: > This can probably be simplified further, say by adding a flag to the > intrinsic of whether we're encoding ASCII only or ISO-8859-1. Done: Removed the addition of a new C2 Node, merged the macro assembler encode_iso_array and encode_ascii_array and added a predicate to select the behavior. > It also needs to be implemented and tested on all architectures. Implementing this on other hardware is Future Work. The non-x86 intrinsics for implEncodeISOArray all seem to use clever tricks rather than a simple mask that can be easily switched from detecting non-latin-1(0xFF00) to detecting ASCII (0xFF80). Clever tricks make it rather challenging to extend this like I could easily do in the x86 code (most all assembler is foreign to me) - PR: https://git.openjdk.java.net/jdk/pull/5621
RFR: 8274293: Build failure with Xcode 13.0 due to vfork is deprecated
remove vfork() on Darwin - Commit messages: - 8274293: Build failure with Xcode 13.0 due to vfork is deprecated Changes: https://git.openjdk.java.net/jdk/pull/5686/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5686&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274293 Stats: 10 lines in 2 files changed: 8 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5686.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5686/head:pull/5686 PR: https://git.openjdk.java.net/jdk/pull/5686
Re: RFR: 8274293: Build failure with Xcode 13.0 due to vfork is deprecated
On Fri, 24 Sep 2021 16:18:57 GMT, xpbob wrote: > remove vfork() on Darwin src/java.base/unix/native/libjava/ProcessImpl_md.c line 447: > 445: #endif > 446: > 447: /* vfork(2) is deprecated on Solaris and Darwin */ Drive by comment is that we can probably drop "Solaris" from the comment. - PR: https://git.openjdk.java.net/jdk/pull/5686
Re: RFR: 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.
Thanks, Jaikiran. Looks good. Some minor comments.
src/java.base/share/classes/sun/util/calendar/CalendarSystem.java line 123:
> 121: */
> 122: public static Gregorian getGregorianCalendar() {
> 123: var gCal = GREGORIAN_INSTANCE;
Do we need the local variable `gCal`?
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 43:
> 41: * @run main/othervm CalendarSystemDeadLockTest
> 42: * @run main/othervm CalendarSystemDeadLockTest
> 43: * @run main/othervm CalendarSystemDeadLockTest
Just curious, before the fix, how many test instances caused the deadlock? I'd
think these 5 runs are arbitrary numbers, Just wanted to have those 5 runs are
appropriate.
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 75:
> 73: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
> 74: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
> 75: final ExecutorService executor =
> Executors.newFixedThreadPool(tasks.size());
Asserting `tasks.size() == numTasks` may help here.
test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 171:
> 169: }
> 170: }
> 171: }
Need a new line at the EOF.
-
PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 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. As an alternative, can the Gregorian Instance be moved to a nested (static) class. That will delay initialization until it is needed. This "holder" pattern is used elsewhere to defer initialization and break cycles. - PR: https://git.openjdk.java.net/jdk/pull/5683
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. core library part of the changes looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5621
Re: RFR: 8266936: Add a finalization JFR event [v12]
On Fri, 24 Sep 2021 09:27:02 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: > > no precompiled headers and mtServiceability nmt classification Hi Markus, It's a little surprised to see Finalizer.c to depend JMM interface which is used by `java.management` and `jdk.management` modules only. It's more appropriate for it to be a JVM_* entry point for Finalizer to report completion of the finalization instead.I understand that you want to make FinalizerService to be a conditional feature which is a good idea. Such JVM entry can be made as a nop if not INCLUDE_SERVICES. - PR: https://git.openjdk.java.net/jdk/pull/4731
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. x86 part of changes look good. - PR: https://git.openjdk.java.net/jdk/pull/5621
RFR: JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable
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. - Commit messages: - JDK-8274311: Make build.tools.jigsaw.GenGraphs more configurable Changes: https://git.openjdk.java.net/jdk/pull/5690/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5690&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274311 Stats: 167 lines in 3 files changed: 100 ins; 33 del; 34 mod Patch: https://git.openjdk.java.net/jdk/pull/5690.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5690/head:pull/5690 PR: https://git.openjdk.java.net/jdk/pull/5690
Re: RFR: 8274293: Build failure with Xcode 13.0 due to vfork is deprecated [v2]
> remove vfork() on Darwin xpbob has updated the pull request incrementally with one additional commit since the last revision: Drop drop "Solaris" from the comment - Changes: - all: https://git.openjdk.java.net/jdk/pull/5686/files - new: https://git.openjdk.java.net/jdk/pull/5686/files/89bb9c31..27d19543 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5686&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5686&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5686.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5686/head:pull/5686 PR: https://git.openjdk.java.net/jdk/pull/5686
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/5683/files - new: https://git.openjdk.java.net/jdk/pull/5683/files/8855f910..8b276d4d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5683&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5683&range=00-01 Stats: 22 lines in 2 files changed: 8 ins; 10 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5683/head:pull/5683 PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 8273790: Potential cyclic dependencies between Gregorian and CalendarSystem [v2]
On Fri, 24 Sep 2021 17:53:03 GMT, Naoto Sato wrote:
>> 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
>
> src/java.base/share/classes/sun/util/calendar/CalendarSystem.java line 123:
>
>> 121: */
>> 122: public static Gregorian getGregorianCalendar() {
>> 123: var gCal = GREGORIAN_INSTANCE;
>
> Do we need the local variable `gCal`?
This was there to avoid additional volatile reads in that method. A performance
optimization. However, with the change Roger suggested, this is no longer
relevant.
> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 43:
>
>> 41: * @run main/othervm CalendarSystemDeadLockTest
>> 42: * @run main/othervm CalendarSystemDeadLockTest
>> 43: * @run main/othervm CalendarSystemDeadLockTest
>
> Just curious, before the fix, how many test instances caused the deadlock?
> I'd think these 5 runs are arbitrary numbers, Just wanted to have those 5
> runs are appropriate.
Hello @naotoj,
On my setup, without the fix, the test deadlocks almost always right on the
first run. There have been cases where it did pass the first time, but running
it a second time has always reproduced the failure. The 5 runs that I have in
this test is indeed an arbitrary number. Given how quickly this test completes,
I decided to use a slightly higher number of 5 instead of maybe 2 or 3. Do you
think, we should change the run count to something else?
> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 75:
>
>> 73: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
>> 74: tasks.add(new GetGregorianCalTask(taskTriggerLatch));
>> 75: final ExecutorService executor =
>> Executors.newFixedThreadPool(tasks.size());
>
> Asserting `tasks.size() == numTasks` may help here.
Yes, that makes sense. I've updated the test to add this check.
> test/jdk/sun/util/calendar/CalendarSystemDeadLockTest.java line 171:
>
>> 169: }
>> 170: }
>> 171: }
>
> Need a new line at the EOF.
Done. I've updated this in the latest version of the PR.
-
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 Hello Roger, > As an alternative, can the Gregorian Instance be moved to a nested (static) > class. That will delay initialization until it is needed. This "holder" pattern is used elsewhere to defer initialization and break cycles. I did indeed have that in mind when I started work on this. That was something Chris Hegarty had suggested and we have used in a different (but similar) issue a while back[1]. I was however unsure if that's a common enough technique, so had started off with the volatile approach. I've now updated the PR to use the holder technique instead. [1] https://github.com/openjdk/jdk/pull/2893#issuecomment-797539503 - 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 @jaikiran Thanks for fixing this. Delaying instance creation via a static holder class seems reasonable to me. - Marked as reviewed by yyang (Committer). PR: https://git.openjdk.java.net/jdk/pull/5683
Re: RFR: 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. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5690
