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.
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
* SharedArchiveFile is a product flag for all configurations in JDK 11
* DumpLoadedClassList produces a complete classlist including all loaded
library and application classes
Thanks David Holmes for his guidance on CSR process.
Following are the details for the VM and test changes:
- Removed various ‘if (UseAppCDS)’ checks and UseAppCDS asserts.
- Removed some of the CDS/AppCDS specifics from general class loading code.
- Removed scattered checks for non-empty directory. Dump time non-empty
directory check is done at one central place,
FileMapInfo::check_nonempty_dir_in_shared_path_table() after loading all
classes on the classlist. The behavior is backwards compatible:
- If no app/platform classes are loaded, dump time only reports error if
non-empty directory exists in -Xbootclasspath/a path
- If app/platform classes are loaded, dump time reports error for any
non-empty directory exists in -Xbootclasspath/a path, class path, or module path
- Removed unnecessary ‘if (!DumpSharedSpaces)’ from
SharedClassUtil::initialize(). The code path is only executed when
UseSharedSpaces is enabled.
- Return immediately in SystemDictionaryShared::find_or_load_shared_class() if
the archive header indicates there is no app or platform classes in the shared
archive. This reduces overhead in class loading if the shared archive only
contains system classes. It also provides backwards compatibility in cases
where shared application classes should be disabled. For example, when the
default system class loader is replaced by user provided loader, archived
application classes are inactive (not loaded from archive) at runtime without
affecting the use of archived system classes.
- Changed GenerateLinkOptData.gmk to filter out HelloClasslist from the default
classlist in JDK image, which is generated using DumpLoadedClassList option.
Thanks for David and Ioi's input on this.
- Updated various cds/appcds tests to reflect the JVM changes. The use of
-XX:+UseAppCDS in tests remains unchanged.
- Removed -XX:+UnlockCommercialFeatures for -XX:+UseAppCDS in tests
- Removed -XX:+UnlockDiagnosticVMOptions for -XX:SharedArchiveFile in tests
- Updated NonBootLoaderClasses.java: ensure app/platform classes on classlist
are archived without -XX:+UseAppCDS
- Updated DirClasspathTest.java: reflect the change for non-empty directory
handling
- Updated MismatchedUseAppCDS.java: -XX:-UseAppCDS has no effect
Tested with all cds and appcds tests locally with both fastdebug and release
builds. Tested with hs-teir1, hs-tier2, jdk-tier1 and jdk-tier2. The hs-tier3,
hs-tier4 and hs-tier4 are in progress.
Thanks,
Jiangli
