RFR: 8207812: Implement Dynamic CDS Archive
Calvin Cheung
calvin.cheung at oracle.com
Thu May 2 01:23:10 UTC 2019
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