RFR: 8207812: Implement Dynamic CDS Archive

[Calvin Cheung]

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/

Please see my replies inline below.

On 4/19/19, 9:29 AM, Karen Kinnear wrote:
> 1. metaSpaceShared.hpp
> line 156:
> what is the hardcoded -100 for? Should that be an enum?
I don't know what the -100 means either. The function in question is not 
new code. It was moved from the metaSpaceShared.cpp. I've moved it back 
there.
>
> 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.
> Is that a future rfe?
>
> 3. systemDictionaryShared.cpp
> Could you possibly add a comment to add_verification_constraint
> for if (DynamicDumpSharedSpaces)
>    return false
Comment added.
>
> -- I think the logic is:
>   because we have successfully linked any instanceKlass we archive 
> with DynamicDumpSharedSpaces, we have resolved all the constraint classes.
>
> -- I didn't check the order - is this called before or after
> excluding? If after, then would it make sense to add an assertion
> here is_linked? Then if you ever change how/when linking is done, this 
> might catch future errors.
Not all InstanceKlass are linked at that point; the ones failed 
verification won't be linked.
>
> 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);
   }
>
> 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.
>
> 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.
>
> I have forgotten:
> Today with UseSharedSpaces - do we allow these flags? Is that also the 
> same behavior with the dynamic
> archive?
Yes, same behavior with the dynamic archive. We ran those tests in 
dynamic archive mode.
>
> 7. classLoaderExt.cpp
> assert line 66: only used with -Xshare:dump
>  -> "only used at dump time"
Done.
>
> 8. symbolTable.cpp
> line 473: comment // used by UseSharedArchived2
> — command-line arg name has changed
Actually, the entire SymbolTableCreateEntry class can be removed.
It was left there probably due to merge error.
>
> 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.
>
> Should any of these restrictions be clarified in documentation/CSR 
> since they appear to be new?
I'll update the doc.
>
> 10. filemap.cpp
> check_archive
> Do some of the return false paths skip performing os::close(fd)?
Fixed.
>
> and get_base_archive_name_from_header
> Does the first return false path fail to os::free(dynamic_header)
Fixed.
>
> lines 753-754: two FIXME comments
I implemented the fist FIXME.
The second one is not needed. I've changed it to a comment.
>
> Could you delete commented out line 1087 in filemap.cpp ?
Done.
>
> 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.
>
> 12. metaspace.cpp
> line 1418 FIXME left in
Removed.
>
> 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. 

>
> 14. dynamicArchive.hpp
> line 55 (and others): MetsapceObj -> MetaspaceObj
Done
>
> 15. dynamicArchive.cpp
> line 285 rel-ayout -> re-layout
Done
>
> 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.
> Is it possible that GatherKlassesAndSymbols::do_unique_ref could be 
> called with an array class?
> Same question for copy_impl?
>
> line 934: "no onger" -> "no longer"
Done
>
> 16. What is AllowArchivingWithJavaAgent? Is that a hook for a 
> potential future rfe?
> Do you want to check in that code at this time? In product?
The flag was added for the 
https://bugs.openjdk.java.net/browse/JDK-8201375 in JDK12.
I needed to add the same check for the dynamic archive case.

thanks,
Calvin