RFR: 8207812: Implement Dynamic CDS Archive

[Karen Kinnear]

Calvin,

Thank you for the updated webrev as well as the detailed explanations.
These changes look good.

thanks,
Karen

minor notes below

> On Apr 23, 2019, at 6:55 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> Hi Karen,
> 
> The following incremental webrev should have addressed most of your comments:
>     http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/ <http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/delta_01_02/>
> 
> Please see my replies inline below.
> 
>> 
>> 2. jfrRecorder.cpp
>> So JFR recordings are disabled if DynamicDumpSharedSpaces?
>> why?
> It was also done for DumpSharedSpaces via the fix for https://bugs.openjdk.java.net/browse/JDK-8203664 <https://bugs.openjdk.java.net/browse/JDK-8203664>.
>> Is that a future rfe?
So I get that right now we don’t support JFR recordings. Thank you for the link - which explores at least
two issues with doing that 1) JFR rewrites some core libraries classes to add tracing and 2) issues with
add-exports/add-reads. So I get that it would not be easy to figure out a way to do this.
My concern is for the longer-term goal of having the user able to do a first run which creates and archive
and subsequent runs - with the same command-line - which uses the archive.
So could you please consider filing an RFE for future to investigate this more to see if there is a possible
way to make this work?
>> 
>> 
>> 4. systemDictionaryShared.cpp
>> EstimateSizeForArchive::do_entry
>> Is it the case that for info.is_builtin() there are no verification constraints? So you could skip that calculation? Or did I misunderstand?
> 
> The size also includes header and crc sizes:
>   static size_t byte_size(InstanceKlass* klass, int num_constraints) {
>     return header_size_size() +
>            crc_size(klass) +
>            verifier_constraints_size(num_constraints) +
>            verifier_constraint_flags_size(num_constraints);
>   }
Thank you. So my assumption is accurate about no verification constraints, and the call is needed anyway.
>> 
>> 5. compactHashtable.cpp
>> serialize/header/calculate_header_size
>> -- could you dynamically determine size_of header so you don't need
>> to hardcode a 5?
> I 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.
Thank you for trying.
>> 
>> 6. classLoader.cpp
>> line 1337: //FIXME: DynamicDumpSharedSpaces and --patch-modules are mutually exclusive. 
>> Can you clarify for me:
>> My memory of the base archive is that we do not allow the following options at dump time - and these
>> are the same for the dynamic archive: —limit-modules, —upgrade-module-path, —patch-module.
> Yes, the same should apply for the dynamic archive.
> FIXME has been replaced with a comment.
Thank you.
>> 
>> 9. filemap.cpp
>> Comment lines 529 ...
>> Is this true - that you can only support dynamic dumping with the default CDS archive? Could you clarify what the restrictions are?
>> The CSR implies you can support “a specific base CDS archive"
> Yes.
>>   - so base layer can not have appended boot class path
>>   - and base layer can't have a module path
> Correct.
>> 
>> What can you specify for the dynamic dumping relative to the base archive?
>>   - matching class path?
>>   - appended class path?
> Yes.
>>   in future - could it have a module path that matched the base archive?
> Sure, in another RFE.
Yes - part of this exercise is identifying future RFEs.
>> 
>> Should any of these restrictions be clarified in documentation/CSR since they appear to be new?
> I'll update the doc.
Thank you.
>> 
>> 11. filemap.hpp
>> line 214: TODO left in
> I leave it there for now. It isn't too simple to get rid of the static declaration.
> I can do a follow up after this RFE.
It doesn’t bother me that they are static - I don’t need an RFE here.
I just figured with a TODO that you meant to get back to it.
>> 
>> 13. java.cpp
>> FIXME: is this the right place?
>> For starting the DynamicArchive::dump
>> 
>> Please check with David Holmes on that one
> 
> I've removed the FIXME. I've also check with David H. He said the following:
>> Not an easy question to answer It depends on all the code that might be touched through DynamicArchive::dump() and whether it might interact with anything already "shutdown". It will really come down to testing (run all tests with dynamic dumping enabled) to see if there are any unexpected interactions.
Have to agree with him - not an easy question to answer - and the benefits of testing :-)
>> 
>> lines 277 && 412
>> Do we archive array klasses in the base archive but not in the dynamic archive?
>> Is that a potential RFE?
> We currently don't handle array klasses in the AppCDS archive either.
> Please refer to: open/test/hotspot/jtreg/runtime/appcds/javaldr/ArrayTest.java.
Thank you. I missed that.
>> 
> 
> thanks,
> Calvin