Re: RFR: 8253495: CDS generates non-deterministic output [v8]
On Tue, 15 Mar 2022 17:08:27 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: > > Avoid memset twice in os::malloc(); added comments about > NMTPreInit::handle_malloc vs DumpSharedSpaces Marked as reviewed by ccheung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v8]
> 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: Avoid memset twice in os::malloc(); added comments about NMTPreInit::handle_malloc vs DumpSharedSpaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/cd934f3c..f202bcbf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=06-07 Stats: 7 lines in 1 file changed: 5 ins; 2 del; 0 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: 8253495: CDS generates non-deterministic output [v7]
On Tue, 15 Mar 2022 08:17:24 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 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 12 additional commits since the > last revision: > > - fixed copyright > - Merge branch 'master' into > 8253495-cds-generateds-non-deterministic-output-2 > - @calvinccheung review: fixed typo > - Added helper function CollectedHeap::zap_filler_array_with > - @kimbarrett comments > - zero GC heap filler arrays > - improvement zeroing of alignment gaps > - Fixed zero build > - Merge branch 'master' into > 8253495-cds-generateds-non-deterministic-output-2 > - fixed test > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/1ab17a39...cd934f3c Build changes look good. - Marked as reviewed by ihse (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v6]
On Mon, 14 Mar 2022 22:07:24 GMT, Calvin Cheung wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added helper function CollectedHeap::zap_filler_array_with
>
> test/hotspot/jtreg/runtime/cds/appcds/javaldr/LockDuringDumpAgent.java line
> 65:
>
>> 63: if (elapsed >= timeout) {
>> 64: System.out.println("This JVM may decide to not
>> launch any Java threads during -Xshare:dump.");
>> 65: System.out.println("This is OK because no string
>> objects be in a locked state during heap dump.");
>
> Should `no string objects be` be `no string objects could be`?
Thanks for the review. I've fixed the comment as you suggested.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v7]
> 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 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 12 additional commits since the last revision: - fixed copyright - Merge branch 'master' into 8253495-cds-generateds-non-deterministic-output-2 - @calvinccheung review: fixed typo - Added helper function CollectedHeap::zap_filler_array_with - @kimbarrett comments - zero GC heap filler arrays - improvement zeroing of alignment gaps - Fixed zero build - Merge branch 'master' into 8253495-cds-generateds-non-deterministic-output-2 - fixed test - ... and 2 more: https://git.openjdk.java.net/jdk/compare/a6344ded...cd934f3c - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/47e0238a..cd934f3c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=05-06 Stats: 6804 lines in 210 files changed: 3347 ins; 1802 del; 1655 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: 8253495: CDS generates non-deterministic output [v6]
On Fri, 11 Mar 2022 06:55:23 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:
>
> Added helper function CollectedHeap::zap_filler_array_with
CDS changes look good. One minor comment on a test.
test/hotspot/jtreg/runtime/cds/appcds/javaldr/LockDuringDumpAgent.java line 65:
> 63: if (elapsed >= timeout) {
> 64: System.out.println("This JVM may decide to not
> launch any Java threads during -Xshare:dump.");
> 65: System.out.println("This is OK because no string
> objects be in a locked state during heap dump.");
Should `no string objects be` be `no string objects could be`?
-
Marked as reviewed by ccheung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Fri, 11 Mar 2022 08:28:32 GMT, Ioi Lam wrote: >> Is reproducibility also a topic for users calling -Xdump with custom JNI >> coding? Or maybe having the VM instrumented somehow? Since it seems such an >> easy fix, I would prevent attaching too. At least the user would get a clear >> error message. > >> Is reproducibility also a topic for users calling -Xdump with custom JNI >> coding? Or maybe having the VM instrumented somehow? Since it seems such an >> easy fix, I would prevent attaching too. At least the user would get a clear >> error message. > > It's impossible to execute arbitrary Java code when running "java > -Xshare:dump", so this means there's no way to load a JNI library when > creating a *static* CDS archive. The loading of JVMTI agents is also not > supported. So this is not a case we need to handle. > > During *dynamic* CDS dumps, arbitrary Java code can execute, but we don't > have a requirement for the *dynamic* CDS archive to be deterministic (at > least not for now). Thanks for that clarification. Never mind then :) - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Fri, 11 Mar 2022 07:13:35 GMT, Thomas Stuefe wrote: > Is reproducibility also a topic for users calling -Xdump with custom JNI > coding? Or maybe having the VM instrumented somehow? Since it seems such an > easy fix, I would prevent attaching too. At least the user would get a clear > error message. It's impossible to execute arbitrary Java code when running "java -Xshare:dump", so this means there's no way to load a JNI library when creating a *static* CDS archive. The loading of JVMTI agents is also not supported. So this is not a case we need to handle. During *dynamic* CDS dumps, arbitrary Java code can execute, but we don't have a requirement for the *dynamic* CDS archive to be deterministic (at least not for now). - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output
On Fri, 11 Mar 2022 07:03:00 GMT, David Holmes wrote: > > Well, he does it for `DumpSharedSpaces` only. Are you really worried about > > that one load+conditional jump? > > As I said (and I'm not the only one who says this :) ) "death by a thousand > cuts". Thank you, that is a good expression. I just wanted to clarify what you meant. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 19:34:29 GMT, Ioi Lam wrote: >> src/hotspot/share/prims/jvm.cpp line 2887: >> >>> 2885: return; >>> 2886: } >>> 2887: #endif >> >> Should we do this for jni_AttachCurrentThread too? > > This hasn't been necessary for me because jni_AttachCurrentThread is not > called during "java -Xshare:dump", which executes under a very strict > condition and doesn't normally allow arbitrary JNI libraries to be loaded. Is reproducibility also a topic for users calling -Xdump with custom JNI coding? Or maybe having the VM instrumented somehow? Since it seems such an easy fix, I would prevent attaching too. At least the user would get a clear error message. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v6]
On Fri, 11 Mar 2022 06:55:23 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: > > Added helper function CollectedHeap::zap_filler_array_with GC changes look good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output
On 11/03/2022 4:50 pm, Thomas Stuefe wrote: On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes wrote: Thanks for pointing this out. I ran more tests and found that on certain platforms, there are other structures that have problems with uninitialized gaps. I ended up changing `os::malloc()` to zero the buffer when running with -Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't matter too much for regular VM executions because `os::malloc()` already has a high overhead. This is raising red flags for me sorry. Every user of the JDK is now paying a penalty because of something only needed when dumping the shared archive. It might not be much but it is the old "death by a thousand cuts". Well, he does it for `DumpSharedSpaces` only. Are you really worried about that one load+conditional jump? As I said (and I'm not the only one who says this :) ) "death by a thousand cuts". @iklam: I dislike the fact that CDS terminology is now in os::malloc. I would give this another flag, "ZapMalloc" or similar, and maybe merge it with the `DEBUG_ONLY(memset(..uninitBlockPad))` above. Is there any way to tell the OS to pre-zero all memory provided to the current process, such that we could set that when dumping and not have to check on each allocation? No. Malloced memory is not provided by the OS but by the glibc, and it may be polluted with whatever the allocator did with it (eg pointers to chain free blocks), or by prior user payload. Glibc has a specific tunable, `glibc.malloc.perturb`, that initializes malloc memory to a given value, but you cannot directly set the value. It is very handy to check for uninitialized memory. Always wanted to add some tests that used it, but never got around. But obviously its nothing you could do in production. And I have to wonder how easy it would be to re-introduce non-deterministic values in these data structures that are being dumped. Does malloc itself even guarantee to return the same set of addresses for the same sequence of requests in different executions of a program? No, of course not. Non-Java threads may still run concurrently, no? System libraries do malloc too. But are pointer *values* even the problem? I assumed that pointer values could be the problem, even if not presently, but apparently all the pointers get rewritten to use offsets from the known base of the shared archive - so not an issue. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v6]
> 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: Added helper function CollectedHeap::zap_filler_array_with - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/584c6572..47e0238a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=04-05 Stats: 10 lines in 2 files changed: 6 ins; 2 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: 8253495: CDS generates non-deterministic output
On Fri, 11 Mar 2022 06:50:00 GMT, David Holmes wrote: > I can combine the tests for `MemTracker::tracking_level()` and > `DumpSharedSpaces` into a single test and do more work only when the uncommon > path is taken. This would require some refactoring of the > MemTracker/MallocTracker code. I'd rather do that in a separate RFE. > > In fact, `MemTracker::_tracking_level` is tested twice in the current > implementation. We can change it to do a single test in the most common case > (NMT_summary) if we really want to cut down the number of tests. But honestly > I don't think this makes any difference. > Before going down that road, I would really like to see some measurements, whether this really matters. malloc is not blindingly fast. The malloc code in glibc does test a lot of conditions too. If you need fast allocation (or well packed, for that matter), you need another allocator. Thats why we have Arenas. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output
On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes wrote: > > Thanks for pointing this out. I ran more tests and found that on certain > > platforms, there are other structures that have problems with uninitialized > > gaps. I ended up changing `os::malloc()` to zero the buffer when running > > with -Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` > > doesn't matter too much for regular VM executions because `os::malloc()` > > already has a high overhead. > > This is raising red flags for me sorry. Every user of the JDK is now paying a > penalty because of something only needed when dumping the shared archive. It > might not be much but it is the old "death by a thousand cuts". Well, he does it for `DumpSharedSpaces` only. Are you really worried about that one load+conditional jump? @iklam: I dislike the fact that CDS terminology is now in os::malloc. I would give this another flag, "ZapMalloc" or similar, and maybe merge it with the `DEBUG_ONLY(memset(..uninitBlockPad))` above. > Is there any way to tell the OS to pre-zero all memory provided to the > current process, such that we could set that when dumping and not have to > check on each allocation? No. Malloced memory is not provided by the OS but by the glibc, and it may be polluted with whatever the allocator did with it (eg pointers to chain free blocks), or by prior user payload. Glibc has a specific tunable, `glibc.malloc.perturb`, that initializes malloc memory to a given value, but you cannot directly set the value. It is very handy to check for uninitialized memory. Always wanted to add some tests that used it, but never got around. But obviously its nothing you could do in production. > > And I have to wonder how easy it would be to re-introduce non-deterministic > values in these data structures that are being dumped. Does malloc itself > even guarantee to return the same set of addresses for the same sequence of > requests in different executions of a program? No, of course not. Non-Java threads may still run concurrently, no? System libraries do malloc too. But are pointer *values* even the problem? - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output
On 11/03/2022 4:40 pm, Ioi Lam wrote:
On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes wrote:
I ended up changing `os::malloc()` to zero the buffer when running with
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't
matter too much for regular VM executions because `os::malloc()` already has a
high overhead.
This is raising red flags for me sorry. Every user of the JDK is now paying a penalty
because of something only needed when dumping the shared archive. It might not be much
but it is the old "death by a thousand cuts". Is there any way to tell the OS
to pre-zero all memory provided to the current process, such that we could set that when
dumping and not have to check on each allocation?
I don't know how to tell the OS (or C library) to zero out the buffer returned
by malloc. However, in the current code path, we already have a test for an
uncommon condition when `os::malloc()` calls `MemTracker::record_malloc()`
which calls `MallocTracker::record_malloc()`
void* MallocTracker::record_malloc(void* malloc_base, size_t size, MEMFLAGS
flags,
const NativeCallStack& stack)
{
if (MemTracker::tracking_level() == NMT_detail) {
MallocSiteTable::allocation_at(stack, size, _marker, flags);
}
I can combine the tests for `MemTracker::tracking_level()` and
`DumpSharedSpaces` into a single test and do more work only when the uncommon
path is taken. This would require some refactoring of the
MemTracker/MallocTracker code. I'd rather do that in a separate RFE.
In fact, `MemTracker::_tracking_level` is tested twice in the current
implementation. We can change it to do a single test in the most common case
(NMT_summary) if we really want to cut down the number of tests. But honestly I
don't think this makes any difference.
And I have to wonder how easy it would be to re-introduce non-deterministic
values in these data structures that are being dumped. Does malloc itself even
guarantee to return the same set of addresses for the same sequence of requests
in different executions of a program?
The malloc'ed objects are copied into the CDS archive at deterministic
addresses. Any pointers inside such objects will be relocated.
Okay. I won't object further but I really don't like it - c'est la vie!
I'll let others review the actual code changes in detail.
Cheers,
David
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output
On Fri, 11 Mar 2022 05:59:00 GMT, David Holmes wrote:
> > I ended up changing `os::malloc()` to zero the buffer when running with
> > -Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't
> > matter too much for regular VM executions because `os::malloc()` already
> > has a high overhead.
>
> This is raising red flags for me sorry. Every user of the JDK is now paying a
> penalty because of something only needed when dumping the shared archive. It
> might not be much but it is the old "death by a thousand cuts". Is there any
> way to tell the OS to pre-zero all memory provided to the current process,
> such that we could set that when dumping and not have to check on each
> allocation?
I don't know how to tell the OS (or C library) to zero out the buffer returned
by malloc. However, in the current code path, we already have a test for an
uncommon condition when `os::malloc()` calls `MemTracker::record_malloc()`
which calls `MallocTracker::record_malloc()`
void* MallocTracker::record_malloc(void* malloc_base, size_t size, MEMFLAGS
flags,
const NativeCallStack& stack)
{
if (MemTracker::tracking_level() == NMT_detail) {
MallocSiteTable::allocation_at(stack, size, _marker, flags);
}
I can combine the tests for `MemTracker::tracking_level()` and
`DumpSharedSpaces` into a single test and do more work only when the uncommon
path is taken. This would require some refactoring of the
MemTracker/MallocTracker code. I'd rather do that in a separate RFE.
In fact, `MemTracker::_tracking_level` is tested twice in the current
implementation. We can change it to do a single test in the most common case
(NMT_summary) if we really want to cut down the number of tests. But honestly I
don't think this makes any difference.
> And I have to wonder how easy it would be to re-introduce non-deterministic
> values in these data structures that are being dumped. Does malloc itself
> even guarantee to return the same set of addresses for the same sequence of
> requests in different executions of a program?
The malloc'ed objects are copied into the CDS archive at deterministic
addresses. Any pointers inside such objects will be relocated.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v4]
On Fri, 11 Mar 2022 05:55:20 GMT, Kim Barrett wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> zero GC heap filler arrays
>
> src/hotspot/share/gc/shared/collectedHeap.cpp line 449:
>
>> 447: allocator.initialize(start);
>> 448: DEBUG_ONLY(zap_filler_array(start, words, zap);)
>> 449: if (DumpSharedSpaces) {
>
> Probably shouldn't both zap and clear for dumping, to avoid wasting time.
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v5]
> 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: @kimbarrett comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/be7673af..584c6572 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=03-04 Stats: 3 lines in 1 file changed: 2 ins; 1 del; 0 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: 8253495: CDS generates non-deterministic output [v4]
On Fri, 11 Mar 2022 04:56:23 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:
>
> zero GC heap filler arrays
Changes requested by kbarrett (Reviewer).
src/hotspot/share/gc/shared/collectedHeap.cpp line 449:
> 447: allocator.initialize(start);
> 448: DEBUG_ONLY(zap_filler_array(start, words, zap);)
> 449: if (DumpSharedSpaces) {
Probably shouldn't both zap and clear for dumping, to avoid wasting time.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
I can't find this comment in the PR so replying via email ...
On 11/03/2022 9:24 am, Ioi Lam wrote:
On Wed, 9 Mar 2022 07:47:19 GMT, Thomas Stuefe wrote:
Ioi Lam has updated the pull request incrementally with one additional commit
since the last revision:
Fixed zero build
src/hotspot/share/utilities/hashtable.hpp line 42:
40:
41: LP64_ONLY(unsigned int _gap;)
42:
For 64-bit, you now lose packing potential in the theoretical case the
following payload does not have to be aligned to 64 bit. E.g. for T=char, where
the whole entry would fit into 8 bytes. Probably does not matter as long as
entries are allocated individually from C-heap which is a lot more wasteful
anyway.
For 32-bit, I think you may have the same problem if the payload starts with a
uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not
you build on 64-bit?
I think setting the memory, or at least the first 8..16 bytes, of the entry to zero
in BasicHashtable::new_entry could be more robust:
(16 bytes in case the payload starts with a long double but that may be
overthinking it :)
template BasicHashtableEntry*
BasicHashtable::new_entry(unsigned int hashValue) {
char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
BasicHashtableEntry* entry = ::new (p) BasicHashtableEntry(hashValue);
return entry;
}
If you are worried about performance, this may also be controlled by a template
parameter, and then you do it just for the system dictionary.
Thanks for pointing this out. I ran more tests and found that on certain
platforms, there are other structures that have problems with uninitialized
gaps. I ended up changing `os::malloc()` to zero the buffer when running with
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't
matter too much for regular VM executions because `os::malloc()` already has a
high overhead.
This is raising red flags for me sorry. Every user of the JDK is now
paying a penalty because of something only needed when dumping the
shared archive. It might not be much but it is the old "death by a
thousand cuts". Is there any way to tell the OS to pre-zero all memory
provided to the current process, such that we could set that when
dumping and not have to check on each allocation?
And I have to wonder how easy it would be to re-introduce
non-deterministic values in these data structures that are being dumped.
Does malloc itself even guarantee to return the same set of addresses
for the same sequence of requests in different executions of a program?
Cheers,
David
-
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v4]
> 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: zero GC heap filler arrays - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/6974021f..be7673af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=02-03 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 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: 8253495: CDS generates non-deterministic output [v3]
> 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: improvement zeroing of alignment gaps - Changes: - all: https://git.openjdk.java.net/jdk/pull/7748/files - new: https://git.openjdk.java.net/jdk/pull/7748/files/44db40f1..6974021f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7748=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7748=01-02 Stats: 8 lines in 2 files changed: 3 ins; 2 del; 3 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: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 07:47:19 GMT, Thomas Stuefe wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed zero build
>
> src/hotspot/share/utilities/hashtable.hpp line 42:
>
>> 40:
>> 41: LP64_ONLY(unsigned int _gap;)
>> 42:
>
> For 64-bit, you now lose packing potential in the theoretical case the
> following payload does not have to be aligned to 64 bit. E.g. for T=char,
> where the whole entry would fit into 8 bytes. Probably does not matter as
> long as entries are allocated individually from C-heap which is a lot more
> wasteful anyway.
>
> For 32-bit, I think you may have the same problem if the payload starts with
> a uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or
> not you build on 64-bit?
>
> I think setting the memory, or at least the first 8..16 bytes, of the entry
> to zero in BasicHashtable::new_entry could be more robust:
>
> (16 bytes in case the payload starts with a long double but that may be
> overthinking it :)
>
>
> template BasicHashtableEntry*
> BasicHashtable::new_entry(unsigned int hashValue) {
> char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
> ::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
> BasicHashtableEntry* entry = ::new (p) BasicHashtableEntry(hashValue);
> return entry;
> }
>
> If you are worried about performance, this may also be controlled by a
> template parameter, and then you do it just for the system dictionary.
Thanks for pointing this out. I ran more tests and found that on certain
platforms, there are other structures that have problems with uninitialized
gaps. I ended up changing `os::malloc()` to zero the buffer when running with
-Xshare:dump. Hopefully one extra check of `if (DumpSharedSpaces)` doesn't
matter too much for regular VM executions because `os::malloc()` already has a
high overhead.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 19:41:03 GMT, Ioi Lam wrote: >> I think he already did. I'm quoting: >> >>> 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. > >> @magicus the issue is not the list of classes dumped, or their format in the >> dump. As Ioi indicated that list is fixed. The issue is with the heap dump >> part of the archive. Running these other threads affects the heap so by not >> running them with end up with a different heap. So the question is whether >> there is anything about having a different heap dumped that we need to be >> concerned about. We dump the heap into the archive for a reason and this >> changes what we dump, > > To be clear, if multiple threads are running, classes could be loaded in a > different order and the symbols will have different orders. This would cause > the vtables in Klass objects to be laid out differently. Trying to fix this > is very difficult. I have a different patch that makes it work but it's just > too complicated. > > So, disabling the threads is also necessary for (easily) sorting the > metaspace objects. @iklam thanks for clarifying. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 13:51:56 GMT, Magnus Ihse Bursie wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > I think he already did. I'm quoting: > >> 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. > @magicus the issue is not the list of classes dumped, or their format in the > dump. As Ioi indicated that list is fixed. The issue is with the heap dump > part of the archive. Running these other threads affects the heap so by not > running them with end up with a different heap. So the question is whether > there is anything about having a different heap dumped that we need to be > concerned about. We dump the heap into the archive for a reason and this > changes what we dump, To be clear, if multiple threads are running, classes could be loaded in a different order and the symbols will have different orders. This would cause the vtables in Klass objects to be laid out differently. Trying to fix this is very difficult. I have a different patch that makes it work but it's just too complicated. So, disabling the threads is also necessary for (easily) sorting the metaspace objects. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 07:51:46 GMT, Thomas Stuefe 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 2887: > >> 2885: return; >> 2886: } >> 2887: #endif > > Should we do this for jni_AttachCurrentThread too? This hasn't been necessary for me because jni_AttachCurrentThread is not called during "java -Xshare:dump", which executes under a very strict condition and doesn't normally allow arbitrary JNI libraries to be loaded. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 13:51:56 GMT, Magnus Ihse Bursie wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixed zero build
>
> I think he already did. I'm quoting:
>
>> 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.
> @magicus I think we need @iklam to weigh in here and explain exactly what the
> "heap dump" consists of and how not running those threads affects its
> contents. Presently the heap dump is potentially different on each run, IIUC,
> only due to the order of its contents, not the contents themselves.
CDS doesn't dump the entire Java heap. Instead, it dumps only a selected
portion of the Java heap. For example, the module graph. The contents of the
dumped objects are always the same, except that the identity hashcode may be
different if multiple threads are executed. The identity hashcode is computed
here, and its value is "sticky" to the first thread that tries to get the
hashcode for an object.
static inline intptr_t get_next_hash(Thread* current, oop obj) {
intptr_t value = 0;
if (hashCode == 0) {
...
} else if (hashCode == 4) {
...
} else { // default hashCode is 5:
// Marsaglia's xor-shift scheme with thread-specific state
// This is probably the best overall implementation -- we'll
// likely make this the default in future releases.
unsigned t = current->_hashStateX;
t ^= (t << 11);
current->_hashStateX = current->_hashStateY;
current->_hashStateY = current->_hashStateZ;
current->_hashStateZ = current->_hashStateW;
unsigned v = current->_hashStateW;
v = (v ^ (v >> 19)) ^ (t ^ (t >> 8));
current->_hashStateW = v;
value = v;
}
So, when the main Java thread tries to store an object `O` into a hashtable
inside the module graph, if the hashcode of `O` has already been computed by a
non-main thread, then the module graph will have unstable contents.
-
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 think he already did. I'm quoting: > 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. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 12:50:58 GMT, Magnus Ihse Bursie wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > Well, previously we'd get different dumps on different runs. If that was an > issue, surely it would have manifested itself by now? With this change, we'll > just get the same dump each run. I fail to see how that could be a risk. @magicus I think we need @iklam to weigh in here and explain exactly what the "heap dump" consists of and how not running those threads affects its contents. Presently the heap dump is potentially different on each run, IIUC, only due to the order of its contents, not the contents themselves. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
The Skara bots messed up this one badly. It was a reply to David's comment, not Ioi's latest push. /Magnus On 2022-03-10 13:56, Magnus Ihse Bursie wrote: 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 Well, previously we'd get different dumps on different runs. If that was an issue, surely it would have manifested itself by now? With this change, we'll just get the same dump each run. I fail to see how that could be a risk. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 07:58:51 GMT, Thomas Stuefe wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > Hi Ioi, > > some questions, comments inline. > > Like David in the comments, I am also a bit vague on the usefulness, but I > may not know the whole story. Is it to enable repackagers like Debian to > check the "reproducable" tickbox on their OpenJDK package? Or is there a > practical need for this? > > Thanks, Thomas > @tstuefe Without commenting on Ioi's actual implementation, let me explain a > bit on the importance of this fix. > > Reproducible builds is not just a "checkbox", any more than "does not crash > on startup" is a checkbox. It is an important security tool. See e.g. > https://reproducible-builds.org/ for more information. > Hi @magicus, thanks for explaining, and for the link. That one was a good explanation. I had no idea, but I'm convinced now. Cheers, Thomas - 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 Well, previously we'd get different dumps on different runs. If that was an issue, surely it would have manifested itself by now? With this change, we'll just get the same dump each run. I fail to see how that could be a risk. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Thu, 10 Mar 2022 12:11:06 GMT, Magnus Ihse Bursie wrote: >> The "heap dump" aspect of this is not something I'm familiar with, but if >> the threads don't affect the list of classes dumped, they surely must affect >> what is in the heap dump otherwise their execution would not be an issue. So >> you must be sacrificing something by not having these threads start. > > @dholmes-ora That something is "sacrificed" does not follow from that > something is "different". The list of classes to dump is specified in the > lib/classlist file, which is generated during the build. > > The process of creating this involves running a suitable "exercise most > important parts" java program, and logging the classes loaded. This class > file is then post-processed (sorted) to make sure it is reproducible for the > same JDK code base. > > As Ioi say, in this case threads are started freely, and may run in any > non-deterministic order. > > At the next stage, we take this file (which is just done implicitly by > -Xshare:dump), and generate the actual CDS archive, classes.jsa. Now it turns > out this generation is non-deterministic. And Ioi's analysis is that this is > due to thread non-determinism. So if we just disable threads during the dump > process (where we are not really running the JVM "actually" -- it's a special > mode, where we don't even have a Java program to run!), there's no harm in > that. @magicus the issue is not the list of classes dumped, or their format in the dump. As Ioi indicated that list is fixed. The issue is with the heap dump part of the archive. Running these other threads affects the heap so by not running them with end up with a different heap. So the question is whether there is anything about having a different heap dumped that we need to be concerned about. We dump the heap into the archive for a reason and this changes what we dump, - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 11:45:59 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > The "heap dump" aspect of this is not something I'm familiar with, but if the > threads don't affect the list of classes dumped, they surely must affect what > is in the heap dump otherwise their execution would not be an issue. So you > must be sacrificing something by not having these threads start. @dholmes-ora That something is "sacrificed" does not follow from that something is "different". The list of classes to dump is specified in the lib/classlist file, which is generated during the build. The process of creating this involves running a suitable "exercise most important parts" java program, and logging the classes loaded. This class file is then post-processed (sorted) to make sure it is reproducible for the same JDK code base. As Ioi say, in this case threads are started freely, and may run in any non-deterministic order. At the next stage, we take this file (which is just done implicitly by -Xshare:dump), and generate the actual CDS archive, classes.jsa. Now it turns out this generation is non-deterministic. And Ioi's analysis is that this is due to thread non-determinism. So if we just disable threads during the dump process (where we are not really running the JVM "actually" -- it's a special mode, where we don't even have a Java program to run!), there's no harm in that. - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v2]
On Wed, 9 Mar 2022 07:58:51 GMT, Thomas Stuefe wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fixed zero build > > Hi Ioi, > > some questions, comments inline. > > Like David in the comments, I am also a bit vague on the usefulness, but I > may not know the whole story. Is it to enable repackagers like Debian to > check the "reproducable" tickbox on their OpenJDK package? Or is there a > practical need for this? > > Thanks, Thomas @tstuefe Without commenting on Ioi's actual implementation, let me explain a bit on the importance of this fix. Reproducible builds is not just a "checkbox", any more than "does not crash on startup" is a checkbox. It is an important security tool. See e.g. https://reproducible-builds.org/ for more information. The problem with CDS generating non-deterministic output is that during the build process we generate the file classes.jsa (and classes_nocoops.jsa). These files in turn are included in the java.base jmod, which in turn is included in the entire jlinked image. So if classes.jsa gets random bits, these random bits propagate to java.base.jmod and finally, to the entire jimage. This means that it is imposslbe to get bit-by-bit reproducibility verification of the entire JDK build. For several years, we have relentlessly (albeit with an unfortunately low priority) addressed and fixed indeterminism in the build of the JDK. We are now at the point were the only major issue is the randomness of classes.jsa and classes_nocoops.jsa. - 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 The "heap dump" aspect of this is not something I'm familiar with, but if the threads don't affect the list of classes dumped, they surely must affect what is in the heap dump otherwise their execution would not be an issue. So you must be sacrificing something by not having these threads start. - 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
Hi Ioi,
some questions, comments inline.
Like David in the comments, I am also a bit vague on the usefulness, but I may
not know the whole story. Is it to enable repackagers like Debian to check the
"reproducable" tickbox on their OpenJDK package? Or is there a practical need
for this?
Thanks, Thomas
src/hotspot/share/prims/jvm.cpp line 2887:
> 2885: return;
> 2886: }
> 2887: #endif
Should we do this for jni_AttachCurrentThread too?
src/hotspot/share/utilities/hashtable.hpp line 42:
> 40:
> 41: LP64_ONLY(unsigned int _gap;)
> 42:
For 64-bit, you now lose packing potential in the theoretical case the
following payload does not have to be aligned to 64 bit. E.g. for T=char, where
the whole entry would fit into 8 bytes. Probably does not matter as long as
entries are allocated individually from C-heap which is a lot more wasteful
anyway.
For 32-bit, I think you may have the same problem if the payload starts with a
uint64_t. Would that not be aligned to a 64-bit boundary too? Whether or not
you build on 64-bit?
I think setting the memory, or at least the first 8..16 bytes, of the entry to
zero in BasicHashtable::new_entry could be more robust:
(16 bytes in case the payload starts with a long double but that may be
overthinking it :)
template BasicHashtableEntry*
BasicHashtable::new_entry(unsigned int hashValue) {
char* p = :new (NEW_C_HEAP_ARRAY(char, this->entry_size(), F);
::memset(p, 0, MIN2(this->entry_size(), 16)); // needs reproducable
BasicHashtableEntry* entry = ::new (p) BasicHashtableEntry(hashValue);
return entry;
}
If you are worried about performance, this may also be controlled by a template
parameter, and then you do it just for the system dictionary.
-
PR: https://git.openjdk.java.net/jdk/pull/7748
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: 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
