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. LGTM - Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8478
Re: RFR: 8253495: CDS generates non-deterministic output [v8]
On Tue, 15 Mar 2022 17:08:27 GMT, Ioi Lam wrote: >> This patch makes the result of "java -Xshare:dump" deterministic: >> - Disabled new Java threads from launching. This is harmless. See comments >> in jvm.cpp >> - Fixed a problem in hashtable ordering in heapShared.cpp >> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random >> bits. Added code to zero it. >> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in >> make/scripts/compare.sh >> >> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic. >> This will be fixed in >> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828). >> >> Testing under way: >> - tier1~tier5 >> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline, >> windows-x86-cmp-baseline, etc). > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > Avoid memset twice in os::malloc(); added comments about > NMTPreInit::handle_malloc vs DumpSharedSpaces Marked as reviewed by ccheung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8253495: CDS generates non-deterministic output [v6]
On Fri, 11 Mar 2022 06:55:23 GMT, Ioi Lam wrote:
>> This patch makes the result of "java -Xshare:dump" deterministic:
>> - Disabled new Java threads from launching. This is harmless. See comments
>> in jvm.cpp
>> - Fixed a problem in hashtable ordering in heapShared.cpp
>> - BasicHashtableEntry has a gap on 64-bit platforms that may contain random
>> bits. Added code to zero it.
>> - Enabled checking of $JAVA_HOME/lib/server/classes.jsa in
>> make/scripts/compare.sh
>>
>> Note: $JAVA_HOME/lib/server/classes_ncoops.jsa is still non-deterministic.
>> This will be fixed in
>> [JDK-8282828](https://bugs.openjdk.java.net/browse/JDK-8282828).
>>
>> Testing under way:
>> - tier1~tier5
>> - Run all *-cmp-baseline jobs 20 times each (linux-aarch64-cmp-baseline,
>> windows-x86-cmp-baseline, etc).
>
> Ioi Lam has updated the pull request incrementally with one additional commit
> since the last revision:
>
> Added helper function CollectedHeap::zap_filler_array_with
CDS changes look good. One minor comment on a test.
test/hotspot/jtreg/runtime/cds/appcds/javaldr/LockDuringDumpAgent.java line 65:
> 63: if (elapsed >= timeout) {
> 64: System.out.println("This JVM may decide to not
> launch any Java threads during -Xshare:dump.");
> 65: System.out.println("This is OK because no string
> objects be in a locked state during heap dump.");
Should `no string objects be` be `no string objects could be`?
-
Marked as reviewed by ccheung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7748
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v4]
On Fri, 25 Jun 2021 22:28:50 GMT, Yumin Qi wrote: >> src/hotspot/share/cds/lambdaProxyClassDictionary.hpp line 28: >> >>> 26: #define SHARED_CDS_LAMBDAPROXYCLASSINFO_HPP >>> 27: #include "cds/metaspaceShared.hpp" >>> 28: #include "classfile/javaClasses.hpp" >> >> Is this include necessary? > > It is needed, line 84: > return java_lang_String::hash_code((const jbyte*)sym->bytes(), > sym->utf8_length()); I see. - PR: https://git.openjdk.java.net/jdk/pull/4568
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 Latest changes look good. One question. src/hotspot/share/cds/lambdaProxyClassDictionary.hpp line 28: > 26: #define SHARED_CDS_LAMBDAPROXYCLASSINFO_HPP > 27: #include "cds/metaspaceShared.hpp" > 28: #include "classfile/javaClasses.hpp" Is this include necessary? - Marked as reviewed by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v3]
On Thu, 24 Jun 2021 03:11:35 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: > > Reorder including header files, remove unused function > dd_to_dump_time_lambda_proxy_class_dictionary > > _Mailing list message from [David Holmes](mailto:david.hol...@oracle.com) > > on [hotspot-runtime-dev](mailto:hotspot-runtime-...@mail.openjdk.java.net):_ > > On 24/06/2021 2:23 am, Yumin Qi wrote: > > > On Wed, 23 Jun 2021 06:08:41 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 > > > > > > > > > > _Mailing list message from [David Holmes](mailto:david.holmes at > > > > oracle.com) on [build-dev](mailto:build-dev at mail.openjdk.java.net):_ > > > > Hi Yumin, > > > > On 23/06/2021 4:19 pm, 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. > > > > > > > > > > > > I'm not really seeing a consistent or recognisable naming pattern here. > > > > We seem to have a mix of: > > > > > > > > * cds/foo.cpp > > > > * cds/fooShared.cpp > > > > * cds/sharedFoo.cpp > > > > Can we establish a simple naming scheme here? > > > > Thanks, > > > > David > > > > > > > > > Thanks David. I was thinking of that too. The best practice is for a > > > class Foo we have foo.hpp for definition and foo.cpp for implementation. > > > Here indeed exists non-consistency that I put DumpTime/RunTtime in a > > > single file. Let me double check and update. > > > > > > That's not what I was saying. I'm talking about the names of the cpp > > file and whether they contain "shared" and whether it is a prefix or > > postfix. There doesn't seem to be a consistent naming scheme employed here. > > That comes from day one. The case class FooShared is like > cds/fooShared.[ch]pp > Usually it is for a class with counterpart of a non-shared version, like > Metaspace, SystemDictionary etc. > The classes [DumpTime/RunTime]SharedClassInfo are used for shared only, they > don't have non-shared version. The "Shared" embedded just an indication of > used in CDS, without it is OK i think. I agree that it's fine to drop the word "Shared" from the [DumpTime/RunTime]SharedClassInfo so the the new files could be named [dump|run]TimeClassInfo.[c|h]pp instead. - PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR: 8268821: Split systemDictionaryShared.cpp [v2]
On Wed, 23 Jun 2021 20:45:49 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: > > Split further sharedClassInfo.hpp to dumpTimeSharedClassInfo.hpp and > runTimeSharedClassInfo.hpp to conform to simple rule of separation of > definition and implementation Looks good. Few minor comments. src/hotspot/share/cds/cdsProtectionDomain.cpp line 34: > 32: #include "classfile/vmClasses.hpp" > 33: #include "classfile/vmSymbols.hpp" > 34: #include "classfile/symbolTable.hpp" Please move line 34 before line 31. src/hotspot/share/cds/dumpTimeSharedClassInfo.hpp line 31: > 29: #include "cds/archiveBuilder.hpp" > 30: #include "cds/archiveUtils.hpp" > 31: #include "cds/metaspaceShared.hpp" Please move line 28 after line 31. src/hotspot/share/cds/metaspaceShared.cpp line 35: > 33: #include "cds/lambdaFormInvokers.hpp" > 34: #include "cds/metaspaceShared.hpp" > 35: #include "cds/cdsProtectionDomain.hpp" This line should be after line 27. src/hotspot/share/classfile/systemDictionaryShared.hpp line 291: > 289: static void start_dumping() NOT_CDS_RETURN; > 290: static bool is_supported_invokedynamic(BootstrapInfo* bsi) > NOT_CDS_RETURN_(false); > 291: static void > dd_to_dump_time_lambda_proxy_class_dictionary(LambdaProxyClassKey key, > InstanceKlass* proxy_klass); I don't think this is being referenced. - Changes requested by ccheung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4568
Re: RFR(L): 8199255: [TESTBUG] Open source VM testbase default methods tests
Hi Misha,
I've compared the file.list from your closed webrev with the one from
this open webrev and didn't see any missing files.
Also spot checked a few copyright headers and they look good.
Regarding TEST.groups, why was the following removed?
1160 vmTestbase_nsk_stress = \
1161 vmTestbase/nsk/stress
Could you also remove the extra blank line added at line 1273?
thanks,
Calvin
On 5/21/18, 11:34 AM, Mikhailo Seledtsov wrote:
Please review this change that will open source VM default method tests.
These tests have been used internally for a while, and are now being
open sourced. Since this is not an creation of new tests, we would
like to keep the changes during this review to a minimum required for
open sourcing these tests, such as major issues and integration
blockers. If you have other feedback regarding improvements to these
tests, please file RFE(s) that will be addressed later in order of
priority.
Here is what was done for this change:
1. Moved the tests to OpenJDK repository to the specified directory
location and structure.
3. Updated Copyright statements accordingly.
4. Updated "@library" statements accordingly.
5. Updated TEST.groups and a HotSpot test make file
JBS:https://bugs.openjdk.java.net/browse/JDK-8199255
Webrev: http://cr.openjdk.java.net/~mseledtsov/8199255.01/
Testing:
1. Ran the following tests on open-only repository and build,
using "make run-test" (Linux-x64)
vmTestbase_vm_defmeth
All PASS
2. Automated multip-platform test system (usual 4 platforms):
- vmTestbase_vm_defmeth
- hs-tier{1,2}
In progress
Thank you,
Misha
Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete
On 4/26/18, 12:00 PM, Jiangli Zhou wrote:
Hi Calvin,
On Apr 26, 2018, at 10:10 AM, Calvin Cheung<calvin.che...@oracle.com> wrote:
On 4/25/18, 9:34 PM, Jiangli Zhou wrote:
Here is the incremental webrev with updates that incorporate all feedbacks:
http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev_inc.02/
Looks good.
Thanks!
- Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for
TestCommon.makeCommandLineForAppCDS() cleanup.
- Removed case 2, 3 and 4 in SharedArchiveFile.java.
To address David's comment on checking the result of -Xshare:dump, you can
replace
52 out.shouldContain("Dumping");
with
TestCommon.checkDump(out);
checkDump() contains the following check:
output.shouldContain("Loading classes to share");
No need to generate another webrev if you make the above change.
I removed appcds/SharedArchiveFile.java (please see my reply to David for
details).
- Removed UseAppCDS.java test.
Since you've removed the UseAppCDS.java, I think the UseAppCDS_Test.java should
also be removed.
It would involve changing the GraalWithLimitedMetaspace.java; the test could
just use the test-classes/Hello.java instead of UseAppCDS_Test.java.
This cleanup can be done later, perhaps as part of the bug you've filed.
Could you please specify the connection between UseAppCDS.java and
UseAppCDS_Test.java?
UseAppCDS.java dump the classlist by running the UseAppCDS_Test.
It then creates an archive based on the classlist.
It then runs the UseAppCDS_Test with the archive.
The UseAppCDS_Test simply does the following:
public class UseAppCDS_Test {
// args are from UseAppCDS:
// args[0] = TEST_OUT
public static void main(String[] args) {
System.out.println(args[0]);
}
}
GraalWithLimitedMetaspace.java depends on UseAppCDS_Test in a similar
way but it only performs dumping.
thanks,
Calvin
- Removed UseAppCDS in various tests.
- Changed to keep the original version of the classlist and renamed to
classlist.raw.
- Changed check_nonempty_dir_in_shared_path_table() to report all non-empty
directories in the shared path table entries before exiting VM.
Full webrev:
http://java.se.oracle.com:10065/mdash/jobs/jianzhou-jdk-20180426-0406-20150
I think the correct URL is:
http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.02/ ?
Yes.
Thanks!
Jiangli
thanks,
Calvin
Tested all modified tests locally. Rerunning hs-tier1 ~ hs-tier5 tests.
Thanks for all the suggestions!
Jiangli
On Apr 25, 2018, at 5:24 PM, Jiangli Zhou<jiangli.z...@oracle.com> wrote:
Hi David,
On Apr 25, 2018, at 3:12 PM, David Holmes<david.hol...@oracle.com> wrote:
Hi Jiangli,
On 26/04/2018 3:39 AM, Jiangli Zhou wrote:
Hi David,
Thanks a lot for the review!
On Apr 24, 2018, at 11:17 PM, David Holmes<david.hol...@oracle.com> wrote:
cc'ing build-dev for the makefile change
Hi Jiangli,
On 25/04/2018 10:53 AM, Jiangli Zhou wrote:
Please review the following changes that streamline the support for application
class data sharing by eliminating the requirement of using -XX:+UseAppCDS to
enable the feature. The support for application class data sharing is now
enabled transparently when application classes or platform classes present in
the classlist or generated archive with -Xshare:dump/on/auto.
RFE: https://bugs.openjdk.java.net/browse/JDK-8193213
Bug: https://bugs.openjdk.java.net/browse/JDK-8182731
These issues are not publicly visible, can you change them to be so?
Done. Thanks for noticing that!
webrev: http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.00/
Overall this seems fine. Thanks for explaining the logic changes below - much
appreciated!
One query: why was UseAppCDS not removed from the tests (or at least the tests
you were modifying anyway) ? They will all need updating before 12 when the
flag is expired.
The usages are left as backwards compatibility check. They also serve the
testing purpose to make sure the presence of the flag does not cause any
unexpected behavior. Those are the main reasons why I didn’t remove the flag
usages in this webrev.
This sounds like you are basically testing whether obsoleting the flag has
worked correctly.
Right, that was part of the intention.
You don't need to do that through formal testing. A simple run of "java
-XX:+UseAppCDS" should show the obsoletion warning and that's that. We don't
maintain an obsolete flag test the way we do for deprecated flags.
That sounds reasonable.
So IMHO there's really no reason to keep the flag in any of the tests. As I
said they will all have to be removed when the flag is expired in 12.
Ok. I’ll remove the flag from the tests.
Thanks!
Jiangli
Thanks,
David
test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java
Test 2 reference to UseAppCDS seems unnecessary now.
Test 3 is not needed as you're not using any diagnostic flag now.
Re: RFR: 8193213 & 8182731: Make the UseAppCDS option obsolete
On 4/25/18, 9:34 PM, Jiangli Zhou wrote:
Here is the incremental webrev with updates that incorporate all feedbacks:
http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev_inc.02/
Looks good.
- Filed JDK-8202282 (https://bugs.openjdk.java.net/browse/JDK-8202282) for
TestCommon.makeCommandLineForAppCDS() cleanup.
- Removed case 2, 3 and 4 in SharedArchiveFile.java.
To address David's comment on checking the result of -Xshare:dump, you
can replace
52 out.shouldContain("Dumping");
with
TestCommon.checkDump(out);
checkDump() contains the following check:
output.shouldContain("Loading classes to share");
No need to generate another webrev if you make the above change.
- Removed UseAppCDS.java test.
Since you've removed the UseAppCDS.java, I think the UseAppCDS_Test.java
should also be removed.
It would involve changing the GraalWithLimitedMetaspace.java; the test
could just use the test-classes/Hello.java instead of UseAppCDS_Test.java.
This cleanup can be done later, perhaps as part of the bug you've filed.
- Removed UseAppCDS in various tests.
- Changed to keep the original version of the classlist and renamed to
classlist.raw.
- Changed check_nonempty_dir_in_shared_path_table() to report all non-empty
directories in the shared path table entries before exiting VM.
Full webrev:
http://java.se.oracle.com:10065/mdash/jobs/jianzhou-jdk-20180426-0406-20150
I think the correct URL is:
http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.02/ ?
thanks,
Calvin
Tested all modified tests locally. Rerunning hs-tier1 ~ hs-tier5 tests.
Thanks for all the suggestions!
Jiangli
On Apr 25, 2018, at 5:24 PM, Jiangli Zhou wrote:
Hi David,
On Apr 25, 2018, at 3:12 PM, David Holmes wrote:
Hi Jiangli,
On 26/04/2018 3:39 AM, Jiangli Zhou wrote:
Hi David,
Thanks a lot for the review!
On Apr 24, 2018, at 11:17 PM, David Holmes wrote:
cc'ing build-dev for the makefile change
Hi Jiangli,
On 25/04/2018 10:53 AM, Jiangli Zhou wrote:
Please review the following changes that streamline the support for application
class data sharing by eliminating the requirement of using -XX:+UseAppCDS to
enable the feature. The support for application class data sharing is now
enabled transparently when application classes or platform classes present in
the classlist or generated archive with -Xshare:dump/on/auto.
RFE: https://bugs.openjdk.java.net/browse/JDK-8193213
Bug: https://bugs.openjdk.java.net/browse/JDK-8182731
These issues are not publicly visible, can you change them to be so?
Done. Thanks for noticing that!
webrev: http://cr.openjdk.java.net/~jiangli/8193213_8182731/webrev.00/
Overall this seems fine. Thanks for explaining the logic changes below - much
appreciated!
One query: why was UseAppCDS not removed from the tests (or at least the tests
you were modifying anyway) ? They will all need updating before 12 when the
flag is expired.
The usages are left as backwards compatibility check. They also serve the
testing purpose to make sure the presence of the flag does not cause any
unexpected behavior. Those are the main reasons why I didn’t remove the flag
usages in this webrev.
This sounds like you are basically testing whether obsoleting the flag has
worked correctly.
Right, that was part of the intention.
You don't need to do that through formal testing. A simple run of "java
-XX:+UseAppCDS" should show the obsoletion warning and that's that. We don't
maintain an obsolete flag test the way we do for deprecated flags.
That sounds reasonable.
So IMHO there's really no reason to keep the flag in any of the tests. As I
said they will all have to be removed when the flag is expired in 12.
Ok. I’ll remove the flag from the tests.
Thanks!
Jiangli
Thanks,
David
test/hotspot/jtreg/runtime/appcds/SharedArchiveFile.java
Test 2 reference to UseAppCDS seems unnecessary now.
Test 3 is not needed as you're not using any diagnostic flag now.
Test 4 seems unnecessary now.
Will do.
test/hotspot/jtreg/runtime/appcds/TestCommon.java
makeCommandLineForAppCDS should just be removed (yes that will cause some churn
in the other tests) - but this can be a follow up test cleanup.
Ok.
test/hotspot/jtreg/runtime/appcds/UseAppCDS.java
This test no longer makes much sense to me. The only tests should be that app
classes get archived and used. It makes no sense to claim to be testing
-XX:+UseAppCDS when that flag is obsolete. Likewise now that it is obsolete you
don't need to test that -XX:-UseAppCDS has no affect.
Sounds reasonable. I’ll remove UseAppCDS.java. There are existing tests that
check app classes get archived and used.
Thanks!
Jiangli
Thanks,
David
Highlights of the changes:
* UseAppCDS is obsolete in JDK 11
* Remove the +UnlockCommercialFeatures requirement when using application class
data sharing in OracleJDK binary
*
Re: RFR (XS) 8022740 - Visual 2008 IDE build is broken
Hi Ioi, I've tried your patch on vs2010 and it seems working fine. I was able to generate a project file and used vs2010 to open the project file. So I think your fix is good. (I'm not a Reviewer) Calvin On 8/9/2013 5:39 PM, Ioi Lam wrote: |Please review a very small fix:|| || ||http://cr.openjdk.java.net/~iklam/8022740/vs2008_ide_001/|| || ||Bug: Visual 2008 IDE build is broken|| || ||http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022740|| ||https://jbs.oracle.com/bugs/browse/JDK-8022740|| || ||Summary of fix:|| || ||Apparently some refactoring (7163863??) of the VC project creator has|| ||not been tested with VS2008.|| || ||I made some minor fixes. I verified that VS2008 works.|| || ||I changed one file related to VS2010. Could someone with VS2010 give it a try?|| ||Thanks|| ||- Ioi|| || |
