Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make [v4]
On Tue, 7 Jun 2022 17:39:14 GMT, Leo Korinth wrote: >> One can select a testcase by ID when running a jtreg test case directly from >> jtreg (using the testcase.java#testID syntax). However, this has not been >> possible to do when launching jtreg indirectly from make. >> >> This fix attempts to address this issue. I have not tested this thoroughly >> yet, I wanted to show the code to get feedback first. The idea is to *not* >> emulated destructive imperative assignments through the use of eval. I >> instead try to handle it in a functional style without reassigning >> variables. I have tried to make the change as small as possible. >> >> I am not used to change the build system, so I would appreciate thorough >> review. >> >> `IfAppend` and `IfPrepend` are general purpose functions. If similar >> functions exists elsewhere inside the code base or in make itself I should >> probably use those instead. If they do not exist, they might be useful >> elsewhere and maybe should be placed in a common make file instead of the >> RunTests.gmk file. > > Leo Korinth has updated the pull request incrementally with one additional > commit since the last revision: > > wip Marked as reviewed by iklam (Reviewer). make/RunTests.gmk line 357: > 355: $(subst .java,,$(subst .java,,$(suffix $(notdir $1 > 356: > 357: # The test selector starting with a hash (#testid) will be stripped by > all Small nit - I think "selection" and "test selector" in the comments should be changed to "test id" so you don't call the same thing with 3 different names. But otherwise the change looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/9028
Re: RFR: 8287828: Fix so that one can select jtreg test case by ID from make
On Mon, 6 Jun 2022 10:48:05 GMT, David Holmes wrote: > Let me ask the obvious "dumb" question ... why does this have to be so > complicated? Why isn't the name of the test simply passed through to jtreg as > typed? Is it because `#` is treated as comment by the shell? Could it be encoded by something like `TestName.java%38testID`? - PR: https://git.openjdk.java.net/jdk/pull/9028
Re: pre-submit tests for github PRs
On 5/23/2022 4:41 AM, Alan Bateman wrote: On 22/05/2022 23:58, David Holmes wrote: GHA tests a range of OpenJDK ports, not just the "mainstream platforms". The existing linux-86 failures (and others) are due to the Loom integration which only fully supports a couple of platforms and which broke all the other ports upon initial integration. Some folks in the community have been working through things to fix that. GHA is configured to cross build for most of these ports/configurations and they should pass. Running them is another matter of course. GHA does run some tests on linux-x86 and the x86_32 port does need work before it can run the tests with --enable-preview. Aleksey Shipilëv seems to be making good progress on it. -Alan Maybe it makes sense to disable the x86 GHA runs before the underlying issue is fixed? There's too much noise.
Integrated: 8282828: CDS uncompressed oops archive is not deterministic
On Fri, 29 Apr 2022 22:50:45 GMT, Ioi Lam wrote: > During `java -Xshare:dump -XX:-UseCompressedOops`, the location of the Java > heap is chosen by the OS. Due to Address Space Layout Randomization, the heap > will always start at a different location. This causes the archive for > uncompressed oops ($JAVA_HOME/lib/server/classes_nocoops.jsa) to be > non-deterministic. > > The fix is to patch the archived object pointers to make it look like the > heap starts at a fixed address -- I chose 0x1000, but the exact value > doesn't really matter. > > At runtime, the object pointers will be patched again according to the real > location of the heap. > > Tested with tiers 1-5. I am running builds-tier5 several times to test the > xxx-cmp-baseline builds. This pull request has now been integrated. Changeset: 64b5b2b0 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/64b5b2b0b3e5156d9b0c5f378ce3a1ae52aa0819 Stats: 86 lines in 6 files changed: 62 ins; 2 del; 22 mod 8282828: CDS uncompressed oops archive is not deterministic Reviewed-by: erikj, ihse, ccheung - PR: https://git.openjdk.java.net/jdk/pull/8478
Re: RFR: 8282828: CDS uncompressed oops archive is not deterministic [v2]
On Mon, 2 May 2022 12:56:36 GMT, Erik Joelsson wrote: >> 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 two additional commits >> since the last revision: >> >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> 8282828-uncompressed-oop-cds-archive-not-determinisic >> - 8282828: CDS uncompressed oops archive is not deterministic > > Change to compare.sh looks good. Thanks for fixing this! Thanks @erikj79 @magicus @calvinccheung for the review. - PR: https://git.openjdk.java.net/jdk/pull/8478
Re: RFR: 8282828: CDS uncompressed oops archive is not deterministic [v2]
> During `java -Xshare:dump -XX:-UseCompressedOops`, the location of the Java > heap is chosen by the OS. Due to Address Space Layout Randomization, the heap > will always start at a different location. This causes the archive for > uncompressed oops ($JAVA_HOME/lib/server/classes_nocoops.jsa) to be > non-deterministic. > > The fix is to patch the archived object pointers to make it look like the > heap starts at a fixed address -- I chose 0x1000, but the exact value > doesn't really matter. > > At runtime, the object pointers will be patched again according to the real > location of the heap. > > Tested with tiers 1-5. I am running builds-tier5 several times to test the > xxx-cmp-baseline builds. 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 two additional commits since the last revision: - Merge branch 'master' of https://github.com/openjdk/jdk into 8282828-uncompressed-oop-cds-archive-not-determinisic - 8282828: CDS uncompressed oops archive is not deterministic - Changes: - all: https://git.openjdk.java.net/jdk/pull/8478/files - new: https://git.openjdk.java.net/jdk/pull/8478/files/f0261c51..8ddea12f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8478=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8478=00-01 Stats: 15990 lines in 519 files changed: 10708 ins; 2732 del; 2550 mod Patch: https://git.openjdk.java.net/jdk/pull/8478.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8478/head:pull/8478 PR: https://git.openjdk.java.net/jdk/pull/8478
Re: RFR: 8282828: CDS uncompressed oops archive is not deterministic
On Fri, 29 Apr 2022 22:50:45 GMT, Ioi Lam wrote: > During `java -Xshare:dump -XX:-UseCompressedOops`, the location of the Java > heap is chosen by the OS. Due to Address Space Layout Randomization, the heap > will always start at a different location. This causes the archive for > uncompressed oops ($JAVA_HOME/lib/server/classes_nocoops.jsa) to be > non-deterministic. > > The fix is to patch the archived object pointers to make it look like the > heap starts at a fixed address -- I chose 0x1000, but the exact value > doesn't really matter. > > At runtime, the object pointers will be patched again according to the real > location of the heap. > > Tested with tiers 1-5. I am running builds-tier5 several times to test the > xxx-cmp-baseline builds. src/hotspot/share/cds/heapShared.cpp line 184: > 182: // Do not call p->identity_hash() as that will update the > 183: // object header. > 184: return primitive_hash(cast_from_oop(p)); This fix doesn't affect the contents of the archive, but it's necessary to avoid incorrect output from `-Xlog:cds+map=trace`. - PR: https://git.openjdk.java.net/jdk/pull/8478
RFR: 8282828: CDS uncompressed oops archive is not deterministic
During `java -Xshare:dump -XX:-UseCompressedOops`, the location of the Java heap is chosen by the OS. Due to Address Space Layout Randomization, the heap will always start at a different location. This causes the archive for uncompressed oops ($JAVA_HOME/lib/server/classes_nocoops.jsa) to be non-deterministic. The fix is to patch the archived object pointers to make it look like the heap starts at a fixed address -- I chose 0x1000, but the exact value doesn't really matter. At runtime, the object pointers will be patched again according to the real location of the heap. Tested with tiers 1-5. I am running builds-tier5 several times to test the xxx-cmp-baseline builds. - Commit messages: - 8282828: CDS uncompressed oops archive is not deterministic Changes: https://git.openjdk.java.net/jdk/pull/8478/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8478=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282828 Stats: 86 lines in 6 files changed: 62 ins; 2 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/8478.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8478/head:pull/8478 PR: https://git.openjdk.java.net/jdk/pull/8478
Integrated: 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). This pull request has now been integrated. Changeset: de4f04cb Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/de4f04cb71a26ce03b96460cb8d1c1e28cd1ed38 Stats: 100 lines in 15 files changed: 69 ins; 9 del; 22 mod 8253495: CDS generates non-deterministic output Reviewed-by: erikj, kbarrett, ccheung, ihse - 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 [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 [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 [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 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]
> 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 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 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]
> 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
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: Does CDS archive generation work for crossbuilds?
On 11/5/21 9:38 AM, Magnus Ihse Bursie wrote: On 2021-10-29 21:35, Anton Kozlov wrote: QEMU user space emulation [1] works pretty fast. On my linux-x86 laptop native java -Xshare:dump completes in 0.4 sec, and with qemu-aarch64 it's about 2.3 sec. GHA now provides a runnable instruction on how to create the sysroot [2]. But that time excludes booting up the virtual machine, I presume. I think user space emulation doesn't spin up a new virtual machine. It runs on the same kernel as the build host. QEMU does the translation between the the user program and the OS (system calls, signals, etc). So the start-up overhead is pretty minimal. Nevertheless, if someone is interested in going down this road to get CDS generation for cross-compilation, I would not object to putting it in the build system. I filed https://bugs.openjdk.java.net/browse/JDK-8276791 in case there are any volunteers :-) Thanks - Ioi /Magnus Regarding problems, different page size for linux-aarch64 first comes to mind, but it should be fixed by JDK-8268396: "CDS archive with 4K alignment unusable on machines with 64k pages" Thanks, Anton [1] https://www.qemu.org/docs/master/user/main.html [2] https://github.com/openjdk/jdk/blob/master/.github/workflows/submit.yml#L514 On 10/29/21 14:35, Magnus Ihse Bursie wrote: On 2021-10-28 22:56, Ioi Lam wrote: How reliable would it be to use qemu to run the cross-compiled binaries? Has anyone tried that recently? I experimented with qemu and the JDK build some years ago (so not really recently). As I remember it, the takeaway was that it probably was reliable, but it was slow as h*ll, up to the point of being practically unusable. Add to this the fact that you need to prepare an entire OS image, complete with all tools needed to build the JDK... I set up a few such images for my own use (I think it was emulating linux-aarch64 on x64) to run testing. I had an idea of describing how I did to share the knowledge, but in the end it was just too complicated and slow to even consider recommending. /Magnus On 10/23/21 5:48 AM, Thomas Stüfe wrote: Hi Alan, On Sat, Oct 23, 2021 at 9:58 AM Alan Bateman wrote: On 23/10/2021 07:57, Thomas Stüfe wrote: Hi, when I crossbuild (for linux aarch64, using a devkit, building on linux x64), for some reason I don't get the classes.jsa generated inside the images directory. My configure options: --with-devkit=/shared/projects/openjdk/devkits/x86_64-linux-gnu-to-aarch64-linux-gnu --openjdk-target=aarch64-linux-gnu --with-boot-jdk=/shared/projects/openjdk/jdks/sapmachine17 --with-build-jdk=/shared/projects/openjdk/jdk-jdk/output-release/images/jdk --with-gtest=/shared/projects/openjdk/gtest/googletest --with-debug-level=fastdebug The build jdk is a freshly build x64 release VM from the same source tree. Am I missing something obvious? Is CDS archive generation even supported for crossbuilds? It needs the generate run-time to execute "java -Xshare:dump" so I don't expect so. hotspot-runtime-dev is probably the place to discuss the details. BTW: this came up recently in the context of the jlink plugin that generates the CDS archive. The plugin needed a check to ensure that the target platform matched the current platform as it could launch the target VM to create the dump. Thinking for a second, probably it cannot work since we copy binary structures verbatim to the archive; I guess the chance that they are binary compatible between platforms is very small. But it should be easily rectified by calling Xshare:dump on the target platform. Thank you! ..Thomas -Alan
Re: Does CDS archive generation work for crossbuilds?
How reliable would it be to use qemu to run the cross-compiled binaries? Has anyone tried that recently? On 10/23/21 5:48 AM, Thomas Stüfe wrote: Hi Alan, On Sat, Oct 23, 2021 at 9:58 AM Alan Bateman wrote: On 23/10/2021 07:57, Thomas Stüfe wrote: Hi, when I crossbuild (for linux aarch64, using a devkit, building on linux x64), for some reason I don't get the classes.jsa generated inside the images directory. My configure options: --with-devkit=/shared/projects/openjdk/devkits/x86_64-linux-gnu-to-aarch64-linux-gnu --openjdk-target=aarch64-linux-gnu --with-boot-jdk=/shared/projects/openjdk/jdks/sapmachine17 --with-build-jdk=/shared/projects/openjdk/jdk-jdk/output-release/images/jdk --with-gtest=/shared/projects/openjdk/gtest/googletest --with-debug-level=fastdebug The build jdk is a freshly build x64 release VM from the same source tree. Am I missing something obvious? Is CDS archive generation even supported for crossbuilds? It needs the generate run-time to execute "java -Xshare:dump" so I don't expect so. hotspot-runtime-dev is probably the place to discuss the details. BTW: this came up recently in the context of the jlink plugin that generates the CDS archive. The plugin needed a check to ensure that the target platform matched the current platform as it could launch the target VM to create the dump. Thinking for a second, probably it cannot work since we copy binary structures verbatim to the archive; I guess the chance that they are binary compatible between platforms is very small. But it should be easily rectified by calling Xshare:dump on the target platform. Thank you! ..Thomas -Alan
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern [v3]
On Wed, 6 Oct 2021 05:17:28 GMT, Jie Fu wrote:
>> Hi all,
>>
>> I tried to build OpenJDK on Cygwin (Windows 2016 + VS2019).
>> However, I failed with C4474 and C4778 warnings as below:
>>
>> Compiling 100 properties into resource bundles for java.desktop
>> Compiling 3038 files for java.base
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): error
>> C2220: the following warning is treated as an error
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
>> C4778: 'sscanf' : unterminated format string
>> '%255[*\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e\x97%n'
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
>> C4474: 'sscanf' : too many arguments passed for format string
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): note:
>> placeholders and their parameters expect 1 variadic arguments, but 3 were
>> provided
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
>> C4778: 'sscanf' : unterminated format string
>> '%1022[[);/\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e%n'
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
>> C4474: 'sscanf' : too many arguments passed for format string
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): note:
>> placeholders and their parameters expect 0 variadic arguments, but 2 were
>> provided
>>
>>
>> The failure is caused by non-ASCII chars in the format string of sscanf
>> [1][2], which is non-portable on our Windows platform.
>> In fact, these non-ASCII coding also triggers C4819 warning, which had been
>> disabled in JDK-8216154 [3].
>> And I also found an article showing that sscanf may fail with non-ASCII in
>> the format string [4].
>>
>> So it would be nice to remove these non-ASCII chars (`\x80 ~ \xef`).
>> And I think it's safe to do so.
>>
>> This is because:
>> 1) There are actually no non-ASCII chars for package/class/method/signature
>> names.
>> 2) I don't think there is a use case, in which people will input non-ASCII
>> for `CompileCommand`.
>>
>> You may argue that the non-ASCII may be used by the parser itself.
>> But I didn't find that usage at all. (Please let me know if I miss
>> something.)
>>
>> So I suggest to remove these non-ASCII code to make HotSpot to be more
>> portable.
>> And if we do so, we can also remove the only one
>> `PRAGMA_DISABLE_MSVC_WARNING(4819)` [5].
>>
>> Testing:
>> - Build tests on Windows
>> - tier1~3 on Linux/x64
>>
>> Thanks.
>> Best regards,
>> Jie
>>
>> [1]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L269
>> [2]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L319
>> [3]
>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-January/032014.html
>> [4] https://jeffpar.github.io/kbarchive/kb/047/Q47369/
>> [5]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L246
>
> Jie Fu has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Split with RANGEBASE_ASCII and RANGEBASE_NON_ASCII
Marked as reviewed by iklam (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/5704
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern [v2]
On Wed, 6 Oct 2021 02:33:30 GMT, Jie Fu wrote:
>> Hi all,
>>
>> I tried to build OpenJDK on Cygwin (Windows 2016 + VS2019).
>> However, I failed with C4474 and C4778 warnings as below:
>>
>> Compiling 100 properties into resource bundles for java.desktop
>> Compiling 3038 files for java.base
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): error
>> C2220: the following warning is treated as an error
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
>> C4778: 'sscanf' : unterminated format string
>> '%255[*\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e\x97%n'
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
>> C4474: 'sscanf' : too many arguments passed for format string
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): note:
>> placeholders and their parameters expect 1 variadic arguments, but 3 were
>> provided
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
>> C4778: 'sscanf' : unterminated format string
>> '%1022[[);/\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e%n'
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
>> C4474: 'sscanf' : too many arguments passed for format string
>> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): note:
>> placeholders and their parameters expect 0 variadic arguments, but 2 were
>> provided
>>
>>
>> The failure is caused by non-ASCII chars in the format string of sscanf
>> [1][2], which is non-portable on our Windows platform.
>> In fact, these non-ASCII coding also triggers C4819 warning, which had been
>> disabled in JDK-8216154 [3].
>> And I also found an article showing that sscanf may fail with non-ASCII in
>> the format string [4].
>>
>> So it would be nice to remove these non-ASCII chars (`\x80 ~ \xef`).
>> And I think it's safe to do so.
>>
>> This is because:
>> 1) There are actually no non-ASCII chars for package/class/method/signature
>> names.
>> 2) I don't think there is a use case, in which people will input non-ASCII
>> for `CompileCommand`.
>>
>> You may argue that the non-ASCII may be used by the parser itself.
>> But I didn't find that usage at all. (Please let me know if I miss
>> something.)
>>
>> So I suggest to remove these non-ASCII code to make HotSpot to be more
>> portable.
>> And if we do so, we can also remove the only one
>> `PRAGMA_DISABLE_MSVC_WARNING(4819)` [5].
>>
>> Testing:
>> - Build tests on Windows
>> - tier1~3 on Linux/x64
>>
>> Thanks.
>> Best regards,
>> Jie
>>
>> [1]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L269
>> [2]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L319
>> [3]
>> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-January/032014.html
>> [4] https://jeffpar.github.io/kbarchive/kb/047/Q47369/
>> [5]
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L246
>
> Jie Fu has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Disable non-ASCII for Windows only
The idea looks good to me. I just have a suggestion to make the code more
readable.
src/hotspot/share/compiler/methodMatcher.cpp line 77:
> 75: "\x60\x61\x62\x63\x64\x65\x66\x67\x68\x69\x6a\x6b\x6c\x6d\x6e\x6f" \
> 76: "\x70\x71\x72\x73\x74\x75\x76\x77\x78\x79\x7a\x7b\x7c\x7d\x7e\x7f"
> 77: #endif
It's hard to tell what's the difference between these two RANGEBASE
definitions. How about doing it like this to make the code more readable?
#define RANGEBASE_ASCII "."
#define RANGEBASE_NON_ASCII "."
#ifdef WINDOWS
#define RANGEBASE RANGEBASE_ASCII
#else
#define RANGEBASE RANGEBASE_ASCII RANGEBASE_NON_ASCII
#endif
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5704
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
On Sun, 26 Sep 2021 09:55:00 GMT, Jie Fu wrote:
> Hi all,
>
> I tried to build OpenJDK on Cygwin (Windows 2016 + VS2019).
> However, I failed with C4474 and C4778 warnings as below:
>
> Compiling 100 properties into resource bundles for java.desktop
> Compiling 3038 files for java.base
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): error
> C2220: the following warning is treated as an error
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
> C4778: 'sscanf' : unterminated format string
> '%255[*\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e\x97%n'
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
> C4474: 'sscanf' : too many arguments passed for format string
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): note:
> placeholders and their parameters expect 1 variadic arguments, but 3 were
> provided
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
> C4778: 'sscanf' : unterminated format string
> '%1022[[);/\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e%n'
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
> C4474: 'sscanf' : too many arguments passed for format string
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): note:
> placeholders and their parameters expect 0 variadic arguments, but 2 were
> provided
>
>
> The failure is caused by non-ASCII chars in the format string of sscanf
> [1][2], which is non-portable on our Windows platform.
> In fact, these non-ASCII coding also triggers C4819 warning, which had been
> disabled in JDK-8216154 [3].
> And I also found an article showing that sscanf may fail with non-ASCII in
> the format string [4].
>
> So it would be nice to remove these non-ASCII chars (`\x80 ~ \xef`).
> And I think it's safe to do so.
>
> This is because:
> 1) There are actually no non-ASCII chars for package/class/method/signature
> names.
> 2) I don't think there is a use case, in which people will input non-ASCII
> for `CompileCommand`.
>
> You may argue that the non-ASCII may be used by the parser itself.
> But I didn't find that usage at all. (Please let me know if I miss something.)
>
> So I suggest to remove these non-ASCII code to make HotSpot to be more
> portable.
> And if we do so, we can also remove the only one
> `PRAGMA_DISABLE_MSVC_WARNING(4819)` [5].
>
> Testing:
> - Build tests on Windows
> - tier1~3 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L269
> [2]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L319
> [3]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-January/032014.html
> [4] https://jeffpar.github.io/kbarchive/kb/047/Q47369/
> [5]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L246
My experiments above with ` -XX:CompileCommand='compileonly,*::爪哇'` was done on
Linux. I tried doing the same on Windows. On US-English Windows, the default
codepage is 437 (DOS Latin US). If I change it to 65001 (UTF8) then Java is
able to output CJK characters to the console.
public class CJK {
public static void main(String args[]) {
System.out.println(args[0]);
\u722a\u54c7();
}
static void \u722a\u54c7() { // Chinese word for "Java"
Thread.dumpStack();
}
}
c:\ade>chcp
Active code page: 437
c:\ade>jdk-17\bin\java -cp . CJK 123
123
java.lang.Exception: Stack trace
at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
at CJK.??(CJK.java:8)
at CJK.main(CJK.java:4)
c:\ade>chcp 65001
Active code page: 65001
c:\ade>jdk-17\bin\java -cp . CJK 爪哇
??
java.lang.Exception: Stack trace
at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
at CJK.爪哇(CJK.java:8)
at CJK.main(CJK.java:4)
As you can see, the CJK characters in the command-line arguments can't even be
correctly passed as arguments to the Java main class. If that doesn't work, I
can't see how we can get `-XX:CompileCommand` to work with CJK characters.
-
PR: https://git.openjdk.java.net/jdk/pull/5704
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
On Thu, 30 Sep 2021 23:04:17 GMT, Jie Fu wrote: > I will do your experiment next week. This is because it's already our > National Day week and I can't find an English Windows machine until next > week. I'll let you know the result as soon as possible. Thanks. No need to hurry :-). In case you can't find an English Windows, I think you can use the `chcp 65001` command mentioned in https://stackoverflow.com/questions/388490/how-to-use-unicode-characters-in-windows-command-line to change your command-line window to use the UTF8 codepage. - PR: https://git.openjdk.java.net/jdk/pull/5704
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
On Sun, 26 Sep 2021 09:55:00 GMT, Jie Fu wrote:
> Hi all,
>
> I tried to build OpenJDK on Cygwin (Windows 2016 + VS2019).
> However, I failed with C4474 and C4778 warnings as below:
>
> Compiling 100 properties into resource bundles for java.desktop
> Compiling 3038 files for java.base
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): error
> C2220: the following warning is treated as an error
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
> C4778: 'sscanf' : unterminated format string
> '%255[*\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e\x97%n'
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): warning
> C4474: 'sscanf' : too many arguments passed for format string
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(269): note:
> placeholders and their parameters expect 1 variadic arguments, but 3 were
> provided
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
> C4778: 'sscanf' : unterminated format string
> '%1022[[);/\x01\x02\x03\x04\x05\x06\a\b\n\v\f\r\x0e\x0f\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f!"#$%&'*+,-0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ\^_`abcdefghijklmnopqrstuvwxyz{|}~\xe2\x82\xac\xe4\xba\x97\xe5\x84\x8e\xe5\x8e%n'
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): warning
> C4474: 'sscanf' : too many arguments passed for format string
> e:\jiefu\ws\jdk\src\hotspot\share\compiler\methodMatcher.cpp(319): note:
> placeholders and their parameters expect 0 variadic arguments, but 2 were
> provided
>
>
> The failure is caused by non-ASCII chars in the format string of sscanf
> [1][2], which is non-portable on our Windows platform.
> In fact, these non-ASCII coding also triggers C4819 warning, which had been
> disabled in JDK-8216154 [3].
> And I also found an article showing that sscanf may fail with non-ASCII in
> the format string [4].
>
> So it would be nice to remove these non-ASCII chars (`\x80 ~ \xef`).
> And I think it's safe to do so.
>
> This is because:
> 1) There are actually no non-ASCII chars for package/class/method/signature
> names.
> 2) I don't think there is a use case, in which people will input non-ASCII
> for `CompileCommand`.
>
> You may argue that the non-ASCII may be used by the parser itself.
> But I didn't find that usage at all. (Please let me know if I miss something.)
>
> So I suggest to remove these non-ASCII code to make HotSpot to be more
> portable.
> And if we do so, we can also remove the only one
> `PRAGMA_DISABLE_MSVC_WARNING(4819)` [5].
>
> Testing:
> - Build tests on Windows
> - tier1~3 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L269
> [2]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L319
> [3]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-January/032014.html
> [4] https://jeffpar.github.io/kbarchive/kb/047/Q47369/
> [5]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/compiler/methodMatcher.cpp#L246
The current limitations of the MethodMather class are:
[1] `parse_method_pattern(char*& line, ...)` requires `line` to be a
UTF8-encoded byte sequence. Essentially, this means that for -XX:CompileCommand
to support non-ASCII characters, the JVM (and the shell that runs the JVM) must
be using UTF8 character encoding.
Note that a "locale" contains 3 parts: language, country and character
encoding. For example,
- en_US.utf8 (English language, United States, UTF8 character encoding)
- zh_CN.utf8 (Chinese language, China, UTF8 character encoding)
- zh_CN.gbk (Chinese language, China, GBK character encoding)
The first two support non-ASCII characters in -XX:CompileCommand, but the
third one doesn't.
[2] MethodMather uses `sscanf`. It assumes that the JVM is running with UTF8
character encoding because
- It uses `char*` strings returned by `sscanf` to compare with the bytes stored
in Symbols. This requires `sscanf` to return strings that are encoded in UTF8,
because Symbols stores the string with UTF8-encoded bytes.
- It relies on range checking by `sscanf` to enforce the following
restrictions. However, these restrictions are given with the RANGE macro which
is UTF8 encoded bytes (and I suspect that these are incorrect when handling
multi-byte UTF8-encoded characters):
// '\0' and 0xf0-0xff are disallowed in constant string values
// 0x20 ' ', 0x09 '\t' and, 0x2c ',' are used in the matching
// 0x5b '[' and 0x5d ']' can not be used because of the matcher
// 0x28 '(' and 0x29 ')' are used for the signature
// 0x2e '.' is always replaced before the matching
// 0x2f '/' is only
Re: RFR: 8274329: Fix non-portable HotSpot code in MethodMatcher::parse_method_pattern
On Thu, 30 Sep 2021 08:31:51 GMT, Jie Fu wrote:
> > `RANGEBASE` was added by
> > [JDK-6500501](https://bugs.openjdk.java.net/browse/JDK-6500501) and later
> > was modified by
> > [JDK-8027829](https://bugs.openjdk.java.net/browse/JDK-8027829)
> > Note the original comment from 6500501:
> > ```
> > // The characters allowed in a class or method name. All characters > 0x7f
> > // are allowed in order to handle obfuscated class files (e.g. Volano)
> > ```
>
> Thanks @vnkozlov for your very helpful comments.
>
> I have one question: how can we specify (non-ascii chars) and (non-printable
> ascii chars) through `-XX:CompileCommand`?
>
> I just learned from https://bugs.openjdk.java.net/browse/JDK-8027829 that we
> can use unicode like `\u`. But it doesn't work in my experiments.
>
> My example was made from:
> https://bugs.openjdk.java.net/secure/attachment/17128/UnicodeIdentifierTest.java
>
> ```
> public class UnicodeIdentifierTest {
> public static void main(String args[]) {
> System.out.println("Can I use \\u0001 in identifier name? " +
>(Character.isJavaIdentifierPart(1) ? "yes" :
> "no"));
> for (int i = 0; i < 10; i++ )
> methodWithUnicode\u0001Char();
>
> System.out.println("Can I use \\u00aa in identifier name? " +
>(Character.isJavaIdentifierPart(0xaa) ? "yes" :
> "no"));
> for (int i = 0; i < 10; i++ )
> methodWithUnicode\u00aaChar();
>
> System.out.println("Can I use \\u006b in identifier name? " +
>(Character.isJavaIdentifierPart(0x6b) ? "yes" :
> "no"));
> for (int i = 0; i < 10; i++ )
> methodWithUnicode\u006bChar();
>
> }
> public static int a = 0;
> public static void methodWithUnicode\u0001Char() {
> a++;
> }
>
> public static void methodWithUnicode\u00aaChar() {
> a++;
> }
>
> public static void methodWithUnicode\u006bChar() {
> a++;
> }
> }
> ```
>
> And I tried to exclude some specific methods like this
>
> ```
> ${JDK}/bin/java \
>-XX:+PrintCompilation \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest::methodWithUnicode\u0001Char"` \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest.methodWithUnicode\u0001Char"` \
>
> -XX:CompileCommand=exclude,"UnicodeIdentifierTest.methodWithUnicode\u0001Char"
> \
>
> -XX:CompileCommand=exclude,'UnicodeIdentifierTest.methodWithUnicode\u0001Char'
> \
>
> -XX:CompileCommand=exclude,UnicodeIdentifierTest.methodWithUnicode\u0001Char \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest::methodWithUnicode\u00aaChar"` \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest.methodWithUnicode\u00aaChar"` \
>
> -XX:CompileCommand=exclude,"UnicodeIdentifierTest.methodWithUnicode\u00aaChar"
> \
>
> -XX:CompileCommand=exclude,'UnicodeIdentifierTest.methodWithUnicode\u00aaChar'
> \
>
> -XX:CompileCommand=exclude,UnicodeIdentifierTest.methodWithUnicode\u00aaChar \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest::methodWithUnicode\u006bChar"` \
>-XX:CompileCommand=exclude,`echo -e
> "UnicodeIdentifierTest.methodWithUnicode\u006bChar"` \
>
> -XX:CompileCommand=exclude,"UnicodeIdentifierTest.methodWithUnicode\u006bChar"
> \
>
> -XX:CompileCommand=exclude,'UnicodeIdentifierTest.methodWithUnicode\u006bChar'
> \
>
> -XX:CompileCommand=exclude,UnicodeIdentifierTest.methodWithUnicode\u006bChar \
>${TEST}
> ```
>
> But none of them worked.
>
> So if there is no other way to specify a non-ascii chars, it seems safe to
> remove the non-ascii code.
>
> If I miss something, please let me know. Thanks.
(The Chinese characters in this comment may not be displayed properly inside an
e-mail reader. Please see this comment on GitHub
https://github.com/openjdk/jdk/pull/5704)
-XX:CompileCommand does not process \u sequences. However, if your shell's
locale is UTF8, you can do something like this, by directly entering them on
the command-line, without escaping with \u:
public class CJK {
public static void main(String args[]) {
\u722a\u54c7();
}
static void \u722a\u54c7() { // Chinese word for "Java"
Thread.dumpStack();
}
}
===
$ locale
LANG=en_US.UTF-8
LANGUAGE=
LC_CTYPE="en_US.UTF-8"
LC_NUMERIC="en_US.UTF-8"
LC_TIME="en_US.UTF-8"
LC_COLLATE="en_US.UTF-8"
LC_MONETARY="en_US.UTF-8"
LC_MESSAGES="en_US.UTF-8"
LC_PAPER="en_US.UTF-8"
LC_NAME="en_US.UTF-8"
LC_ADDRESS="en_US.UTF-8"
LC_TELEPHONE="en_US.UTF-8"
LC_MEASUREMENT="en_US.UTF-8"
LC_IDENTIFICATION="en_US.UTF-8"
LC_ALL=
$ javac CJK.java
$ java -Xcomp -XX:-BackgroundCompilation -XX:CompileCommand='compileonly,*::爪哇'
-XX:+PrintCompilation -cp . CJK > log.txt
java.lang.Exception: Stack trace
at java.base/java.lang.Thread.dumpStack(Thread.java:1380)
at CJK.爪哇 (CJK.java:7)
at
Integrated: 8273092: Sort classlist in JDK image
On Fri, 27 Aug 2021 23:12:52 GMT, Ioi Lam wrote: > When the classlist is generated using build.tools.classlist.HelloClasslist, > its contents may be non-deterministic due to Java thread execution order. > > We should sort the generated classlist to make the JDK image's contents more > deterministic. > > Tested with Mach5 tier1, tier2, builds-tier5 This pull request has now been integrated. Changeset: 1996f649 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/1996f649a3a30b7ac4b547a762417f807f5fa414 Stats: 114 lines in 3 files changed: 102 ins; 6 del; 6 mod 8273092: Sort classlist in JDK image Reviewed-by: redestad, ihse, dfuchs - PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273092: Sort classlist in JDK image [v2]
On Sat, 28 Aug 2021 13:18:37 GMT, Claes Redestad wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> @dfuch comments > > Seems OK. Thanks @cl4es @magicus @dfuch for the review! - PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273092: Sort classlist in JDK image [v2]
On Mon, 30 Aug 2021 12:51:43 GMT, Daniel Fuchs wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @dfuch comments
>
> make/jdk/src/classes/build/tools/classlist/SortClasslist.java line 58:
>
>> 56: String line = scanner.nextLine();
>> 57: Matcher m = p.matcher(line);
>> 58: if (m.find()) {
>
> Are we sure that a comment line will not match this regexp, or that if it
> matches, it is not a comment line?
Thanks for the comments. I've swapper the matching order to check for leading
`#` and `@` characters first.
-
PR: https://git.openjdk.java.net/jdk/pull/5288
Re: RFR: 8273092: Sort classlist in JDK image [v2]
> When the classlist is generated using build.tools.classlist.HelloClasslist, > its contents may be non-deterministic due to Java thread execution order. > > We should sort the generated classlist to make the JDK image's contents more > deterministic. > > Tested with Mach5 tier1, tier2, builds-tier5 Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: @dfuch comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/5288/files - new: https://git.openjdk.java.net/jdk/pull/5288/files/dc170ec0..ee710895 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5288=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5288=00-01 Stats: 15 lines in 1 file changed: 7 ins; 7 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5288.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5288/head:pull/5288 PR: https://git.openjdk.java.net/jdk/pull/5288
RFR: 8273092: Sort classlist in JDK image
When the classlist is generated using build.tools.classlist.HelloClasslist, its contents may be non-deterministic due to Java thread execution order. We should sort the generated classlist to make the JDK image's contents more deterministic. Tested with Mach5 tier1, tier2, builds-tier5 - Commit messages: - 8273092: Sort classlist in JDK image Changes: https://git.openjdk.java.net/jdk/pull/5288/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5288=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8273092 Stats: 114 lines in 3 files changed: 102 ins; 6 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/5288.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5288/head:pull/5288 PR: https://git.openjdk.java.net/jdk/pull/5288
Integrated: 8272113: Build compare script fails with differences in classlist
On Sat, 7 Aug 2021 07:25:01 GMT, Ioi Lam wrote: > The CDS classlist is generated with the `-XX:DumpLoadedClassList` option, > which writes the name of the classes as they are being loaded. Since class > loading order is affected by thread switching, the classes may appear in a > non-deterministic order. > > Previously, the build compare script would sort the classlist before > comparing. Since https://bugs.openjdk.java.net/browse/JDK-8272113, each class > in the classlist has a new ID, so even after sorting, the contents would > differ: > > > $ diff classlist.1 classlist.2 > 207,208c207,208 > < jdk/internal/misc/VM id: 201 > < jdk/internal/util/SystemProps id: 202 > --- >> jdk/internal/misc/VM id: 202 >> jdk/internal/util/SystemProps id: 201 > > > The fix is to strip the id before doing the file comparison. > > Tested with: > > `mach5 remote-build -b > linux-aarch64-cmp-baseline,macosx-x64-cmp-baseline,linux-x64-cmp-baseline,linux-arm32-open-cmp-baseline,windows-x64-cmp-baseline` This pull request has now been integrated. Changeset: 272fcb42 Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/272fcb423a79b5b8bb4a80679b6b48feca66ebca Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod 8272113: Build compare script fails with differences in classlist Reviewed-by: tschatzl, hseigel - PR: https://git.openjdk.java.net/jdk/pull/5041
Re: RFR: 8272113: Build compare script fails with differences in classlist
On Mon, 9 Aug 2021 08:14:44 GMT, Thomas Schatzl wrote: >> The CDS classlist is generated with the `-XX:DumpLoadedClassList` option, >> which writes the name of the classes as they are being loaded. Since class >> loading order is affected by thread switching, the classes may appear in a >> non-deterministic order. >> >> Previously, the build compare script would sort the classlist before >> comparing. Since https://bugs.openjdk.java.net/browse/JDK-8272113, each >> class in the classlist has a new ID, so even after sorting, the contents >> would differ: >> >> >> $ diff classlist.1 classlist.2 >> 207,208c207,208 >> < jdk/internal/misc/VM id: 201 >> < jdk/internal/util/SystemProps id: 202 >> --- >>> jdk/internal/misc/VM id: 202 >>> jdk/internal/util/SystemProps id: 201 >> >> >> The fix is to strip the id before doing the file comparison. >> >> Tested with: >> >> `mach5 remote-build -b >> linux-aarch64-cmp-baseline,macosx-x64-cmp-baseline,linux-x64-cmp-baseline,linux-arm32-open-cmp-baseline,windows-x64-cmp-baseline` > > Lgtm. Thanks @tschatzl and @hseigel for the review. - PR: https://git.openjdk.java.net/jdk/pull/5041
RFR: 8272113: Build compare script fails with differences in classlist
The CDS classlist is generated with the `-XX:DumpLoadedClassList` option, which writes the name of the classes as they are being loaded. Since class loading order is affected by thread switching, the classes may appear in a non-deterministic order. Previously, the build compare script would sort the classlist before comparing. Since https://bugs.openjdk.java.net/browse/JDK-8272113, each class in the classlist has a new ID, so even after sorting, the contents would differ: $ diff classlist.1 classlist.2 207,208c207,208 < jdk/internal/misc/VM id: 201 < jdk/internal/util/SystemProps id: 202 --- > jdk/internal/misc/VM id: 202 > jdk/internal/util/SystemProps id: 201 The fix is to strip the id before doing the file comparison. Tested with: `mach5 remote-build -b linux-aarch64-cmp-baseline,macosx-x64-cmp-baseline,linux-x64-cmp-baseline,linux-arm32-open-cmp-baseline,windows-x64-cmp-baseline` - Commit messages: - 8272113: Build compare script fails with differences in classlist Changes: https://git.openjdk.java.net/jdk/pull/5041/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5041=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8272113 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5041.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5041/head:pull/5041 PR: https://git.openjdk.java.net/jdk/pull/5041
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 01:15:29 GMT, Yumin Qi wrote: >> Hi, Please review >> systemDictionaryShared becomes fatter and fatter so it is time to split it >> into functional files. Moved security and jar operation related code into >> CDSProtectionDomain, and moved shared class info (DumpTime/RunTime) to >> sharedClassInfo.[ch]pp, also moved lambda proxy related to >> lambdaProxyClassInfo.hpp. This way systemDictionaryShared.cpp looks neat and >> light. >> >> Tests: tier1,tier3,tier4 >> >> Thanks >> Yumin > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > Remove 'Shared' from class names and rename corresponding files This looks OK to me. The naming of the various classes is still not consistent since the code has evolved for a long time. It was probably a mistake to add `Shared` as a classname suffix, but that was done since the beginning of CDS. Maybe eventually we should rename all the classes with a CDS prefix (similar to the JFR code). That should be done in a separate PR, though. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8265403: consolidate definition of CPU features [v4]
On Mon, 19 Apr 2021 19:56:45 GMT, Doug Simon wrote: >> While porting >> [JDK-8224974](https://bugs.openjdk.java.net/browse/JDK-8224974) to Graal, I >> noticed that new CPU features were defined for x86 and AArch64 without being >> exposed via JVMCI. To avoid this problem in future, this PR updates x86 and >> AArch64 to define CPU features with a single macro that is used to generate >> enum declarations as well as vmstructs entries. >> >> In addition, the JVMCI API is updated to exposes the new CPU feature >> constants and now has a check that ensure these constants are in sync with >> the underlying macro definition. > > Doug Simon has updated the pull request incrementally with two additional > commits since the last revision: > > - updated date in copyright > - added blank lines after macros LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3558
Re: RFR: 8265696: Move CDS sources to src/hotspot/shared/cds [v6]
On Thu, 22 Apr 2021 06:10:17 GMT, Thomas Stuefe wrote: >> 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 11 additional commits >> since the last revision: >> >> - Merge branch 'master' into 8265696-move-cds-sources >> - fixed merge >> - Merge branch 'master' into 8265696-move-cds-sources >> - Merge branch 'master' into 8265696-move-cds-sources >> - Merge branch 'master' into 8265696-move-cds-sources >> - exclude all files under shared/cds if CDS is disabled; >> compactHashtable.cpp cannot be excluded since a bit of it is used even when >> CDS is disabled >> - fixed include guards -> #ifndef SHARE_CDS_x >> - fixed copyright >> - moved more files >> - fixed include lines >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/f8227451...8a9e7bdf > > Hi @iklam, > > this is a very welcome change! > > Nothing much to add to what David wrote (include guards need renaming). > > Apart from that, I was surprised that no gtests needed to be adapted, but > seems cds has no gtests? > > I tested building without cds, works fine. > > Thanks for doing this! > > If you fix the include guards, this is fine by me. > > ..Thomas Thanks @tstuefe @erikj79 @dholmes-ora for the review. The latest merged code passes Mach5 build tiers 1-5. I also manually tested the minimal VM build to verify the exclusion of CDS object files. - PR: https://git.openjdk.java.net/jdk/pull/3610
Integrated: 8265696: Move CDS sources to src/hotspot/shared/cds
On Wed, 21 Apr 2021 21:55:25 GMT, Ioi Lam wrote: > The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. This pull request has now been integrated. Changeset: 95f0fd6c Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/95f0fd6c Stats: 588 lines in 81 files changed: 254 ins; 282 del; 52 mod 8265696: Move CDS sources to src/hotspot/shared/cds Reviewed-by: erikj, dholmes, stuefe - PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696: Move CDS sources to src/hotspot/shared/cds [v6]
> The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. 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 11 additional commits since the last revision: - Merge branch 'master' into 8265696-move-cds-sources - fixed merge - Merge branch 'master' into 8265696-move-cds-sources - Merge branch 'master' into 8265696-move-cds-sources - Merge branch 'master' into 8265696-move-cds-sources - exclude all files under shared/cds if CDS is disabled; compactHashtable.cpp cannot be excluded since a bit of it is used even when CDS is disabled - fixed include guards -> #ifndef SHARE_CDS_x - fixed copyright - moved more files - fixed include lines - ... and 1 more: https://git.openjdk.java.net/jdk/compare/ba1998d8...8a9e7bdf - Changes: - all: https://git.openjdk.java.net/jdk/pull/3610/files - new: https://git.openjdk.java.net/jdk/pull/3610/files/5ca1c512..8a9e7bdf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3610=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3610=04-05 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696: Move CDS sources to src/hotspot/shared/cds [v5]
> The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. 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 10 additional commits since the last revision: - fixed merge - Merge branch 'master' into 8265696-move-cds-sources - Merge branch 'master' into 8265696-move-cds-sources - Merge branch 'master' into 8265696-move-cds-sources - exclude all files under shared/cds if CDS is disabled; compactHashtable.cpp cannot be excluded since a bit of it is used even when CDS is disabled - fixed include guards -> #ifndef SHARE_CDS_x - fixed copyright - moved more files - fixed include lines - 8265696: Move CDS sources from shared/memory to shared/cds - Changes: - all: https://git.openjdk.java.net/jdk/pull/3610/files - new: https://git.openjdk.java.net/jdk/pull/3610/files/43c75a72..5ca1c512 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3610=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3610=03-04 Stats: 47 lines in 6 files changed: 17 ins; 12 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696: Move CDS sources to src/hotspot/shared/cds [v4]
> The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. 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 eight additional commits since the last revision: - Merge branch 'master' into 8265696-move-cds-sources - Merge branch 'master' into 8265696-move-cds-sources - exclude all files under shared/cds if CDS is disabled; compactHashtable.cpp cannot be excluded since a bit of it is used even when CDS is disabled - fixed include guards -> #ifndef SHARE_CDS_x - fixed copyright - moved more files - fixed include lines - 8265696: Move CDS sources from shared/memory to shared/cds - Changes: - all: https://git.openjdk.java.net/jdk/pull/3610/files - new: https://git.openjdk.java.net/jdk/pull/3610/files/729c6519..43c75a72 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3610=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3610=02-03 Stats: 5509 lines in 251 files changed: 2774 ins; 1594 del; 1141 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources [v3]
> The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. 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 seven additional commits since the last revision: - Merge branch 'master' into 8265696-move-cds-sources - exclude all files under shared/cds if CDS is disabled; compactHashtable.cpp cannot be excluded since a bit of it is used even when CDS is disabled - fixed include guards -> #ifndef SHARE_CDS_x - fixed copyright - moved more files - fixed include lines - 8265696: Move CDS sources from shared/memory to shared/cds - Changes: - all: https://git.openjdk.java.net/jdk/pull/3610/files - new: https://git.openjdk.java.net/jdk/pull/3610/files/a1a2699f..729c6519 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3610=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3610=01-02 Stats: 338 lines in 63 files changed: 131 ins; 143 del; 64 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources [v2]
On Thu, 22 Apr 2021 04:16:57 GMT, David Holmes wrote: >> Ioi Lam has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - exclude all files under shared/cds if CDS is disabled; >> compactHashtable.cpp cannot be excluded since a bit of it is used even when >> CDS is disabled >> - fixed include guards -> #ifndef SHARE_CDS_x > > src/hotspot/share/cds/archiveUtils.inline.hpp line 25: > >> 23: */ >> 24: >> 25: #ifndef SHARE_MEMORY_ARCHIVEUTILS_INLINE_HPP > > The header file include guards all need updating for the new path. Fixed. > src/hotspot/share/cds/dynamicArchive.hpp line 38: > >> 36: #include "utilities/resourceHash.hpp" >> 37: >> 38: #if INCLUDE_CDS > > I have to wonder who is including this file and why, if CDS is not enabled. E.g., jvm.cpp includes dynamicArchive.hpp, but only uses its declarations inside INCLUDE_CDS blocks. It would be too verbose to do this in jvm.cpp #if INCLUDE_CDS #include "cds/dynamicArchive.hpp" #endif - PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources [v2]
On Thu, 22 Apr 2021 04:14:38 GMT, David Holmes wrote: >> Thank you! > > I don't suppose we can just exclude the new directory rather than listing > individual files? Fixed. Now all files under share/cds are excluded. I needed to move compactHashtable.cpp back to its old location since a little of it is used even when CDS is not used. - PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources [v2]
On Thu, 22 Apr 2021 06:10:17 GMT, Thomas Stuefe wrote: > Hi @iklam, > > this is a very welcome change! > > Nothing much to add to what David wrote (include guards need renaming). > > Apart from that, I was surprised that no gtests needed to be adapted, but > seems cds has no gtests? > > I tested building without cds, works fine. > > Thanks for doing this! > > If you fix the include guards, this is fine by me. > > ..Thomas Hi Thomas, thanks for the review. You're right that we don't have any gtests that should be fixed at some point. - PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources [v2]
> The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. Ioi Lam has updated the pull request incrementally with two additional commits since the last revision: - exclude all files under shared/cds if CDS is disabled; compactHashtable.cpp cannot be excluded since a bit of it is used even when CDS is disabled - fixed include guards -> #ifndef SHARE_CDS_x - Changes: - all: https://git.openjdk.java.net/jdk/pull/3610/files - new: https://git.openjdk.java.net/jdk/pull/3610/files/ba45831f..a1a2699f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3610=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3610=00-01 Stats: 66 lines in 19 files changed: 6 ins; 16 del; 44 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8265696 move cds sources
On Wed, 21 Apr 2021 21:55:25 GMT, Ioi Lam wrote: > The number of CDS source files have grown significantly. To improve > modularity, the following files should be moved a new directory, > src/hotspot/share/cds. > > - src/hotspot/share/classfile/classListParser.cpp > - src/hotspot/share/classfile/classListParser.hpp > - src/hotspot/share/classfile/classListWriter.hpp > - src/hotspot/share/classfile/compactHashtable.cpp > - src/hotspot/share/classfile/compactHashtable.hpp > - src/hotspot/share/classfile/lambdaFormInvokers.cpp > - src/hotspot/share/classfile/lambdaFormInvokers.hpp > - src/hotspot/share/memory/archiveBuilder.cpp > - src/hotspot/share/memory/archiveBuilder.hpp > - src/hotspot/share/memory/archiveUtils.cpp > - src/hotspot/share/memory/archiveUtils.hpp > - src/hotspot/share/memory/archiveUtils.inline.hpp > - src/hotspot/share/memory/cppVtables.cpp > - src/hotspot/share/memory/cppVtables.hpp > - src/hotspot/share/memory/dumpAllocStats.cpp > - src/hotspot/share/memory/dumpAllocStats.hpp > - src/hotspot/share/memory/dynamicArchive.cpp > - src/hotspot/share/memory/dynamicArchive.hpp > - src/hotspot/share/memory/filemap.cpp > - src/hotspot/share/memory/filemap.hpp > - src/hotspot/share/memory/heapShared.cpp > - src/hotspot/share/memory/heapShared.hpp > - src/hotspot/share/memory/heapShared.inline.hpp > - src/hotspot/share/memory/metaspaceShared.cpp > - src/hotspot/share/memory/metaspaceShared.hpp > - src/hotspot/share/prims/cdsoffsets.cpp > - src/hotspot/share/prims/cdsoffsets.hpp > > Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and > builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. make/hotspot/lib/JvmFeatures.gmk line 134: > 132: metaspaceShared_$(HOTSPOT_TARGET_CPU_ARCH).cpp \ > 133: sharedClassUtil.cpp \ > 134: sharedPathsMiscInfo.cpp \ Removed obsolete files that no longer exist. src/hotspot/os/windows/os_windows.cpp line 42: > 40: #include "logging/logStream.hpp" > 41: #include "memory/allocation.inline.hpp" > 42: #include "oops/oop.inline.hpp" Removed some unnecessary inclusions. - PR: https://git.openjdk.java.net/jdk/pull/3610
RFR: 8265696 move cds sources
The number of CDS source files have grown significantly. To improve modularity, the following files should be moved a new directory, src/hotspot/share/cds. - src/hotspot/share/classfile/classListParser.cpp - src/hotspot/share/classfile/classListParser.hpp - src/hotspot/share/classfile/classListWriter.hpp - src/hotspot/share/classfile/compactHashtable.cpp - src/hotspot/share/classfile/compactHashtable.hpp - src/hotspot/share/classfile/lambdaFormInvokers.cpp - src/hotspot/share/classfile/lambdaFormInvokers.hpp - src/hotspot/share/memory/archiveBuilder.cpp - src/hotspot/share/memory/archiveBuilder.hpp - src/hotspot/share/memory/archiveUtils.cpp - src/hotspot/share/memory/archiveUtils.hpp - src/hotspot/share/memory/archiveUtils.inline.hpp - src/hotspot/share/memory/cppVtables.cpp - src/hotspot/share/memory/cppVtables.hpp - src/hotspot/share/memory/dumpAllocStats.cpp - src/hotspot/share/memory/dumpAllocStats.hpp - src/hotspot/share/memory/dynamicArchive.cpp - src/hotspot/share/memory/dynamicArchive.hpp - src/hotspot/share/memory/filemap.cpp - src/hotspot/share/memory/filemap.hpp - src/hotspot/share/memory/heapShared.cpp - src/hotspot/share/memory/heapShared.hpp - src/hotspot/share/memory/heapShared.inline.hpp - src/hotspot/share/memory/metaspaceShared.cpp - src/hotspot/share/memory/metaspaceShared.hpp - src/hotspot/share/prims/cdsoffsets.cpp - src/hotspot/share/prims/cdsoffsets.hpp Testing with mach5: tier1, builds-tier2, builds-tier3, builds-tier4 and builds-tier5. Also locally: aarch64, arm, ppc64, s390, x86, and zero. - Commit messages: - fixed copyright - moved more files - fixed include lines - 8265696: Move CDS sources from shared/memory to shared/cds Changes: https://git.openjdk.java.net/jdk/pull/3610/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3610=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265696 Stats: 275 lines in 78 files changed: 116 ins; 134 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/3610.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3610/head:pull/3610 PR: https://git.openjdk.java.net/jdk/pull/3610
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Fri, 9 Apr 2021 16:54:51 GMT, Ioi Lam wrote: >> Vladimir Kozlov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove exports from Graal module to jdk.aot > > LGTM. Just a small nit. > @iklam > I thought the fingerprint code was also used by CDS. CDS actually uses a different mechanism (CRC of the classfile bytes) to validate that a class has not changed between archive dumping time and runtime. See https://github.com/openjdk/jdk/blob/5784f6b7f74d0b8081ac107fea172539d57d6020/src/hotspot/share/classfile/systemDictionaryShared.cpp#L1126-L1130 - PR: https://git.openjdk.java.net/jdk/pull/3398
Re: RFR: 8264805: Remove the experimental Ahead-of-Time Compiler [v4]
On Thu, 8 Apr 2021 17:24:38 GMT, Vladimir Kozlov wrote:
>> As part of [JEP 410](http://openjdk.java.net/jeps/410) remove code related
>> to Ahead-of-Time Compiler from JDK:
>>
>> - `jdk.aot` module — the `jaotc` tool
>> - `src/hotspot/share/aot` — loads AoT compiled code into VM for execution
>> - related HotSpot code guarded by `#if INCLUDE_AOT`
>>
>> Additionally, remove tests as well as code in makefiles related to AoT
>> compilation.
>>
>> Tested hs-tier1-4
>
> Vladimir Kozlov has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove exports from Graal module to jdk.aot
LGTM. Just a small nit.
src/hotspot/share/oops/methodCounters.cpp line 77:
> 75: }
> 76:
> 77: void MethodCounters::metaspace_pointers_do(MetaspaceClosure* it) {
MethodCounters::metaspace_pointers_do can be removed altogether (also need to
remove the declaration in methodCounter.hpp).
-
Marked as reviewed by iklam (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3398
Integrated: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
On Wed, 7 Apr 2021 22:37:28 GMT, Ioi Lam wrote: > Many HotSpot developers make only the `hotspot` target, and copy the > resulting `libjvm.so` into an already-build JDK image. > > HotSpot requires `interim-langtools` only when Graal is enabled. When Graal > is disabled, we can avoid building `interim-langtools`. This improves the > build time of `make hotspot` by about 20 seconds, or about 15%: > > Old: > $ make clean > $ time make hotspot > > real 1m57.905s > user 42m22.524s > sys 3m7.372s > > New: > $ make clean > $ time make hotspot > > real 1m39.916s > user 41m59.984s > sys 3m3.188s This pull request has now been integrated. Changeset: 951f277a Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/951f277a Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8264874: Build interim-langtools for HotSpot only if Graal is enabled Reviewed-by: kvn, erikj - PR: https://git.openjdk.java.net/jdk/pull/3389
Re: RFR: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
On Thu, 8 Apr 2021 01:13:45 GMT, Vladimir Kozlov wrote: >> Many HotSpot developers make only the `hotspot` target, and copy the >> resulting `libjvm.so` into an already-build JDK image. >> >> HotSpot requires `interim-langtools` only when Graal is enabled. When Graal >> is disabled, we can avoid building `interim-langtools`. This improves the >> build time of `make hotspot` by about 20 seconds, or about 15%: >> >> Old: >> $ make clean >> $ time make hotspot >> >> real 1m57.905s >> user 42m22.524s >> sys 3m7.372s >> >> New: >> $ make clean >> $ time make hotspot >> >> real 1m39.916s >> user 41m59.984s >> sys 3m3.188s > > Good. Thanks @vnkozlov and @erikj79 for the review! - PR: https://git.openjdk.java.net/jdk/pull/3389
RFR: 8264874: Build interim-langtools for HotSpot only if Graal is enabled
Many HotSpot developers make only the `hotspot` target, and copy the resulting `libjvm.so` into an already-build JDK image. HotSpot requires `interim-langtools` only when Graal is enabled. When Graal is disabled, we can avoid building `interim-langtools`. This improves the build time of `make hotspot` by about 20 seconds, or about 15%: Old: $ make clean $ time make hotspot real 1m57.905s user 42m22.524s sys 3m7.372s New: $ make clean $ time make hotspot real 1m39.916s user 41m59.984s sys 3m3.188s - Commit messages: - 8264874: Build interim-langtools for HotSpot only if Graal is enabled Changes: https://git.openjdk.java.net/jdk/pull/3389/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3389=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264874 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/3389.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3389/head:pull/3389 PR: https://git.openjdk.java.net/jdk/pull/3389
Re: RFR: 8263914: CDS fails to find the default shared archive on x86_32 [v2]
On Mon, 22 Mar 2021 03:37:05 GMT, Jie Fu wrote: >> Hi all, >> >> The VM fails to get initialized when running with `java -Xshare:on -version` >> on x86_32. >> The reason is that the default shared archive (classes_nocoops.jsa) doesn't >> exist. >> So the build system should generate classes_nocoops.jsa instead of >> classes.jsa on x86_32. >> >> Thanks. >> Best regards, >> Jie > > Jie Fu has updated the pull request incrementally with one additional commit > since the last revision: > > Fix the logic to look for the archive LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3109
Re: RFR: 8263465: JDK-8236847 causes tier1 build failure on linux-aarch64
On Thu, 11 Mar 2021 18:41:10 GMT, Yumin Qi wrote: > Hi, Please review > > JDK-8236847 changes failed on build linux-aarch64 on xcross build. The > reason is we check BUILD_CDS_ARCHIVE which is not correct in such case. We > should check ENABLE_CDS instead. > > Thanks > Yumin LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2946
Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v6]
On Wed, 3 Mar 2021 20:59:28 GMT, Ioi Lam wrote:
>> Yumin Qi has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains six commits:
>>
>> - Merge master
>> - Add --enable-compatible-cds-alignment for linux-aarch64 and macosx-x64 in
>> jib job
>> - Switch to enble compatible-cds-alignment at configuration
>> - Make CDS core region alignment configurable at build time
>> - Make 64K core region alignment only for specific platforms, also fixed
>> comments as suggestions.
>> - 8236847: CDS archive with 4K alignment unusable on machines with 64k pages
>
> LGTM
I think this part of
test/hotspot/jtreg/runtime/cds/appcds/SharedArchiveConsistency.java needs to be
changed, too:
public static long align_up_page(long l) throws Exception {
// wb is obtained in getFileOffsetInfo() which is called first in
main() else we should call
// WhiteBox.getWhiteBox() here first.
int pageSize = wb.getVMPageSize();<<<<<
return (l + pageSize -1) & (~ (pageSize - 1));
}
The `pageSize` should be `FileMapHeader::_core_region_alignment`.
There's a similar problem in the test dynamicArchive/ArchiveConsistency.java.
-
PR: https://git.openjdk.java.net/jdk/pull/2651
Re: RFR: 8236847: CDS archive with 4K alignment unusable on machines with 64k pages [v6]
On Mon, 1 Mar 2021 17:08:02 GMT, Yumin Qi wrote: >> Hi, Please review >> Usually most OSes are configured with page size of 4K, but some others are >> configured with 64K. If jdk binary is built on 4K platform and run on >> different configured platforms, CDS fails to be loaded due to region >> alignment mismatch: >> Unable to map CDS archive -- os::vm_allocation_granularity() expected: >> 4096 actual: 65536 >> This change uses 64K as region alignment if OS page size is less than 64K. >> For most of the current OSes, means always use 64K as file map region >> alignment. >>The archive size will increase about 300K due to the change. >>Tests: tier1-4 >> Run MacOS/X64 binary on MacOS/aarch64 >> >>Thanks >>Yumin > > Yumin Qi has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains six commits: > > - Merge master > - Add --enable-compatible-cds-alignment for linux-aarch64 and macosx-x64 in > jib job > - Switch to enble compatible-cds-alignment at configuration > - Make CDS core region alignment configurable at build time > - Make 64K core region alignment only for specific platforms, also fixed > comments as suggestions. > - 8236847: CDS archive with 4K alignment unusable on machines with 64k pages LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2651
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Tue, 2 Feb 2021 16:00:43 GMT, Gerard Ziemski wrote: >> I am not sure if jni_utils.c is the right file (it defines the `JNU_XXX` >> functions that are used by other shared libraries). >> >> There are other .c files that have trivial `DEF_JNI_OnLoad` functions (e.g., >> java.base/share/native/libnio/nio_util.c). >> >> @AlanBateman do you have any suggestions? > > I'm fine with the way it is, just thought we might want to consider cleaning > up a bit more, since it's a cleanup task itself. Thanks Gerard. The main purpose of this PR is to clean up the JVM side. I'll leave the refactoring of check_version.c to the core-lib team. - PR: https://git.openjdk.java.net/jdk/pull/2338
Integrated: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX
On Mon, 1 Feb 2021 18:40:54 GMT, Ioi Lam wrote: > - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which allowed > the same JDK library to use different version of HotSpot. However, HSX is no > longer supported so this API should be removed. > - Implementations of APIs such as JVM_DTraceActivate, were removed in > [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their > declarations should be removed from jvm.h This pull request has now been integrated. Changeset: b9d4211b Author:Ioi Lam URL: https://git.openjdk.java.net/jdk/commit/b9d4211b Stats: 112 lines in 4 files changed: 0 ins; 110 del; 2 mod 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX Reviewed-by: alanb, lfoltan, gziemski, ihse - PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Tue, 2 Feb 2021 15:59:47 GMT, Gerard Ziemski wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> fixed macos build > > Marked as reviewed by gziemski (Committer). Thanks @gerard-ziemski @magicus @AlanBateman @lfoltan for the review. - PR: https://git.openjdk.java.net/jdk/pull/2338
Re: [External] : Re: AArch64 OpenJDK bootstrap failure on head
On 2/2/21 1:32 AM, Thomas Stüfe wrote:
On Mon, Feb 1, 2021 at 10:11 PM Ioi Lam <mailto:ioi@oracle.com>> wrote:
On 2/1/21 9:36 AM, Thomas Stüfe wrote:
This does not solve the alignment problem, but I don't like
that we unconditionally print a message here since this is a
non-fatal error. Also, CDS mapping may fail for other reasons,
for which we do not print unconditionally. I think we should make
this info log level:
--- a/src/hotspot/share/memory/metaspaceShared.cpp
+++ b/src/hotspot/share/memory/metaspaceShared.cpp
@@ -1725,7 +1725,7 @@ MapArchiveResult
MetaspaceShared::map_archive(FileMapInfo* mapinfo, char* mapped
mapinfo->set_is_mapped(false);
if (mapinfo->alignment() !=
(size_t)os::vm_allocation_granularity()) {
- log_error(cds)("Unable to map CDS archive --
os::vm_allocation_granularity() expected: " SIZE_FORMAT
+ log_info(cds)("Unable to map CDS archive --
os::vm_allocation_granularity() expected: " SIZE_FORMAT
" actual: %d", mapinfo->alignment(),
os::vm_allocation_granularity());
return MAP_ARCHIVE_OTHER_FAILURE;
}
@ Ioi, does that make sense?
Yes, your fix makes sense.
Thanks. https://github.com/openjdk/jdk/pull/2348
<https://urldefense.com/v3/__https://github.com/openjdk/jdk/pull/2348__;!!GqivPVa7Brio!LRn8UjBQlnYs4XgafXHxU7RUqFZZx-Y19XBMcwy44BEjEXSIBS1GBrWKZ6pwcw$>
This issue is being address in
https://bugs.openjdk.java.net/browse/JDK-8236847
<https://bugs.openjdk.java.net/browse/JDK-8236847>. We will
probably unconditionally change the alignment to 64KB for AARCH64,
as well as MacOS (so that you can run a X64 JDK on M1 using Rosetta).
I thought so too. I also see it being used for region-to-region
alignment, where I believe page size would be sufficient?
The problem in JDK-8236847
<https://bugs.openjdk.java.net/browse/JDK-8236847> happens when you
create a CDS archive on one machine, and use it on another, and the two
machines have different page sizes.
(a) linux/aarch64 can be configured to have either 4KB or 64KB page sizes
(b) macOS/x64 uses 4KB, but macOS/aarch64 uses 64KB
So if you create the archive on a machine with 4KB page size, your RW
region may start at (64KB * N + 4KB), and this region cannot be mapped
directly on a machine with 64KB sizes.
My proposal is to always align the CDS regions by 64KB on these
platforms, so they can always be mapped under all circumstances.
Alternatives are: use read() instead of mmap; or, instead of mmaping the
individual regions, mmap all of them at once (assuming that the first
region, MC, is 64KB aligned). Either solution will reduce the
possibility of sharing, and make the code more complicated.
Since the CDS archive is at least 10MB in size, adding 3 extra padding
areas of up to 64KB each doesn't seem that outrageous in file size.
There's no change in physical memory usage since we never touch the
padding area.
Thanks
- Ioi
.:Thomas
Thanks
- Ioi
Cheers, Thomas
On Mon, Feb 1, 2021 at 6:20 PM Andrew Haley mailto:a...@redhat.com>> wrote:
On 2/1/21 5:14 PM, Aleksey Shipilev wrote:
> On 2/1/21 4:38 PM, Andrew Haley wrote:
>> but that doesn't work either. Any ideas? I'm really stuck.
>
> Did you "make clean" after changing any of the configure
files and/or configure arguments? I.e. did
> AWTIcon32_java_icon16_png actually regenerate?
Many times.
> Prepending the build with "LOG=debug" would tell what
cmdlines are used at every step of build process.
Sure, I can see what it's doing. I think this is actually a
regression
caused by the Windows-AArch64 port. I'll do some bisecting.
--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com
<https://urldefense.com/v3/__https://www.redhat.com__;!!GqivPVa7Brio!MzT8lD4heOPkWgVBap3cDC2aM4W8zJ1wWS_-PVlTdPwr96wHRafdO7zjS2x2qQ$>>
https://keybase.io/andrewhaley
<https://urldefense.com/v3/__https://keybase.io/andrewhaley__;!!GqivPVa7Brio!MzT8lD4heOPkWgVBap3cDC2aM4W8zJ1wWS_-PVlTdPwr96wHRafdO7wP-EZNow$>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Re: AArch64 OpenJDK bootstrap failure on head
On 2/1/21 9:36 AM, Thomas Stüfe wrote:
This does not solve the alignment problem, but I don't like that we
unconditionally print a message here since this is a non-fatal error.
Also, CDS mapping may fail for other reasons, for which we do not
print unconditionally. I think we should make this info log level:
--- a/src/hotspot/share/memory/metaspaceShared.cpp
+++ b/src/hotspot/share/memory/metaspaceShared.cpp
@@ -1725,7 +1725,7 @@ MapArchiveResult
MetaspaceShared::map_archive(FileMapInfo* mapinfo, char* mapped
mapinfo->set_is_mapped(false);
if (mapinfo->alignment() != (size_t)os::vm_allocation_granularity()) {
- log_error(cds)("Unable to map CDS archive --
os::vm_allocation_granularity() expected: " SIZE_FORMAT
+ log_info(cds)("Unable to map CDS archive --
os::vm_allocation_granularity() expected: " SIZE_FORMAT
" actual: %d", mapinfo->alignment(),
os::vm_allocation_granularity());
return MAP_ARCHIVE_OTHER_FAILURE;
}
@ Ioi, does that make sense?
Yes, your fix makes sense.
This issue is being address in
https://bugs.openjdk.java.net/browse/JDK-8236847. We will probably
unconditionally change the alignment to 64KB for AARCH64, as well as
MacOS (so that you can run a X64 JDK on M1 using Rosetta).
Thanks
- Ioi
Cheers, Thomas
On Mon, Feb 1, 2021 at 6:20 PM Andrew Haley > wrote:
On 2/1/21 5:14 PM, Aleksey Shipilev wrote:
> On 2/1/21 4:38 PM, Andrew Haley wrote:
>> but that doesn't work either. Any ideas? I'm really stuck.
>
> Did you "make clean" after changing any of the configure files
and/or configure arguments? I.e. did
> AWTIcon32_java_icon16_png actually regenerate?
Many times.
> Prepending the build with "LOG=debug" would tell what cmdlines
are used at every step of build process.
Sure, I can see what it's doing. I think this is actually a regression
caused by the Windows-AArch64 port. I'll do some bisecting.
--
Andrew Haley (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. >
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
On Mon, 1 Feb 2021 20:29:10 GMT, Gerard Ziemski wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> fixed macos build
>
> src/java.base/share/native/libjava/check_version.c line 33:
>
>> 31: DEF_JNI_OnLoad(JavaVM *vm, void *reserved)
>> 32: {
>> 33: return JNI_VERSION_1_2;
>
> This leaves an entire file with one trivial function implementation. Can we
> remove the file and implement `DEF_JNI_OnLoad()` in `jni_util.h` (or some
> other existing suitable file) ?
I am not sure if jni_utils.c is the right file (it defines the `JNU_XXX`
functions that are used by other shared libraries).
There are other .c files that have trivial `DEF_JNI_OnLoad` functions (e.g.,
java.base/share/native/libnio/nio_util.c).
@AlanBateman do you have any suggestions?
-
PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8260193: Remove JVM_GetInterfaceVersion() and JVM_DTraceXXX [v2]
> - JVM_GetInterfaceVersion() was used by "HotSpot Express" (HSX) which allowed > the same JDK library to use different version of HotSpot. However, HSX is no > longer supported so this API should be removed. > - Implementations of APIs such as JVM_DTraceActivate, were removed in > [JDK-8068976](https://bugs.openjdk.java.net/browse/JDK-8068976), so their > declarations should be removed from jvm.h Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: fixed macos build - Changes: - all: https://git.openjdk.java.net/jdk/pull/2338/files - new: https://git.openjdk.java.net/jdk/pull/2338/files/c0307e7d..3a6415eb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2338=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2338=00-01 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2338.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2338/head:pull/2338 PR: https://git.openjdk.java.net/jdk/pull/2338
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074 [v2]
On Tue, 5 Jan 2021 12:06:18 GMT, Hao Sun wrote: >> From the error log we can see the root cause is that, develop_pd flag >> 'pd_CICompileOSR' is undeclared in zero build. >> >> Where this flag is used? >> Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 >> of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. >> >> Where this flag can be declared? >> Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if >> VM is built with compiler1 or compiler2. See lines 30 to 38 of >> 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get >> declared in the header files for specific arch, e.g., >> 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. >> However, regarding zero build (without compiler1 and compiler2 and >> jvmci) , this flag is undelcared. Hence, this patch gets header file >> 'compiler/compiler_globals_pd.hpp' included where this flag is declared >> for the case when neither COMPILER1 nor COMPILER2 are defined and >> INCLUDE_JVMCI is inactive. >> >> Note that 'compiler/compiler_globals_pd.hpp' already includes >> 'runtime/globals_shared.hpp'. >> >> Note that zero build with PCH succeeds because 'runtime/globals.hpp' is >> included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is >> included in 'runtime/globals.hpp'. > > Hao Sun has updated the pull request incrementally with one additional commit > since the last revision: > > Header 'runtime/globals_shared.hpp' should be kept > > Header 'runtime/globals_shared.hpp' is kept even though > 'compiler/compiler_globals_pd.hpp' already includes it, because > 'compiler_globals.hpp' uses the DECLARE_FLAGS macro, which is defined by > 'runtime/globals_shared.hpp', and it should be included directly. > > Besides, update the copyright year to 2021. > > Change-Id: Ia355f3b6e98b495dc265093e71b2d1fec1ca45ca > CustomizedGitHooks: yes LGTM - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
On Tue, 5 Jan 2021 03:07:49 GMT, Chris Plummer wrote: > Given that this seems to be a common problem in our code, and likely a very > very old problem at that, why has it never been reported before? I'm not > questioning the fix except to the extent that I'm questioning our > understanding of the problem. I think it's probably because the errors only happen in very rare cases. For example, when a DLL is missing from the JAVA_HOME. For more common cases, such as Process_impl.c, the code has been rewritten to use FormatMessageW, which is unicode. - PR: https://git.openjdk.java.net/jdk/pull/1928
Re: RFR: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
On Tue, 5 Jan 2021 01:31:28 GMT, Chris Plummer wrote: > > jdk.hotspot.agent do not have `FormatMessage()` call in other place. > > Did you say about whole JDK code? I haven't checked all of them, but some > > code (e.g. net_util_md.c) uses `JNU_ThrowByName()` which is provided by > > java.dll. > > Yes, I was referring to all JDK code. Given how many references there are to > FormatMessage(), I'm surprised this issue has not turned up before. I was > wondering if there is something unique about the SA reference that makes the > issue only turn up there. I looked at a cases in the JDK code where `Java_sun_security_pkcs11_wrapper_PKCS11_connect()` calls `FormatMessage()`. It eventually passes the `char*` to `jni_ThrowNew`, which eventually gets to `Exceptions::new_exception` with `to_utf8_safe==safe_to_utf8`. This means the message will be converted with `java_lang_String::create_from_str()`, which does NOT call `JNU_NewStringPlatform`. So I think in this case, the error message will also be garbled. java.base/windows/native/libnet/Inet4AddressImpl.c seems to have a similar problem in the `ping4` function. But, I don't know how to write a simple test case for the above. - PR: https://git.openjdk.java.net/jdk/pull/1928
Re: RFR: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
On Mon, 4 Jan 2021 09:25:55 GMT, Yasumasa Suenaga wrote: > I got garbled exception message as following when I run `livenmethods` CLHSDB > command: > > sun.jvm.hotspot.debugger.DebuggerException : ?w???W > > My Windows laptop is set Japanese Locale, garbled message was written in > Japanese. > saproc.dll would throw exception via > [ThrowNew()](https://docs.oracle.com/en/java/javase/15/docs/specs/jni/functions.html#thrownew) > JNI function, but it accepts UTF-8 encoded message. However > [FormatMessage()](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-formatmessage) > Windows API might not return UTF-8 encoded string on Japanese locale. > > java.dll (libjava,so) provides good functions to resolve this issue. We can > convert localized (non ascii) chars to UTF-8 string. I use them in this PR > and remove `FormatMessage()` call from sadis.c. > And also I remove `-D_MBCS` from compiler option because [MBCS has been > already > deprecated](https://docs.microsoft.com/ja-jp/cpp/text/support-for-multibyte-character-sets-mbcss) > - it does not seem to add to any other executables. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1928
Re: RFR: 8259045: Exception message from saproc.dll is garbled on Windows with Japanese locale
On Tue, 5 Jan 2021 00:41:11 GMT, Yasumasa Suenaga wrote: >> There are probably 25 or so places in our code where we use FormatMessage to >> get the error message. Are these all going to run into the same >> FormateMessage bug? >> >> Also, it's not clear to me why you are getting garbled text in the first >> place. You said "Windows API might not return UTF-8 encoded string on >> Japanese locale." Why is that the case? > >> There are probably 25 or so places in our code where we use FormatMessage to >> get the error message. Are these all going to run into the same >> FormateMessage bug? > > jdk.hotspot.agent do not have `FormatMessage()` call in other place. > Did you say about whole JDK code? I haven't checked all of them, but some > code (e.g. net_util_md.c) uses `JNU_ThrowByName()` which is provided by > java.dll. > >> Also, it's not clear to me why you are getting garbled text in the first >> place. You said "Windows API might not return UTF-8 encoded string on >> Japanese locale." Why is that the case? > > Japanese locale on Windows uses CP932., so `FormatMessage()` would return > error message with Japanese chars which are encoded by CP932. It is not UTF-8. Now the saproc DLL has an external reference to getLastErrorString, JNU_NewStringPlatform and JNU_NewObjectByName on all platforms. Do we need to modify the makefiles for platforms other than Windows? - PR: https://git.openjdk.java.net/jdk/pull/1928
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 23:39:18 GMT, Jie Fu wrote: >> src/hotspot/share/compiler/compiler_globals.hpp line 29: >> >>> 27: >>> 28: #include "compiler/compiler_globals_pd.hpp" >>> 29: #ifdef COMPILER1 >> >> `#include "runtime/globals_shared.hpp"` should not be removed. >> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >> globals_shared.hpp. > >> `#include "runtime/globals_shared.hpp"` should not be removed. >> compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by >> globals_shared.hpp. > > Since globals_shared.hpp is included in compiler_globals_pd.hpp, I think it's > fine to remove it. We should not rely on indirect inclusion. Otherwise, compiler_globals.hpp will break if compiler_globals_pd.hpp is changed to not include globals_shared.hpp. In fact, I have been fixing a lot of such problems when restructuring the header files. See https://github.com/openjdk/jdk/pull/1610 -- the vast majority of the 59 files changed in that PR were caused by relying on indirect inclusion of stubRoutines.hpp. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Mon, 4 Jan 2021 10:09:23 GMT, Aleksey Shipilev wrote: > It is unclear to me why the original change in JDK-8258074 included > `compiler_globals_pd.hpp` in `globals.hpp` at all, @iklam? The reason is, for historical reasons, some PD flags related to the compiler, such as `BackgroundCompilation`, are declared in `globals.hpp`. As a result, `globals.hpp` must include `compiler_globals_pd.hpp`, which provides the platform-specific default value for `BackgroundCompilation`. This should eventually be fixed by moving the declaration of these flags to compiler_globals.hpp instead. > I believe we should additionally change the `#include > compiler/compiler_globals_pd.hpp` to `#include compiler/compiler_globals.hpp` > in `globals.hpp`? This is not necessary. `globals.hpp` does not use anything declared in `compiler_globals.hpp` - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: RFR: 8258857: Zero: non-PCH release build fails after JDK-8258074
On Fri, 25 Dec 2020 10:11:13 GMT, Hao Sun wrote: > From the error log we can see the root cause is that, develop_pd flag > 'pd_CICompileOSR' is undeclared in zero build. > > Where this flag is used? > Flag 'pd_CICompileOSR' is assigned to flag 'CICompileOSR'. See line 77 > of 'compiler_globals.hpp' and further line 86 of 'globals_shared.hpp'. > > Where this flag can be declared? > Header files 'c1_globals.hpp' or 'c2_globals.hpp' would be included if > VM is built with compiler1 or compiler2. See lines 30 to 38 of > 'complier_globals.hpp'. And further, flag 'pd_CICompileOSR' may get > declared in the header files for specific arch, e.g., > 'c1_globals_aarch64.hpp', 'c2_globals_aarch64.hpp'. > However, regarding zero build (without compiler1 and compiler2 and > jvmci) , this flag is undelcared. Hence, this patch gets header file > 'compiler/compiler_globals_pd.hpp' included where this flag is declared > for the case when neither COMPILER1 nor COMPILER2 are defined and > INCLUDE_JVMCI is inactive. > > Note that 'compiler/compiler_globals_pd.hpp' already includes > 'runtime/globals_shared.hpp'. > > Note that zero build with PCH succeeds because 'runtime/globals.hpp' is > included in 'precompiled.hpp', and further 'compiler_globals_pd.hpp' is > included in 'runtime/globals.hpp'. Changes requested by iklam (Reviewer). src/hotspot/share/compiler/compiler_globals.hpp line 29: > 27: > 28: #include "compiler/compiler_globals_pd.hpp" > 29: #ifdef COMPILER1 `#include "runtime/globals_shared.hpp"` should not be removed. compiler_globals.hpp uses the `DECLARE_FLAGS` macro, which is defined by globals_shared.hpp. - PR: https://git.openjdk.java.net/jdk/pull/1894
Re: Why do we export "JVM_handle_xxx_signal"?
I have no idea, but this symbol has been exported since we moved the HotSpot source code from SCCS to Mercurial in 2008. It's probably vestige from the early days of Java. http://hg.openjdk.java.net/jdk7/modules/hotspot/annotate/9646293b9637/make/linux/makefiles/mapfile-vers-product#l244 I checked all .so files in our JDK build and no one uses JVM_handle_linux_signal. I think it's probably safe to hide it. We should probably drop the JVM_ prefix, since this function is not declared in jvm.h. Thanks - Ioi On 10/29/20 9:02 AM, Thomas Stüfe wrote: Hi, Does anyone know why we explicitly export JVM_handle_bsd_signal and JVM_handle_linux_signal (the latter also accidentally from symbols-aix)? These functions are not even the real signal handler, just an internal function; the signal handler is "javaSignalHandler", but that one is not exported... Thanks, Thomas
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v16]
On Sat, 10 Oct 2020 00:08:26 GMT, Yumin Qi wrote: >> This patch is reorganized after 8252725, which is separated from this patch >> to refactor jlink glugin code. The previous >> webrev with hg can be found at: >> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 >> integrated, the >> regeneration of holder classes is simply to call the new added >> GenerateJLIClassesHelper.cdsGenerateHolderClasses >> function. Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with three additional > commits since the last revision: > > - Correct typo for check message > - Removed try/catch from java side and moved output to vm. > - Make change to original indent Latest version LGTM. - Marked as reviewed by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 21:49:24 GMT, Yumin Qi wrote:
>> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
>>
>>> 142: String line = s.trim();
>>> 143: if (!line.startsWith("[LF_RESOLVE]") &&
>>> !line.startsWith("[SPECIES_RESOLVE]")) {
>>> 144: System.out.println("Wrong prefix: " + line);
>>
>> Should this throw an exception instead?
>
> This part is for check the format only, throw exceptions will lead more
> objects generated which should not be archived
> in shared heap. Since this is only called from VM, so decide not to throw
> exception here.
The exception object will not be automatically added to the shared heap, so
it's OK to throw exceptions here.
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains
> 23 commits:
> - Added new separate function to CDS for logging species and modified the
> existing function to log lambda form invokers.
>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
> read only in CDS.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Removed unused imports.
> - Fixed comments with correct class and method name in CDS, removed unused
> variables after last change.
> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
> CDS as generateLambdaFormHolderClasses. Added
>input verification function in CDS before class generation. Added more
> test scenarios. Removed trailing unused ending
>words for output of lambda form trace line in case of DumpLoadedClassList.
> - Move the check work to java, restore code in VM. Modified test code
> according to the changes. The invoke name
>verififcation is not implemented since not all the holder class are
> processed, not all the functions of processed
>holder classes are added. For holder class with DirectMethodHandle in its
> name, only the name in the
>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
> skipped silently. This makes the verification
>on invoke names difficul, a name not in the keyset should not fail the
> test. Also add a boolean to
>cdsGenerateHolderClasses to indicate call path.
> - Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
>white spaces from both front and end of the line or it will fail method
> type validation.
> - In case of exception happens during reloading class, CHECK will return
> without free the allocated buffer for class
>bytes so moved the buffer allocation and freeing to caller. Also removed
> test 6 since there is not guarantee that we
>can give a signature which will always fail. Additional changes to
> GenerateJLIClassesHelper according to review
>suggestion.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
> - ... and 13 more:
> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
Marked as reviewed by iklam (Reviewer).
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 133:
> 131: log_info(cds)("Class %s not present, skip", name);
> 132: return;
> 133: }
`assert(klass->is_instance_klass(), "Should be");` should be after the NULL
check of `klass`
src/hotspot/share/prims/jvm.cpp line 3872:
> 3870: JVM_ENTRY(jboolean, JVM_IsDumpingClassList(JNIEnv *env))
> 3871: JVMWrapper("JVM_IsDumpingClassList");
> 3872: return DumpLoadedClassList != NULL && classlist_file->is_open();
For sanity, it's better to add `classlist_file != NULL`
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v7]
On Wed, 30 Sep 2020 04:59:20 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
> white spaces from both front and end of the line or it will fail method
> type validation.
Changes requested by iklam (Reviewer).
src/java.base/share/classes/jdk/internal/misc/CDS.java line 30:
> 28: public class CDS {
> 29: // cache the result
> 30: static private boolean isDumpLoadedClassList;
`isDumpLoadedClassList` is not gramatically correct. Also the field should be
final. How about:
static final private boolean isDumpingClassList = isDumpingClassList0();
public static boolean isDumpingClassList() {
return isDumpingClassList;
}
private static boolean isDumpingClassList0();
src/java.base/share/classes/jdk/internal/misc/CDS.java line 82:
> 80: * log output to DumpLoadedClassList
> 81: */
> 82: public static void logTraceResolve(String line) {
`logTraceResolve` is too generic. How about `CDS.logLambdaFormInvoker()` to
match the `@lambda-form-invoker` in the
classlist file?
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v7]
On Wed, 30 Sep 2020 04:59:20 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
> white spaces from both front and end of the line or it will fail method
> type validation.
Changes requested by iklam (Reviewer).
test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 70:
> 68: "Hello",
> 69: "@lambda-form-invoker [LF_RESOLVE]
> java.lang.invoke.DirectMethodHandle$Holder invokeNothing L7_L
> (anyword)", 70: "@lambda-form-invoker [LF_RESOLVE]
> java.lang.invoke.DirectMethodHandle$Holder
> invokeNothing LL_I anyword"),
We shouldn't allow the classlist to contain arbitrary data. These two cases
should generate an error.
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 52:
> 50:
> 51: // trim white spaces from front and end of string.
> 52: char* trim(char* s) {
I think this creates unnecessary dependency between the C code and the Java
code. The C code assumes that the Java code
has appended something like "(salvaged)" into the output, and tries to get rid
of that in a non-obvious way. It's
better to modify the Java code from
static void traceSpeciesType(String cn, Class salvage) {
if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) {
String traceSP = SPECIES_RESOLVE + " " + cn + (salvage != null ? "
(salvaged)" : " (generated)");
if (TRACE_RESOLVE) {
System.out.println(traceSP);
}
CDS.logTraceResolve(traceSP);
}
}
to
if (TRACE_RESOLVE || CDS.isDumpLoadedClassList()) {
String traceSP = SPECIES_RESOLVE + " " + cn;
if (TRACE_RESOLVE) {
System.out.println(traceSP + (salvage != null ? " (salvaged)" : "
(generated)"));
}
CDS.logTraceResolve(traceSP);
}
test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 85:
> 83: appJar, classlist(
> 84: "Hello",
> 85: "@lambda-form-invoker [LF_XYRESOLVE]
> java.lang.invoke.DirectMethodHandle$Holder invokeStatic L7_L
> (any)",
We should not allow incorrect input. This should generate an error.
test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 78:
> 76: "Hello",
> 77: "@lambda-form-invoker [LF_RESOLVE]
> my.nonexist.package.MyNonExistClassName$holder invokeStatic
> L7_L", 78: "@lambda-form-invoker [LF_RESOLVE]
> my.nonexist.package.MyNonExistClassName$holder
> invokeStatic LL_I"),
I think it's dangerous to allow arbitrary class names here.
InvokerBytecodeGenerator doesn't check the classname. This
will make it possible to overwrite the contents of arbitrary classes. We should
have a check here and allow only the
specific holder classes that are supported.
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v3]
On Fri, 25 Sep 2020 21:19:39 GMT, Yumin Qi wrote: >> This patch is reorganized after 8252725, which is separated from this patch >> to refactor jlink glugin code. The previous >> webrev with hg can be found at: >> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 >> integrated, the >> regeneration of holder classes is simply to call the new added >> GenerateJLIClassesHelper.cdsGenerateHolderClasses >> function. Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > 8247536: Support for pre-generated java.lang.invoke classes in CDS static > archive Changes requested by iklam (Reviewer). test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 32: > 30: * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds > 31: * @compile ClassListFormatBase.java test-classes/Hello.java > 32: * @run driver DumpClassListWithLF The `@compile` line can be removed to speed up the test execution. You can add this: @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds/test-classes and also change the code below appJar, classlist( -"Hello", +Hello.class.getName(), (There's no need for `/test/hotspot/jtreg/runtime/cds/appcds` in `@library` because that's the current directory of this test). test/hotspot/jtreg/runtime/cds/appcds/DumpClassListWithLF.java line 53: > 51: "Hello", > 52: "@lambda-form-invoker [LF_RESOLVE] > java.lang.invoke.DirectMethodHandle$Holder invokeStatic L7_L > (success)", 53: "@lambda-form-invoker [LF_RESOLVE] > java.lang.invoke.DirectMethodHandle$Holder > invokeStatic LL_I (success)"), I think the `(success)` part should not be included in the classlist. The classlist is the public interface. It should be concise and include only the necessary information. @lambda-form-invoker [LF_RESOLVE] java.lang.invoke.DirectMethodHandle$Holder invokeStatic L7_L @lambda-form-invoker [LF_RESOLVE] java.lang.invoke.DirectMethodHandle$Holder invokeStatic LL_I test/hotspot/jtreg/runtime/cds/appcds/customLoader/ProhibitedPackageNamesTest.java line 31: > 29: * @requires vm.cds.custom.loaders > 30: * @library /test/lib /test/hotspot/jtreg/runtime/cds/appcds > 31: * @compile ../ClassListFormatBase.java ../test-classes/Hello.java > test-classes/InProhibitedPkg.java Similar comment for avoiding `@compile`. `@compile` has been mis-used by older CDS tests. We should avoid using it, and should remove it when we are touching the old tests. src/hotspot/share/classfile/systemDictionary.cpp line 1864: > 1862: > 1863: // Used for assertions and verification, also used from > LambdaFormInvokers::reload_class > 1864: // to get original class which has already been loaded. Is the above comment change still needed? - PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8253500: [REDO] JDK-8253208 Move CDS related code to a separate class
On Wed, 23 Sep 2020 23:05:59 GMT, Yumin Qi wrote: > This patch is a REDO for JDK-8253208 which was backed out since it caused > runtime/cds/DeterministicDump.java failed, > see JDK-8253495. Since the failure is another issue and only triggered by > this patch, the test case now is put on > ProblemList.txt. The real root cause for the failure is detailed in > JDK-8253495. When JDK-8253208 was backed out, > CDS.java remained without removed from that patch (see JDK-8253495 subtask), > so this patch does not include CDS.java > (this is why you will see CDS.c without CDS.java in this patch). Tests > tier1-4 passed. Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/327
Re: RFR: 8253208: Move CDS related code to a separate class [v2]
On Mon, 21 Sep 2020 18:17:55 GMT, Yumin Qi wrote: >> With more CDS related code added to VM, it is time to move CDS code to a >> separate class. CDS is the new class which is >> specific to CDS. >> Tests: tier1-4 > > Yumin Qi has updated the pull request incrementally with one additional > commit since the last revision: > > 8253208: Move CDS related code to a separate class Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/261
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive
On Tue, 15 Sep 2020 18:57:55 GMT, Yumin Qi wrote:
> This patch is reorganized after 8252725, which is separated from this patch
> to refactor jlink glugin code. The previous
> webrev with hg can be found at:
> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
> integrated, the
> regeneration of holder classes is simply to call the new added
> GenerateJLIClassesHelper.cdsGenerateHolderClasses
> function. Tests: tier1-4
Changes requested by iklam (Reviewer).
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 51:
> 49: #include "runtime/os.hpp"
> 50: #include "runtime/signature.hpp"
> 51:
Are all these header files needed? E.g., typeArrayKlass.hpp doesn't seem to be
needed.
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 121:
> 119: log_info(cds)("Class %s not present, skip", name);
> 120: return;
> 121: }
Since the classlist can be generated by the user, it may cause the assert at
line 115 to fail. For example, no
java.lang.invoke.*$Holder classes are used by HelloWorld: $ java -verbose
-Xshare:off -cp . HelloWorld | grep Holder
[0.030s][info][class,load] java.util.KeyValueHolder source: jrt:/java.base
[0.080s][info][class,load] java.security.SecureClassLoader$DebugHolder source:
jrt:/java.base
$
But it's possible for the user to generate a classlist using HelloWorld, and
then manually add LF_RESOLVE lines into
the classlist.
So I think line 114 should be changed to a regular lookup (the symbol is
created if it doesn't exist), and line 115
should be removed.
Also, we should add some test cases for invalid LF_RESOLVE input. You can see
examples in
[appcds/customLoader/ClassListFormatA.java](https://github.com/openjdk/jdk/blob/d250f9e08c4a0c047cd3e619918c116f568e31d4/test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassListFormatA.java#L51).
Since the new tests aren't related to custom loader, we should probably move
[appcds/customLoader/ClassListFormatBase.java](https://github.com/openjdk/jdk/blob/d250f9e08c4a0c047cd3e619918c116f568e31d4/test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassListFormatBase.java#L30)
under appcds/, and add a new file like appcds/ClassListFormat.java
src/hotspot/share/classfile/lambdaFormInvokers.cpp line 158:
> 156: // find_class assert on SystemDictionary_lock or safepoint
> 157: MutexLocker lock(SystemDictionary_lock);
> 158: InstanceKlass* old = SystemDictionary::find_class(class_name, cld);
There's no need to call `find_class` here, since it will return the same class
as `klass` on line 117.
src/hotspot/share/classfile/lambdaFormInvokers.hpp line 27:
> 25: #ifndef SHARE_MEMORY_LAMBDAFORMINVOKERS_HPP
> 26: #define SHARE_MEMORY_LAMBDAFORMINVOKERS_HPP
> 27: #include "memory/allocation.hpp"
For the AllStatic base class, you should use memory/allStatic.hpp instead.
src/hotspot/share/classfile/systemDictionary.cpp line 1875:
> 1873: VerifyDuringStartup ||
> 1874: VerifyAfterGC ||
> 1875: DumpSharedSpaces, "too expensive");
This may not be needed if you remove the find_class() call from
LambdaFormInvokers::regenerate_holder_classes?
src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java line
67:
> 65: if (VM.isDumpLoadedClassListSetAndOpen) {
> 66: VM.cdsTraceResolve(traceLF);
> 67: }
GenerateJLIClassesHelper shouldn't need to know why the trace is needed. Also,
"cdsTraceResolve" is too generic.
I think it's better to have
if (TRACE_RESOLVE || VM.CDS_TRACE_JLINV_RESOLVE) {
...
VM.cdsTraceJLINVResolve(traceLF);
The acronym JLINV is used in
[methodHandles.cpp](https://github.com/openjdk/jdk/blob/ce93cbce77e1f4baa52676826c8ae27d474360b6/src/hotspot/share/prims/methodHandles.cpp#L1524)
src/java.base/share/classes/jdk/internal/misc/VM.java line 490:
> 488: */
> 489: public static boolean isDumpLoadedClassListSetAndOpen;
> 490: private static native boolean isDumpLoadedClassListSetAndOpen0();
I would suggest to rename to `isDumpingLoadedClassList`
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8244778: Archive full module graph in CDS
On Tue, 8 Sep 2020 15:59:33 GMT, Ioi Lam wrote:
> This is the same patch as
> [8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/)
> published in
> [hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html).
> The rest of the review will continue on GitHub. I will add new commits to
> respond to comments to the above e-mail.
In response to Lois Foltain's comments on
[hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041616.html):
> Minor nit in moduleEntry.cpp & packageEntry.cpp when dealing with the
> ModuleEntry's reads list and a PackageEntry's
> exports list. The names of the methods to write and read those arrays is
> somewhat confusing.
>
> ModuleEntry::write_archived_entry_array
> ModuleEntry::read_archived_entry_array
>
> At first I thought you were reading/writing an array of archived entries, not
> the array within an archived entry
> itself. I was trying to think of a better name. Please consider adding a
> comment at line #400 & line #417 ahead of
> those methods in moduleEntry.cpp to indicate that they are used for both
> reading/writing a ModuleEntry's reads list and
> a PackageEntry's exports list.
I renamed the functions to `ModuleEntry's::write_growable_array` and
`ModuleEntry::restore_growable_array`, and added
comments as you suggested. See commit
[4f90e77](https://github.com/openjdk/jdk/pull/80/commits/4f90e77207de5fc7cf09a12fb89b75087be57225)
// This function is used to archive ModuleEntry::_reads and
PackageEntry::_qualified_exports.
// GrowableArray cannot be directly archived, as it needs to be expandable at
runtime.
// Write it out as an Array, and convert it back to GrowableArray at runtime.
Array*
ModuleEntry::write_growable_array(GrowableArray* array) {
> A question about this because a user's program can define modules post module
> initialization via
> ModuleDescriptor.newModule(). See for example, tests within
> open/test/hotspot/jtreg/runtime/module/AccessCheck. So
> all of these tests would trigger check_cds_restrictions() if -Xshare:dump was
> turned on. Is that a concern?
Arbitrary user code cannot be executed during -Xshare:dump. The only way to do
it is to use a JVMTI agent, which
requires specifying `-XX:+AllowArchivingWithJavaAgent`. You can see an example
in the
[GCDuringDump.java](https://github.com/openjdk/jdk/blob/704f784c88ee282837c980948167e741e7227f27/test/hotspot/jtreg/runtime/cds/appcds/javaldr/GCDuringDump.java#L65)
test. If the agent tries to define an extra module, it will get an
`UnsupportedOperationException` thrown by
`check_cds_restrictions()`.
-
PR: https://git.openjdk.java.net/jdk/pull/80
RFR: 8244778: Archive full module graph in CDS
This is the same patch as [8244778-archive-full-module-graph.v03](http://cr.openjdk.java.net/~iklam/jdk16/8244778-archive-full-module-graph.v03/) published in [hotspot-runtime-...@openjdk.java.net](https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041496.html). The rest of the review will continue on GitHub. I will add new commits to respond to comments to the above e-mail. - Commit messages: - fixed trailing spaces - Renamed ModuleEntry::write_growable_array - Update to latest repo (JDK-8251557); added comments - 8244778: Archive full module graph in CDS Changes: https://git.openjdk.java.net/jdk/pull/80/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=80=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8244778 Stats: 2039 lines in 59 files changed: 1887 ins; 26 del; 126 mod Patch: https://git.openjdk.java.net/jdk/pull/80.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/80/head:pull/80 PR: https://git.openjdk.java.net/jdk/pull/80
Re: RFR: JDK-8200738 Use --hash-style=gnu for gcc
On 6/15/20 2:08 AM, Magnus Ihse Bursie wrote: On 2020-06-15 07:46, David Holmes wrote: Hi Magnus, On 13/06/2020 1:42 am, Magnus Ihse Bursie wrote: When support for gnu hash tables were added to the gcc toolchains, the OpenJDK build system added a hack to change --hash-style=gnu to --hash-style=both unconditionally, citing compatibility concerns. Exactly how are these "hash styles" used? What difference does it make what style is used? Who raised the compatibility concerns? It's how the hash table for the dynamic linker is implemented. "sysv" is the original, old-style hash table, which has several deficits. [1] The modern gnu-style hash table has been existing in the dynamic linker for ages. The compatibility concerns were likely raised due to Solaris, or that this was a new thing back in the days. I have not heard about who originally claimed there was an issue, but when I wanted to remove it years ago when we created the new build system, Looks good to me. someone said like "eh, I dunno, I think someone said it needed to be there but he's no longer with the company, but I'd leave it in place if I were you", or something along those lines. I think we should go with "if it breaks anything, we can always change it back" :-) Thanks - Ioi /Magnus [1] https://www.akkadia.org/drepper/dsohowto.pdf Thanks, David --hash-style=gnu has a very slight performance advantage over --hash-style=both (very small static footprint reductions, tiny startup cost differences). Both --hash-style=gnu and --hash-style=both have a small to potentially large performance advantage over --hash-style=sysv We should only use --hash-style=gnu. It's been the default in Ubuntu's build of gcc since gcc 4.4[1], and likely similar in other distros. I also removed LIBJSIG_NOEXECSTACK_LDFLAGS which is no longer used. Bug: https://bugs.openjdk.java.net/browse/JDK-8200738 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8200738-use-hash-style-gnu/webrev.01 /Magnus [1] https://launchpad.net/ubuntu/maverick/+source/gcc-4.4/+changelog
Re: RFR: JDK-8246478 Remove src/utils/reorder
Looks good! Thanks - Ioi On 6/3/20 8:20 AM, Magnus Ihse Bursie wrote: The tools in src/utils/reorder was used to produce the "reorder" map files, which were used as an early optimization technique for native libraries. We have not used reorder files for a long time; they have all been gone, but the reorder tool was not removed at the time. Time to fix it. Bug: https://bugs.openjdk.java.net/browse/JDK-8246478 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8246478-remove-reorder-tool/webrev.01 /Magnus
Re: RFR(S): 8242524: Use different default CDS archives depending on UseCompressOops
Hi Yumin,
Looks good to me. Just one small nit:
+ LogTarget(Info, cds) lt;
+ if (lt.is_enabled()) {
+ lt.print("trying to map %s", _full_path);
+ }
This can be simplified as
log_info(cds)("trying to map %s", _full_path);
NO need for updated webrev.
Thanks
- Ioi
On 5/14/20 10:40 AM, Yumin Qi wrote:
Hi,
Please review:
bug: 8242524: https://bugs.openjdk.java.net/browse/JDK-8242524
webrev: http://cr.openjdk.java.net/~minqi/2020/8242524/webrev-00/
Summary: After 8232069, CDS can create shared archive with
-XX:-UseCompressedOops which also turned off by ZGC. The build with
cds enabled will create basic shared archive classes.jsa which is
default with UseCompressedOops turned on. With this change, the second
shared archive classes_nocoops.jsa will be created with
UseCompressedOops turned off. So now with cds enabled build, there are
two shared archive files will be generated: classes.jsa and
classes_nocoops.jsa. The basic shared archive will be chosen at
runtime based on flag setting that classes.jsa selected with
UseCompressedOops on, and classes_nocoops.jsa will be selected with
UseCompressedOops turned off.
Tests: hs-tier1-4, tier1-2.
Thanks
Yumin
Re: Is anyone still building multiple JVMs?
I am just wondering, what are the practical reasons for including two libjvms in the same JDK? We had server/client VMs in the past so we can use the same JDK for running "throughput" jobs vs "desktop/interactive" jobs. But that's no longer needed with advances in tier compilation, etc. Thanks - Ioi On 2/20/20 10:33 AM, Magnus Ihse Bursie wrote: 20 feb. 2020 kl. 16:13 skrev Bob Vandette : Keep in mind that any change here will have an impact on the jlink option that allows for the selection of JVM. Jlink Plugin Name: vm Option: --vm= Description: Select the HotSpot VM in the output image. Default is all Good point. I think if we remove building of multiple JVMs in the same pass, we need to instead add an option to “import” additional JVMs. That way, the user can build the JVM in a separate configuration, so we can simplify the logic to mean one configuration == one JVM variant, and then it would still be possible to create a resulting jimage that consists of multiple JVM variants. /Magnus Bob. On Feb 20, 2020, at 10:04 AM, Magnus Ihse Bursie wrote: On 2020-02-20 12:52, Baesken, Matthias wrote: run a separate task with "configure --with-jvm-variants=minimal && make hotspot". Hello, this would , as far as I know, not produce the same result jdk image with both minimal+server libjvm in the image . So the proposed change sounds a bit like a workaround, but not a real replacement to what we have currently . Well, almost. The resulting minimal/libjvm.so would reside in a different directory, and would have to be copied into place. And the jvm.cfg file needs to be correspondingly updated. All of this is trivial for you to do in your wrapper CI scripts. Just as what Adrian does with zero. The simplicity of that solution compared to the logistical nightmare in the make files does not make a compelling case for keeping the multi-JVM support. However if you do that, you *should* get the same result. If not, then it's a bug somewhere (and that would really explain why this business of having multiple JVMs is hairy!). Give it a try! You can use the compare.sh script to verify that the resulting images are equal. The one thing you need to take care of is making sure that the set of JVM features that code outside Hotspot cares about is matching. This is one of the thing the current system is trying hard to do (and failing, mostly, in all edge cases). E.g., you need to either enable CDS on both the server and the minimal build, or disable it on both. /Magnus Best Regards, Matthias On 2020-02-19 16:59, Baesken, Matthias wrote: Hi Magnus, yes we do. We build (on Linux only currently) "--with-jvm- variants=minimal,server" in our central builds to test that minimal is still working and that is was not destroyed by recent changes . Best Regards, Matthias Is this just to test that minimal is working? If so, you could just as well run a separate task with "configure --with-jvm-variants=minimal && make hotspot". Unless you are actually shipping this configuration, that does not seem like a solid reason for keeping this functionality in the build. /Magnus
Re: Is anyone still building multiple JVMs?
I think we should also ask -- is anyone actually shipping a JDK build with multiple libjvm variants in it? I guess people may be building multiple variants during testing just because it's convenient (and requires less time), but if this is the only reason, then it doesn't seem to be worth the complexity in the build system. (I think are also logics in jlink that are needed to support multiple variants, so those can also be simplified). Thanks - Ioi On 2/19/20 4:26 AM, Magnus Ihse Bursie wrote: Are there still any realistic scenarios where anyone builds multiple variants of Hotspot in the same configuration? This was basically introduced for 32-bit builds, where both the server and the client variant of Hotspot were built. In Oracle at least, we stopped building multiple JVM variants a long time ago. Since the build system is taxed with convoluted logic in places just to support this, I’d prefer to remove it if it is not used anymore. So, is there anyone out there, still doing this? /Magnus
Re: RFR: 8236272: Improve fidelity between contents of default CDS archive and classes loaded at runtime
Hi Claes, This looks OK to me. One thing I noticed is when running -XX:DumpLoadedClassList from the "real" JDK image, with CDS enabled, I get more classes such as these: > java/lang/invoke/BoundMethodHandle$Species_LJ > java/lang/invoke/BoundMethodHandle$Species_LL > java/lang/invoke/BoundMethodHandle$Species_LLL > java/lang/invoke/BoundMethodHandle$Species_ > java/lang/invoke/BoundMethodHandle$Species_L > java/lang/invoke/BoundMethodHandle$Species_LL > java/lang/invoke/BoundMethodHandle$Species_LLL > java/lang/invoke/BoundMethodHandle$Species_ > java/lang/invoke/BoundMethodHandle$Species_L I think these are generated later in the build process. Should we try to get these into the CDS archive as well? My unscientific measurement shows about 500K fewer instructions and up to half a percent elapsed time improvement (vs jdk/jdk, without your patch). Also, these classes are not longer in the classlist, because they are used only when building the archived heap objects. < java/lang/invoke/ClassSpecializer$Factory$1Var < java/lang/module/ModuleDescriptor$Modifier > java/lang/Module$ReflectionData < java/lang/NoSuchMethodError < sun/nio/cs/StandardCharsets$Aliases < sun/nio/cs/StandardCharsets$Cache < sun/nio/cs/Surrogate < sun/nio/cs/Surrogate$Parser < sun/nio/cs/US_ASCII$Encoder < sun/nio/cs/UTF_16 < sun/nio/cs/UTF_16BE < sun/nio/cs/UTF_16LE < ... more ... That might cause a slight degradation when running without archived objects (such as -XX:+UseSerialGC) where these classes will be loaded. Have you measured the differences? Thanks - Ioi On 1/27/20 9:13 AM, Claes Redestad wrote: Hi, when generating the default classlist, doing an extra pass with a -Xshare:dump between means the final classlist will be more in line with what will be used at runtime. Webrev: http://cr.openjdk.java.net/~redestad/8236272/open.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8236272 This removes a number of classes from being dumped into the default archive, effectively reducing size by ~170Kb (~1.5%). The size reduction might be improved upon further in the future by making sure that -Xshare:dump can be limited to not dump classes not listed in the generated classlist, but this requires changes to the runtime and needs to be thoroughly examined. Such potential improvements will probably need this build-time fix to be effective. In the mean-time, this patch provides a small but clear gain. Thanks! /Claes
Re: configuring with --with-jvm-variants=minimal,server makes cds disappear in server
On 1/16/20 9:09 AM, Erik Joelsson wrote: I would lean towards changing the check from "if any of the variants do not support CDS, disable it" to "if any of the variants support CDS, enable it". The cds jvm feature is only added to the variants that support it anyway. If you do this, you probably need to figure out how to explicitly only generate the archive for the supported jvm variants in Images.gmk too. Not sure if the archive is jvm variant dependent or if the layout even supports multiple variants. It will likely get messy. The CDS archive is dependent on the exact libjvm.so, so each JDK image should creates its own CDS archive during the build. I think this can be handled emitting a different value for "BUILD_CDS_ARCHIVE" in the spec.gmk of each variant. Thanks - Ioi /Erik On 2020-01-16 08:22, Baesken, Matthias wrote: Hello, thanks for looking into it . Should I do a check just looking into single JVM configs, something like AC_DEFUN([HOTSPOT_IS_JVM_VARIANT], [ [ [[ " $JVM_VARIANTS " == " $1 " ]] ] ]) if HOTSPOT_IS_JVM_VARIANT(zero) || HOTSPOT_IS_JVM_VARIANT(minimal) || HOTSPOT_IS_JVM_VARIANT(core); then instead of AC_DEFUN([HOTSPOT_CHECK_JVM_VARIANT], [ [ [[ " $JVM_VARIANTS " =~ " $1 " ]] ] ]) if HOTSPOT_CHECK_JVM_VARIANT(zero) || HOTSPOT_CHECK_JVM_VARIANT(minimal) || HOTSPOT_CHECK_JVM_VARIANT(core); then This should remove the error in case of multi-JVM configs . Or maybe the whole check ( if HOTSPOT_CHECK_JVM_VARIANT(zero) || HOTSPOT_CHECK_JVM_VARIANT(minimal) || HOTSPOT_CHECK_JVM_VARIANT(core); then fi ) might be removed , I see not so much value in it because in case one sets --enable-jvm-features directly the check isn’t done . I opened bug : https://bugs.openjdk.java.net/browse/JDK-8237374 JDK-8237374 : configuring with --with-jvm-variants=minimal,server makes cds disappear in server Best regards, Matthias This does indeed look like a bug to me. At least at Oracle, we no longer build any multi JVM configs regularly, so things like this falls through the cracks. /Erik On 2020-01-16 02:18, Baesken, Matthias wrote: Hello, I noticed the following strange "feature" (or is it a bug?) . When building 2 VM variants in one build and using --with-jvm-variants=minimal,server For this, the build works nicely. But I notice that in the server VM, cds is removed. Instead of checking if cds should be enabled... yes I get ( with the following patch that adds tracing) : configure: WARNING: ENABLE_CDS set to false because we found a minimal, core or zero JVM. checking if cds should be enabled... no ... * JVM features: minimal: 'compiler1 minimal serialgc' server: 'compiler1 compiler2 epsilongc g1gc jfr jni-check jvmti management nmt parallelgc serialgc services vm-structs' (patch is --- a/make/autoconf/hotspot.m4 Wed Jan 15 21:20:40 2020 -0800 +++ b/make/autoconf/hotspot.m4 Thu Jan 16 10:24:43 2020 +0100 @@ -528,6 +528,7 @@ if HOTSPOT_CHECK_JVM_VARIANT(zero) || HOTSPOT_CHECK_JVM_VARIANT(minimal) || HOTSPOT_CHECK_JVM_VARIANT(core); then # ..except when the user explicitely requested it with --enable-jvm- features if ! HOTSPOT_CHECK_JVM_FEATURE(cds); then ENABLE_CDS="false" + AC_MSG_WARN([ENABLE_CDS set to false because we found a minimal, core or zero JVM.]) if test "x$enable_cds" = "xyes"; then AC_MSG_ERROR([CDS not implemented for variants zero, minimal, core. Remove --enable-cds.]) Fi Is it expected that cds goes away in "server" when configuring "--with- jvm-variants=minimal,server" ? Looks like a bug to me, should it be fixed (so that cds stays in the server jvm-feature list) ? Thanks, Matthias
