RFR: 8207812: Implement Dynamic CDS Archive
mikhailo.seledtsov at oracle.com
mikhailo.seledtsov at oracle.com
Thu May 2 02:53:05 UTC 2019
Sounds good,
Thank you,
Misha
On 5/1/19 6:23 PM, Calvin Cheung wrote:
> Hi Misha,
>
> Thanks for reviewing the test portion.
>
> On 5/1/19, 3:21 PM, Mikhailo Seledtsov wrote:
>> Hi Calvin,
>>
>> I did a general high-level review of test portion of the change.
>> Overall looks good, lots of work. A few comments:
>>
>> ===
>> http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/webrev.02/test/hotspot/jtreg/runtime/appcds/TestCommon.java.udiff.html
>> *+ if (DYNAMIC_DUMP) {*
>> *+ // FIXME if failed to map base archive, throw SkippedException*
>> *+ output.shouldContain("Written dynamic archive"); * Are you planning to address this before integration?
> I will change the above to the following:
> diff --git a/test/hotspot/jtreg/runtime/appcds/TestCommon.java
> b/test/hotspot/jtreg/runtime/appcds/TestCommon.java
> --- a/test/hotspot/jtreg/runtime/appcds/TestCommon.java
> +++ b/test/hotspot/jtreg/runtime/appcds/TestCommon.java
> @@ -51,6 +51,7 @@
> import java.util.zip.ZipEntry;
> import java.util.zip.ZipFile;
> import java.util.zip.ZipOutputStream;
> +import jtreg.SkippedException;
> import cdsutils.DynamicDumpHelper;
>
>
> @@ -431,7 +432,9 @@
> String... suffix) throws
> Exception {
> OutputAnalyzer output = dump(appJar, classList, suffix);
> if (DYNAMIC_DUMP) {
> - // FIXME if failed to map base archive, throw
> SkippedException
> + if (isUnableToMap(output)) {
> + throw new SkippedException(UnableToMapMsg);
> + }
> output.shouldContain("Written dynamic archive");
> } else {
> output.shouldContain("Loading classes to share");
>
>> === customLoader/test-classes/HelloUnload.java
>> This is a new file, right? If yes, please fix the copyright notice to 2019 only.
>>
>> === dynamicArchive/AppendClasspath.java
>> Copyright (c) 2014, 2018, -->Copyright (c) 2019,
>>
>> Actually, I see the wrong copyright on many new tests; please fix.
> I will take care of the copyright year before check-in.
>> === appcds/test-classes/GenericTestApp.java
>> There are couple of FIXME comments. Please address if possible.
> I'll address it after the initial integration. This is just a helper
> app used only by a small number of tests.
>> === And a general suggestion (unless you have already done so in some way):
>> Make sure you have a test that test major feature/CLF interactions with dynamic archiving. Such as major GC (where compatible), -Xint/-Xcomp, JFR. Just a simple sanity
>> test with other features. If some features are not compatible, outline this in the comments.
> I ran tests in higher tier up to 7 which includes various combination
> of flags like -Xcomp, -esa, -XX:NativeMemoryTracking, etc. More
> testing is always better.
>
> thanks,
> Calvin
>> Thank you,
>> Misha
>>
>>
>> On 4/30/19, 10:46 AM, Calvin Cheung wrote:
>>> Karen,
>>>
>>> Thanks for taking another look.
>>> I'll take care of the typo you mentioned below.
>>>
>>> thanks,
>>> Calvin
>>>
>>> On 4/30/19, 9:00 AM, Karen Kinnear wrote:
>>>> Code changes look good to go for me.
>>>> Many thanks for the updates and additional testing.
>>>>
>>>> thanks,
>>>> Karen
>>>>
>>>> p.s. minor note: dynamicArchive.hpp line 57 “an copy” -> “a copy"
>>>>
>>>>> On Apr 29, 2019, at 1:13 PM, Calvin Cheung
>>>>> <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>>>>
>>>>> Hi Jiangli,
>>>>>
>>>>> Thanks for the re-review.
>>>>> Please see my comments in-line below...
>>>>>
>>>>> On 4/24/19, 7:54 PM, Jiangli Zhou wrote:
>>>>>> Please see comments inlined.
>>>>>>
>>>>>> On Tue, Apr 23, 2019 at 5:08 PM Calvin
>>>>>> Cheung<calvin.cheung at oracle.com
>>>>>> <mailto:calvin.cheung at oracle.com>> wrote:
>>>>>>> Hi Jiangli,
>>>>>>>
>>>>>>> Thanks a lot for your review!
>>>>>>>
>>>>>>> On 4/22/19, 2:07 PM, Jiangli Zhou wrote:
>>>>>>>> Hi Calvin,
>>>>>>>>
>>>>>>>> Congrats on finalizing the dynamic archiving work and completing
>>>>>>>> testing. After the integration of the dynamic archiving, a
>>>>>>>> follow-up
>>>>>>>> RFE can be done to merge the archiving/copying code in
>>>>>>>> dynamicArchive.* and metaspaceShared.* for better maintenance
>>>>>>>> in the
>>>>>>>> future. As there are many duplicates between those two, having
>>>>>>>> shared
>>>>>>>> implementation for both static and dynamic will be beneficial and
>>>>>>>> reduce the maintenance cost.
>>>>>>> I'll file an RFE for the above.
>>>>>>>> Here are my comments mainly for additional cleanups and some
>>>>>>>> minor issues.
>>>>>>>>
>>>>>>>> - src/hotspot/share/classfile/classLoader.cpp
>>>>>>>>
>>>>>>>> 1337 // FIXME: DynamicDumpSharedSpaces and --patch-modules are
>>>>>>>> mutually exclusive
>>>>>>>> 1338 assert(!DynamicDumpSharedSpaces, "sanity");
>>>>>>>>
>>>>>>>> I tagged the comment with 'FIXME' to serve as a reminder to add
>>>>>>>> more
>>>>>>>> details. The reason DynamicDumpSharedSpaces is 'mutually
>>>>>>>> exclusive'
>>>>>>>> with with --patch-modules because DynamicDumpSharedSpaces is only
>>>>>>>> enabled when UseSharedSpaces is also enabled. As
>>>>>>>> --patch-modules is
>>>>>>>> not supported with UseSharedSpaces, it is not supported with
>>>>>>>> DynamicDumpSharedSpaces either.
>>>>>>> I've converted the FIXME to a comment.
>>>>>>>> 1518 ik->set_shared_classpath_index(UNREGISTERED_INDEX);
>>>>>>>> 1519 SystemDictionaryShared::set_shared_class_misc_info(ik,
>>>>>>>> (ClassFileStream*)stream);
>>>>>>>>
>>>>>>>> Please add assert(DynamicDumpSharedSpaces, "sanity"); to the above
>>>>>>>> code. With the new dynamic archiving capability, it's now able to
>>>>>>>> load/archive a class with user defined classloader via this
>>>>>>>> call path.
>>>>>>>> A comment explaining this is also needed.
>>>>>>> I tried the assert but it didn't work. Not only
>>>>>>> DynamicDumpSharedSpaces
>>>>>>> will go through that code path.
>>>>>> I should be more clear. The new code is only intended for the
>>>>>> DynamicDumpSharedSpaces, since the shared_classpath_index is set to
>>>>>> UNREGISTERED_INDEX by ClassLoaderExt::load_class when loading class
>>>>>> with "source:" in the class list file at static dumping time.
>>>>>>
>>>>>> 1518 ik->set_shared_classpath_index(UNREGISTERED_INDEX);
>>>>>> 1519 SystemDictionaryShared::set_shared_class_misc_info(ik,
>>>>>> (ClassFileStream*)stream);
>>>>>>
>>>>>> After thinking more, it's probably better to remove the following
>>>>>> marked code from ClassLoaderExt::load_class. That avoids setting
>>>>>> twice
>>>>>> in two different places during static dumping. It also makes the
>>>>>> code
>>>>>> cleaner.
>>>>>>
>>>>>> InstanceKlass* ClassLoaderExt::load_class(Symbol* name, const char*
>>>>>> path, TRAPS) {
>>>>>> ...
>>>>>> result->set_shared_classpath_index(UNREGISTERED_INDEX);<<<<<<<<<<<
>>>>>> SystemDictionaryShared::set_shared_class_misc_info(result, stream);
>>>>>> <<<<<<<<<<<<
>>>>> http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/src/hotspot/share/classfile/classLoaderExt.cpp.html
>>>>> <http://cr.openjdk.java.net/%7Eccheung/8207812_dynamic_cds_archive/delta_01_02/src/hotspot/share/classfile/classLoaderExt.cpp.html>
>>>>>
>>>>>
>>>>> Only the first statement is still there. I agree that the
>>>>> set_shared_classpath_indexd() can be removed.
>>>>>>
>>>>>>>> - src/hotspot/share/classfile/classLoaderExt.cpp
>>>>>>>>
>>>>>>>> 64 void ClassLoaderExt::setup_app_search_path() {
>>>>>>>> 65 assert(DumpSharedSpaces || DynamicDumpSharedSpaces,
>>>>>>>> 66 "this function is only used with -Xshare:dump");
>>>>>>>>
>>>>>>>> The above message needs to be updated to reflect the new
>>>>>>>> command-line option.
>>>>>>> Done.
>>>>>>>> 304 result->set_shared_classpath_index(UNREGISTERED_INDEX);
>>>>>>>> 305 SystemDictionaryShared::set_shared_class_misc_info(result,
>>>>>>>> stream);<<<<<<<<<<
>>>>>>>>
>>>>>>>> Why is the set_shared_class_misc_info call being removed? If
>>>>>>>> this is a
>>>>>>>> bug fix for loading classes from the classlist for user defined
>>>>>>>> classloaders, it should be handled separately, and with a
>>>>>>>> separate bug
>>>>>>>> ID as well.
>>>>>>> It is called in ClassLoader::record_result() from
>>>>>>> KlassFactory::create_from_stream().
>>>>>>
>>>>>> Ok, this is related to the above comment.
>>>>>>
>>>>>>>> - src/hotspot/share/classfile/compactHashtable.cpp
>>>>>>>>
>>>>>>>> 207 size_t SimpleCompactHashtable::calculate_header_size() {
>>>>>>>> 208 // We have 5 fields. Each takes up sizeof(intptr_t). See
>>>>>>>> WriteClosure::do_u4
>>>>>>>> 209 size_t bytes = sizeof(intptr_t) * 5;
>>>>>>>> 210 return bytes;
>>>>>>>> 211 }
>>>>>>>>
>>>>>>>> 212
>>>>>>>> 213 void
>>>>>>>> SimpleCompactHashtable::serialize_header(SerializeClosure* soc) {
>>>>>>>> 214 // NOTE: if you change this function, you MUST change
>>>>>>>> the number 5 in
>>>>>>>> 215 // calculate_header_size() accordingly.
>>>>>>>> ...
>>>>>>>>
>>>>>>>> As a cleanup, a better way to handle this is to calculate the size
>>>>>>>> within SimpleCompactHashtable::serialize_header during
>>>>>>>> serializing the
>>>>>>>> data and set the size value in a valuable.
>>>>>>>> SimpleCompactHashtable::calculate_header_size() should simply
>>>>>>>> retrieve
>>>>>>>> the value. A renaming of
>>>>>>>> SimpleCompactHashtable::calculate_header_size() can also be done.
>>>>>>> I've checked with Ioi on this one. The problem is
>>>>>>> calculate_header_size() needs to be called during size
>>>>>>> estimation, and
>>>>>>> serialize_header is called after size estimation.
>>>>>> Can you please file a RFE for this? The current code is okay for the
>>>>>> first integration. It deserves some efforts to make it cleaner
>>>>>> (probably with a different solution) since it can be error-prone.
>>>>> I've filed:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8223004
>>>>> Avoid using a hard-coded number in
>>>>> SimpleCompactHashtable::calculate_header_size()
>>>>>>
>>>>>>>> - src/hotspot/share/classfile/dictionary.cpp
>>>>>>>>
>>>>>>>> 315 InstanceKlass* Dictionary::find_class(Symbol* name) {
>>>>>>>> 316 unsigned int hash = compute_hash(name);
>>>>>>>> 317 int index = hash_to_index(hash);
>>>>>>>> 318 return find_class(index, hash, name);
>>>>>>>> 319 }
>>>>>>>>
>>>>>>>> Looks like the new function is not references (unless I'm missing
>>>>>>>> something). Please remove the function.
>>>>>>>>
>>>>>>>> - src/hotspot/share/classfile/dictionary.hpp
>>>>>>>>
>>>>>>>> 65 InstanceKlass* find_class(Symbol* name);
>>>>>>>>
>>>>>>>> Same comment as the above.
>>>>>>> I've removed the function.
>>>>>>>> - src/hotspot/share/classfile/symbolTable.cpp.
>>>>>>>>
>>>>>>>> 473 Symbol* const _archived; // used by UseSharedArchived2
>>>>>>>>
>>>>>>>> Please removed 'UseSharedArchived2'. The comment also needs
>>>>>>>> more clarifications.
>>>>>>>>
>>>>>>>> I couldn't find any references to SymbolTableCreateEntry. Can you
>>>>>>>> please point to me where it is being used?
>>>>>>> I've removed the entire SymbolTableCreateEntry class. It was
>>>>>>> left there
>>>>>>> probably due to merge error.
>>>>>>>> - src/hotspot/share/classfile/systemDictionaryShared.cpp
>>>>>>>>
>>>>>>>> 1218 if (DynamicDumpSharedSpaces) {
>>>>>>>> 1219 return false;
>>>>>>>> 1220 } else {
>>>>>>>>
>>>>>>>> The above case for DynamicDumpSharedSpaces needs to be examined
>>>>>>>> carefully. Can you please ask Harold (and Coleen or Karen) to
>>>>>>>> take a
>>>>>>>> look? Also, a comment is needed to explain that we can complete
>>>>>>>> all
>>>>>>>> verification checks at dynamic dumping time.
>>>>>>> I've added a comment. If it return false, the caller will call
>>>>>>> VerificationType::resolve_and_check_assignability().
>>>>>>>> - src/hotspot/share/classfile/systemDictionaryShared.cpp
>>>>>>>>
>>>>>>>> 1279 ResourceMark rm;
>>>>>>>>
>>>>>>>> You can use 'ResourceMark rm(THREAD)'.
>>>>>>> Fixed.
>>>>>>>> - src/hotspot/share/memory/allocation.hpp
>>>>>>>>
>>>>>>>> 255 //
>>>>>>>> 256 // When CDS is not enabled, both pointers are set to NULL.
>>>>>>>> 257 static void* _shared_metaspace_base; // (inclusive) low
>>>>>>>> address
>>>>>>>> 258 static void* _shared_metaspace_top; // (exclusive)
>>>>>>>> high addres
>>>>>>>>
>>>>>>>> Why the comment at line 256 was removed?
>>>>>>> I've added back the comment.
>>>>>>>> - src/hotspot/share/memory/filemap.cpp
>>>>>>>>
>>>>>>>> 101 void FileMapInfo::fail_continue(const char *msg, ...) {
>>>>>>>> 102 va_list ap;
>>>>>>>> 103 va_start(ap, msg);
>>>>>>>> 104 if (_runtime_dynamic_info == NULL) {
>>>>>>>> 105 MetaspaceShared::set_archive_loading_failed();
>>>>>>>> 106 } else {
>>>>>>>> 107 DynamicArchive::disable();
>>>>>>>> 108 }
>>>>>>>>
>>>>>>>> The above fail_continue only works if _runtime_dynamic_info is
>>>>>>>> setup
>>>>>>>> after the mapping the base archive. Comments should be add to
>>>>>>>> explain
>>>>>>>> that.
>>>>>>> Comment added.
>>>>>>>> Can you please rename '_runtime_dynamic_info' so it's more
>>>>>>>> descriptive? Maybe use 'dynamic_archive_info'.
>>>>>>> Renamed to '_dynamic_archive_info'.
>>>>>>>> 587 bool FileMapInfo::same_files(const char* file1, const char*
>>>>>>>> file2) {
>>>>>>>>
>>>>>>>> The usage of FileMapInfo::same_files is not necessary and
>>>>>>>> should be
>>>>>>>> removed. The base archive's CRC checksum values are recorded
>>>>>>>> in the
>>>>>>>> dynamic archive. The runtime verifies the CRC values to make
>>>>>>>> sure the
>>>>>>>> same archive is used at dump time and runtime, regardless of
>>>>>>>> the base
>>>>>>>> archive path or name. It is designed for all use cases:
>>>>>>> The same_files() function is also used in arguments.cpp:
>>>>>>> 3530 if (DynamicDumpSharedSpaces) {
>>>>>>> 3531 if (FileMapInfo::same_files(SharedArchiveFile,
>>>>>>> ArchiveClassesAtExit)) {
>>>>>>> 3532 vm_exit_during_initialization(
>>>>>>> 3533 "Cannot have the same archive file specified for
>>>>>>> -XX:SharedArchiveFile and -XX:ArchiveClassesAtExit",
>>>>>>> 3534 SharedArchiveFile);
>>>>>>> 3535 }
>>>>>>> 3536 }
>>>>>>>
>>>>>>> The function is also needed for the RFE:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8211723
>>>>>> Ok. It should be treated a bug, not a RFE.
>>>>>>
>>>>>> The shared path table check does not verify the path ordering (also
>>>>>> including the case when new path components are inserted). The bug
>>>>>> should be handled as a high priority task for dynamic archive.
>>>>> I saw that you and Karen have had some discussion in the bug
>>>>> report since you sent this review comment. I take that you're fine
>>>>> with that.
>>>>>>
>>>>>>> We still verify the CRC values during runtime.
>>>>>>>> * base CDS archive is specified in the -XX:SharedArchiveFile at
>>>>>>>> dynamic dumping time
>>>>>>>> * -XX:SharedArchiveFile is not specified at dynamic dumping time,
>>>>>>>> default location for the default CDS archive is used
>>>>>>>> * default CDS archive is specified in the -XX:SharedArchiveFile
>>>>>>>> at runtime
>>>>>>>> * default CDS archive is not specified in the
>>>>>>>> -XX:SharedArchiveFile at
>>>>>>>> runtime, default location for the default CDS archive is used
>>>>>>> Regarding the fourth point above, the user could have a
>>>>>>> non-default base
>>>>>>> archive and only specify the top archive during runtime.
>>>>>> I would argue against it since it doesn't always work and adds extra
>>>>>> code. When the archive path/name is changed, the recorded one in the
>>>>>> dynamic archive would no longer work. User still need to specify the
>>>>>> path/name in the command-line. The use case only works for the
>>>>>> default
>>>>>> CDS archive. For non-default CDS archive, specifying in the
>>>>>> command-line option results a cleaner design and less fragile code.
>>>>> If the base archive is moved, then the user has to modify the
>>>>> command-line anyway, whether the user initially specified (a) only
>>>>> the top archive, or (b) both archives.
>>>>> Requiring both archives to be specified will only hurt the users
>>>>> who never move the archives.
>>>>>
>>>>> We'd like to leave it as is.
>>>>>
>>>>>>
>>>>>>>> In all above cases, the base archive CRC values check is
>>>>>>>> sufficient.
>>>>>>>> The use of path/name is fragile and should be avoided. That
>>>>>>>> will allow
>>>>>>>> you to remove the _base_archive_name_size from the dynamic
>>>>>>>> archive.
>>>>>>> We still need the _base_archive_name_size and the base archive
>>>>>>> name in
>>>>>>> the header because of the above reason.
>>>>>> Please see my comment above.
>>>>>>
>>>>>>>> 752 if (is_static) {
>>>>>>>> 753 // FIXME check for dynamic header as well
>>>>>>>> 754 // FIXME Don't just check the last region -- check
>>>>>>>> all regions!
>>>>>>>>
>>>>>>>> Can you please address the first FIXME at line 753?
>>>>>>>>
>>>>>>>> Checking the last region is sufficient since the archive is
>>>>>>>> written is
>>>>>>>> sequential order. The second FIXME is not necessary.
>>>>>>> I've addressed the first FIXME and converted the second one to a
>>>>>>> comment.
>>>>>>>> - src/hotspot/share/memory/metaspace.cpp
>>>>>>>>
>>>>>>>> 1417 bool Metaspace::contains(const void* ptr) {
>>>>>>>> 1418 // FIXME: need to check the dynamic archive
>>>>>>>>
>>>>>>>> Can you please remove the above FIXME? There is no need for a
>>>>>>>> separate check.
>>>>>>> Done.
>>>>>>>> - src/hotspot/share/memory/metaspaceShared.cpp
>>>>>>>>
>>>>>>>> 830 intptr_t* MetaspaceShared::fix_cpp_vtable_for_second_archive
>>>>>>>>
>>>>>>>> Can you please rename the function to
>>>>>>>> fix_cpp_vtable_for_dynamic_archive?
>>>>>>> Done.
>>>>>>>> - src/hotspot/share/oops/klass.cpp
>>>>>>>>
>>>>>>>> 527 assert (DumpSharedSpaces || DynamicDumpSharedSpaces,
>>>>>>>> 528 "only called for DumpSharedSpaces");
>>>>>>>>
>>>>>>>> 544 void Klass::remove_java_mirror() {
>>>>>>>> 545 assert(DumpSharedSpaces || DynamicDumpSharedSpaces, "only
>>>>>>>> called for DumpSharedSpaces");
>>>>>>>>
>>>>>>>> Please fix the messages above.
>>>>>>> Done.
>>>>>>>> - src/hotspot/share/prims/whitebox.cpp
>>>>>>>>
>>>>>>>> 2332 {CC"getResolvedReferences",
>>>>>>>> CC"(Ljava/lang/Class;)Ljava/lang/Object;",
>>>>>>>> (void*)&WB_GetResolvedReferences},
>>>>>>>> 2333 {CC"linkClass", CC"(Ljava/lang/Class;)V",
>>>>>>>> (void*)&WB_LinkClass},
>>>>>>>> 2334 {CC"areOpenArchiveHeapObjectsMapped", CC"()Z",
>>>>>>>> (void*)&WB_AreOpenArchiveHeapObjectsMapped},
>>>>>>>>
>>>>>>>> Can you please align the indentation of line 2333 (to be the
>>>>>>>> same as
>>>>>>>> line 2332 or 2334)?
>>>>>>> Aligned (void*) with line 2334. (It doesn't show in the webrev
>>>>>>> since
>>>>>>> only blank space changes)
>>>>>>>> - src/hotspot/share/runtime/arguments.cpp
>>>>>>>>
>>>>>>>> 1491 bool Arguments::check_unsupported_cds_runtime_properties() {
>>>>>>>> 1492 assert(UseSharedSpaces, "this function is only used with
>>>>>>>> -Xshare:{on,auto}");
>>>>>>>> 1493 assert(ARRAY_SIZE(unsupported_properties) ==
>>>>>>>> ARRAY_SIZE(unsupported_options), "must be");
>>>>>>>> 1494 if (ArchiveClassesAtExit != NULL) {
>>>>>>>> 1495 // dynamic dumping, just return false,
>>>>>>>> check_unsupported_dumping_properties() will be called
>>>>>>>> 1496 // in init_shared_archive_paths().
>>>>>>>> 1497 return false;
>>>>>>>> 1498 }
>>>>>>>>
>>>>>>>> The check_unsupported_cds_runtime_properties() should be done
>>>>>>>> for the
>>>>>>>> 'ArchiveClassesAtExit != NULL' case as well. Dynamic dumping is a
>>>>>>>> combination of both dump time and runtime.
>>>>>>> The 'ArchiveClassesAtExit != NULL' is for dumping CDS archive to
>>>>>>> the
>>>>>>> user's point of view, that's why the comments in lines 1495 and
>>>>>>> 1496.
>>>>>>> During runtime, ArchiveClassesAtExit will be NULL, so the
>>>>>>> check_unsupported_cds_runtime_properties() will be called as usual.
>>>>>> During dynamic dumping, UseSharedSpace is true. Dynamic dumping is
>>>>>> special case of the 'runtime', that's why Dynamic dumping it is a
>>>>>> combination of both dump time and runtime. So
>>>>>> check_unsupported_cds_runtime_properties() is also need for dynamic
>>>>>> dumping.
>>>>> If the check_unsupported_cds_runtime_properties() is called for
>>>>> dynamic dumping, the user will see a warning message
>>>>> warning("CDS is disabled when the %s option is specified.",
>>>>> unsupported_options[i]);
>>>>> instead of the following error with my current patch:
>>>>> vm_exit_during_initialization("Cannot use the following option
>>>>> when dumping the shared archive", unsupported_options[i]);
>>>>> which gives the user the same feedback on the static dumping case.
>>>>>
>>>>> We think it is important to give correct error message to the
>>>>> user. We'd like to leave the code as is but have updated the
>>>>> comment as follows:
>>>>>
>>>>> if (ArchiveClassesAtExit != NULL) {
>>>>> // dynamic dumping, just return false for now.
>>>>> // check_unsupported_dumping_properties() will be called later
>>>>> to check the same set of
>>>>> // properties, and will exit the VM with the correct error
>>>>> message if the unsupported properties
>>>>> // are used.
>>>>> return false;
>>>>> }
>>>>>
>>>>> Here's an updated delta webrev:
>>>>> http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02%2b/
>>>>> <http://cr.openjdk.java.net/%7Eccheung/8207812_dynamic_cds_archive/delta_01_02%2b/>
>>>>>
>>>>>
>>>>> Additional changes comparing with delta_01_02 include:
>>>>> - the mentioned one-line removal in classLoaderExt.cpp;
>>>>> - updated comment in arguments.cpp;
>>>>> - small fixes to handle tests run in -Xshare:off mode;
>>>>> - enable building on the zero platform.
>>>>>
>>>>> Comparing with the delta_01_02 webrev, the additional changed
>>>>> files are:
>>>>> > make/hotspot/lib/JvmFeatures.gmk
>>>>> > src/hotspot/share/classfile/symbolTable.hpp
>>>>> > src/hotspot/share/runtime/arguments.hpp
>>>>> >
>>>>> test/hotspot/jtreg/runtime/appcds/dynamicArchive/DynamicArchiveTestBase.java
>>>>> >
>>>>> test/hotspot/jtreg/runtime/appcds/dynamicArchive/HelloDynamicCustom.java
>>>>>
>>>>>
>>>>> Thanks again for your review and contribution to this RFE.
>>>>>
>>>>> Calvin
>>>>>>>> 2729 // -Xshare:auto || -Xshare:dynamicDump
>>>>>>>>
>>>>>>>> As you've renamed the command-line argument for dynamic dumping
>>>>>>>> support, the comment needs to be fixed.
>>>>>>> Fixed.
>>>>>>>> 3125 // Compiler threads may concurrently update the class
>>>>>>>> metadata (such as method entries), so it's
>>>>>>>> 3126 // unsafe with DumpSharedSpaces (which modifies the class
>>>>>>>> metadata in place). Let's disable
>>>>>>>> 3127 // compiler just to be safe.
>>>>>>>> 3128 //
>>>>>>>> 3129 // Note: this is not a concern for
>>>>>>>> DynamicDumpSharedSpaces,
>>>>>>>> which makes a copy of the class metadata
>>>>>>>> 3130 // instead of modifying them in place. The copy is
>>>>>>>> inaccessible to the compiler.
>>>>>>>> 3131 set_mode_flags(_int);
>>>>>>>>
>>>>>>>> We need to come back to revisit the above for the 'static' archive
>>>>>>>> dumping at one point. There is a RFE filed for that, if I remember
>>>>>>>> correctly. Could you please add a 'TODO' notes in the above
>>>>>>>> comment.
>>>>>>> Added TODO.
>>>>>>>> A check should be done in arguments.cpp to make sure
>>>>>>>> DynamicDumpSharedSpaces is not manipulated from the command-line
>>>>>>>> directly. DynamicDumpSharedSpaces should not be enabled in the
>>>>>>>> command-line without ArchiveClassesAtExit being specified.
>>>>>>> Done.
>>>>>>>> - src/hotspot/share/runtime/java.cpp
>>>>>>>>
>>>>>>>> 509
>>>>>>>> 510 // FIXME: is this the right place?
>>>>>>>> 511 if (DynamicDumpSharedSpaces) {
>>>>>>>> 512 DynamicArchive::dump();
>>>>>>>> 513 }
>>>>>>>>
>>>>>>>> Again, the above 'FIXME' is served as a cleanup reminder.
>>>>>>>> Please get
>>>>>>>> opinions from others on this change. If the calling place is okay,
>>>>>>>> please remove the FIXME.
>>>>>>> Removed the FIXME for now. Checked with David H. He indicated
>>>>>>> there's no
>>>>>>> easy answer for this. Just need to do a lot of testing.
>>>>>>>> - test
>>>>>>>>
>>>>>>>> Could you please add a test case for setting
>>>>>>>> DynamicDumpSharedSpaces
>>>>>>>> from command-line?
>>>>>>> Here's an incremental webrev which contains a new test:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/
>>>>>>> <http://cr.openjdk.java.net/%7Eccheung/8207812_dynamic_cds_archive/delta_01_02/>
>>>>>>>
>>>>>>>
>>>>>>> thanks,
>>>>>>> Calvin
>>>>>>>> I only took a brief look of the test changes. Please ask Misha to
>>>>>>>> review the test changes as well.
>>>>>>>>
>>>>>>>> Thanks and regards,
>>>>>>>> Jiangli
>>>>>> Thanks,
>>>>>> Jiangli
>>>>
More information about the hotspot-runtime-dev
mailing list