Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 07:04:56 GMT, David Holmes wrote: > I have reservations about contorting things this way just to get > "deterministic output". > > The VM needs to fully initialize and then become quiescent before the dump > occurs, and as I say below if you don't start other threads then you > potentially remove part of the archive because classes won't be loaded by > those threads. > > I think if you care about the order of dumping classes then you should > control that order, you don't try to force the order of loading. Can't you > sort things before dumping? ie rehash/rebuild the hashtables etc so it has a > canonical ordering? I see this was mentioned in the bug report and is > considered a largish/complex fix, but it would be the proper fix IMO. > > Thanks, David I tried the "proper" approach but it's very complicated. I already have an implementation that sorts all the metadata. However, the CDS archive also contains a heap dump, which includes Java HashMaps. If I allow those 3 Java threads to start, some HashMaps in the module graph will have unstable ordering. I think the reason is concurrent thread execution causes unstable assignment of the identity_hash for objects in the heap dump. I cannot think of a clean way to fix this. The alternative, disabling Java thread starts, is much simpler and much more appealing to me. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 06:49:02 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > src/hotspot/share/prims/jvm.cpp line 2873: > >> 2871: // execute in parallel, symbols and classes may be loaded in >> 2872: // random orders which will make the resulting CDS archive >> 2873: // non-deterministic. > > Yes but by not starting these threads you are potentially excluding a range > of classes from the shared archive! `java -Xshare:dump` loads all classes specified in a classlist, which is created without this thread-disabling hack. The number of classes in the CDS archive is the same before/after this PR. The size of the CDS archive is identical. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 05:10:44 GMT, Ioi Lam wrote: >> This patch makes the result of "java -Xshare:dump" deterministic: >> - Disabled new Java threads from launching. This is harmless. See comments >> in jvm.cpp >> - Fixed a problem in hashtable ordering in heapShared.cpp >> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random >> bits. Added code to zero it. >> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in >> make/scripts/compare.sh >> >> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. >> This will be fixed in >> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). >> >> Testing under way: >> - tier1~tier5 >> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, >> windows-x86-cmp-baseline, etc). > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > Fixed zero build I have reservations about contorting things this way just to get "deterministic output". The VM needs to fully initialize and then become quiescent before the dump occurs, and as I say below if you don't start other threads then you potentially remove part of the archive because classes won't be loaded by those threads. I think if you care about the order of dumping classes then you should control that order, you don't try to force the order of loading. Can't you sort things before dumping? ie rehash/rebuild the hashtables etc so it has a canonical ordering? I see this was mentioned in the bug report and is considered a largish/complex fix, but it would be the proper fix IMO. Thanks, David src/hotspot/share/prims/jvm.cpp line 2873: > 2871: // execute in parallel, symbols and classes may be loaded in > 2872: // random orders which will make the resulting CDS archive > 2873: // non-deterministic. Yes but by not starting these threads you are potentially excluding a range of classes from the shared archive! - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
> This patch makes the result of "java -Xshare:dump" deterministic: > - Disabled new Java threads from launching. This is harmless. See comments in > jvm.cpp > - Fixed a problem in hashtable ordering in heapShared.cpp > - BasicHashtableEntry has a gap on 64-bit platforms that may contain random > bits. Added code to zero it. > - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in > make/scripts/compare.sh > > Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. > This will be fixed in > [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). > > Testing under way: > - tier1~tier5 > - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, > windows-x86-cmp-baseline, etc). Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: Fixed zero build - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/1fb3f830..44db40f1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=00-01 Stats: 5 lines in 2 files changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7748.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7748/head:pull/7748 PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 14:02:35 GMT, Magnus Ihse Bursie wrote: >> I'm of the opinion that options which cannot have empty values, and will >> fall back to default ones if no explicit one is provided, would generate an >> error if --without-* is passed, while others that _can_ have empty values >> allow passing --without-* explicitly. From what I can gather so far, >> everything in branding.conf and a few other crucial options like >> --with-version-string or --with-version date are cases of the former, while >> --with-vendor-version-string would be an example of the latter. That said, >> it would be weird if we're folding no results into the yes branch, since >> passing --without-* would then return an error mentioning --with-* instead, >> which is in turn potentially confusing. Since we're on the topic, if this is >> the case, should -with-*= be considered as an error as well, for options >> that cannot have no value? (Currently it just assumes "Use the default >> value") > > This entire PR feels like a non-issue. > > I agree that the UX of the --with-* flags are not optimal, but I also doubt > it worth putting much time and effort into fixing. Going through each and > every --with-flag to determine the best way to handle the (almost always > incorrect) --without-* variant is not something I'd look forward to. > > If anything should be done at all, I sincerely believe that merging "yes" and > "no" will be the best way to handle. The fact that the error message mentions > "--with" instead of "--without" is of no significance, since that's the way > we treat the --with flags everywhere, just as the --enable/--disable > dichotomy. I disagree with the assessment that this isn't an issue, since as I mentioned above this can be fairly confusing, but I do concede that it would be tedious to check every flag like this, so it seems merging yes and no is the way to go. In that case though, I'm wondering if we should fold the x$with_foo = x (ie --with-foo= ) into the yes and no check as well, since right now if that's passed it assumes that the default option is requested. I could also change the error message to be a generic one that matches both conditions, but I'll leave that up for discussion for the time being As a side note, I'm also willing to be responsible for properly checking every --without variant of all the current flags and any future ones, if it ever comes to that - PR: https://git.openjdk.java.net/jdk/pull/7713
Integrated: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame
On Wed, 2 Mar 2022 18:56:40 GMT, Tim Prinzing wrote: > The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. This pull request has now been integrated. Changeset: 31ad80a2 Author:Tim Prinzing Committer: Mandy Chung URL: https://git.openjdk.java.net/jdk/commit/31ad80a229e3f67823ff8f1fc914c5503f184b57 Stats: 218 lines in 4 files changed: 207 ins; 3 del; 8 mod 8280902: ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame Reviewed-by: naoto, mchung, ihse - PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v5]
> The caller class returned by Reflection::getCallerClass was used to gain > access to it's module in most cases and class loader in one case. I added a > method to translate the caller class to caller module so that the decision of > what module represents the caller with no stack frame is made in a single > place. Calls made to caller.getModule() were replaced with > getCallerModule(caller) which returns the system class loader unnamed module > if the caller is null. > > The one place a class loader was produced from the caller in getBundleImpl it > was rewritten to route through the getCallerModule method: > > final ClassLoader loader = (caller != null) ? > caller.getClassLoader() : getLoader(getCallerModule(caller)); > > A JNI test was added which calls getBundle to fetch a test bundle from a > location added to the classpath, fetches a string out of the bundle and > verifies it, and calls clearCache. > > The javadoc was updated for the caller sensitive methods changed. Tim Prinzing has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains seven additional commits since the last revision: - Merge branch 'master' into JDK-8280902 - remove unnecessary variable - more suggested changes - suggested changes - Update src/java.base/share/classes/java/util/ResourceBundle.java Co-authored-by: Mandy Chung - Use the unnamed module defined to the system class loader instead of the boot class loader. - JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame - Changes: - all: https://git.openjdk.java.net/jdk/pull/7663/files - new: https://git.openjdk.java.net/jdk/pull/7663/files/45f9b37b..6600e1f5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7663=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7663=03-04 Stats: 17900 lines in 516 files changed: 12667 ins; 3251 del; 1982 mod Patch: https://git.openjdk.java.net/jdk/pull/7663.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7663/head:pull/7663 PR: https://git.openjdk.java.net/jdk/pull/7663
Re: RFR: 8253495: CDS generates non-deterministic output
On Tue, 8 Mar 2022 19:11:02 GMT, Ioi Lam wrote: > This patch makes the result of "java -Xshare:dump" deterministic: > - Disabled new Java threads from launching. This is harmless. See comments in > jvm.cpp > - Fixed a problem in hashtable ordering in heapShared.cpp > - BasicHashtableEntry has a gap on 64-bit platforms that may contain random > bits. Added code to zero it. > - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in > make/scripts/compare.sh > > Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. > This will be fixed in > [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). > > Testing under way: > - tier1~tier5 > - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, > windows-x86-cmp-baseline, etc). compare.sh looks good. Can't comment on the rest. Thank you for fixing this! - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7748
RFR: 8253495: CDS generates non-deterministic output
This patch makes the result of "java -Xshare:dump" deterministic: - Disabled new Java threads from launching. This is harmless. See comments in jvm.cpp - Fixed a problem in hashtable ordering in heapShared.cpp - BasicHashtableEntry has a gap on 64-bit platforms that may contain random bits. Added code to zero it. - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in make/scripts/compare.sh Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. This will be fixed in [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). Testing under way: - tier1~tier5 - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, windows-x86-cmp-baseline, etc). - Commit messages: - Merge branch 'master' into 8253495-cds-generateds-non-deterministic-output-2 - fixed test - more fixes - 8253495: CDS generates non-deterministic output Changes: https://git.openjdk.java.net/jdk/pull/7748/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7748=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8253495 Stats: 73 lines in 12 files changed: 49 ins; 7 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/7748.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7748/head:pull/7748 PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: JDK-8280902 ResourceBundle::getBundle may throw NPE when invoked by JNI code with no caller frame [v4]
On Thu, 3 Mar 2022 16:55:46 GMT, Tim Prinzing wrote: >> The caller class returned by Reflection::getCallerClass was used to gain >> access to it's module in most cases and class loader in one case. I added a >> method to translate the caller class to caller module so that the decision >> of what module represents the caller with no stack frame is made in a single >> place. Calls made to caller.getModule() were replaced with >> getCallerModule(caller) which returns the system class loader unnamed module >> if the caller is null. >> >> The one place a class loader was produced from the caller in getBundleImpl >> it was rewritten to route through the getCallerModule method: >> >> final ClassLoader loader = (caller != null) ? >> caller.getClassLoader() : getLoader(getCallerModule(caller)); >> >> A JNI test was added which calls getBundle to fetch a test bundle from a >> location added to the classpath, fetches a string out of the bundle and >> verifies it, and calls clearCache. >> >> The javadoc was updated for the caller sensitive methods changed. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > more suggested changes Build changes look good. Can't say anything about the rest. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7663
Integrated: 8282769: BSD date cannot handle all ISO 8601 formats
On Mon, 7 Mar 2022 22:47:13 GMT, Magnus Ihse Bursie wrote: > The BSD version of the date command that ships with macOS cannot handle all > ISO 8601 formats. More specifically, it cannot handle trailing milliseconds. > > This fix will allow it to be more tolerant. This pull request has now been integrated. Changeset: 0f88fc18 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/0f88fc180cd5abc60605a094efa3f3a54f67f7a0 Stats: 7 lines in 1 file changed: 0 ins; 2 del; 5 mod 8282769: BSD date cannot handle all ISO 8601 formats Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/7733
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 02:48:45 GMT, Julian Waters wrote: >> ... and this goes for all the changes in the PR. > > I'm of the opinion that options which cannot have empty values, and will fall > back to default ones if no explicit one is provided, would generate an error > if --without-* is passed, while others that _can_ have empty values allow > passing --without-* explicitly. From what I can gather so far, everything in > branding.conf and a few other crucial options like --with-version-string or > --with-version date are cases of the former, while > --with-vendor-version-string would be an example of the latter. That said, it > would be weird if we're folding no results into the yes branch, since passing > --without-* would then return an error mentioning --with-* instead, which is > in turn potentially confusing. Since we're on the topic, if this is the case, > should -with-*= be considered as an error as well, for options that cannot > have no value? (Currently it just assumes "Use the default value") This entire PR feels like a non-issue. I agree that the UX of the --with-* flags are not optimal, but I also doubt it worth putting much time and effort into fixing. Going through each and every --with-flag to determine the best way to handle the (almost always incorrect) --without-* variant is not something I'd look forward to. If anything should be done at all, I sincerely believe that merging "yes" and "no" will be the best way to handle. The fact that the error message mentions "--with" instead of "--without" is of no significance, since that's the way we treat the --with flags everywhere, just as the --enable/--disable dichotomy. - PR: https://git.openjdk.java.net/jdk/pull/7713
Integrated: 8282770: Set source date in jib profiles from buildId
On Mon, 7 Mar 2022 23:32:33 GMT, Magnus Ihse Bursie wrote: > For reproducability, we should set the source date on jib profiles from the > buildId. This pull request has now been integrated. Changeset: c6d743fb Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/c6d743fb920e740c7b0aac0c0ef5bdc3d60252ad Stats: 10 lines in 1 file changed: 8 ins; 1 del; 1 mod 8282770: Set source date in jib profiles from buildId Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/7735
Re: RFR: 8282770: Set source date in jib profiles from buildId
On Mon, 7 Mar 2022 23:32:33 GMT, Magnus Ihse Bursie wrote: > For reproducability, we should set the source date on jib profiles from the > buildId. Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7735
Re: RFR: JDK-8282700: Properly handle several --without options during configure [v2]
On Tue, 8 Mar 2022 04:15:51 GMT, David Holmes wrote: > Passing --without-x makes no sense at all IMHO. Even so, I'd argue that it should be handled properly and show what really went wrong, rather than throwing, for instance, "no is not a valid version string" (In the case of --without-version-string). Besides, to an outside observer some of the configure options that cannot have --without-* set would still seem to make sense when glanced over, it may not be immediately obvious that said option cannot have no value. Even without the errors though, many of the configure options also don't check if --without-* is passed, which would result in the silent build error of setting said option to a literal "no", that would appear to have worked correctly regardless (All the --with-vender-* options are examples of this) > If you pass "" as the value then you get what you deserve if the build fails. I've been wondering if I should fold that into the "yes" check for several of them, since many interpret --with-*= as "use the default value" - PR: https://git.openjdk.java.net/jdk/pull/7713
