RFR: 8207812: Implement Dynamic CDS Archive

Fri Apr 19 16:29:31 UTC 2019

Calvin,

Many thanks for all the work getting this ready, significantly enhancing the testing and bug fixes.

I marked the CSR as reviewed-by - it looks great!

I reviewed this set of changes - I did not review the tests - I assume you can get someone
else to do that.  I am grateful that Jiangli and Ioi are going to review this also - they are much closer to
the details than I am.

1. Do you have any performance numbers?
1a. Startup: does using a combined dynamic CDS archive + base archive give similar startup benefits
when you have the same classes in the archives?

1b. Do you have samples of uses of the combined dynamic CDS archive + base archive vs. a single
static archive built for an application?
    - how do the sets of archived classes differ?
    - one note was that the AtExit approach exclude list adds anything that has not yet linked - does that make a significant difference in the number of classes that are archived? Does that make a difference in either startup time or in application execution time? I could see that going either way.

1c. Any sense of performance cost for first run - how much time does it take to create an incremental archive?
    - is the time comparable to an existing dump for a single archive for the application?
    - this is an ease-of-use feature - so we are not expecting that to be fast
    - the point is to set expectations in our documentation

2. Footprint
With two archives rather than one, is there a significant footprint difference? Obviously this will vary by app and archive.
Once again, the point is to set expectations.

3. Runtime performance
With two sets of archived dictionaries &  symbolTables - is there any significant performance cost to larger benchmarks, e.g. for class loading lookup for classes that are not in the archives?  Or symbol lookup?

4. Platform support
Which platforms is this supported on?
Which ones did you test? For example, did you run the tests on Windows?

Detailed feedback on the code: Just minor comments - I don’t need to see an updated webrev:

1. metaSpaceShared.hpp
line 156:
what is the hardcoded -100 for? Should that be an enum?

2. jfrRecorder.cpp
So JFR recordings are disabled if DynamicDumpSharedSpaces?
why?
Is that a future rfe?

3. systemDictionaryShared.cpp
Could you possibly add a comment to add_verification_constraint
for if (DynamicDumpSharedSpaces)
   return false

-- 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.

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?

5. compactHashtable.cpp
serialize/header/calculate_header_size
-- could you dynamically determine size_of header so you don't need
to hardcode a 5?

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.

I have forgotten:
Today with UseSharedSpaces - do we allow these flags? Is that also the same behavior with the dynamic
archive?

7. classLoaderExt.cpp
assert line 66: only used with -Xshare:dump
 -> "only used at dump time"

8. symbolTable.cpp
line 473: comment // used by UseSharedArchived2
— command-line arg name has changed

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"
  - so base layer can not have appended boot class path
  - and base layer can't have a module path

What can you specify for the dynamic dumping relative to the base archive?
  - matching class path?
  - appended class path?
  in future - could it have a module path that matched the base archive?

Should any of these restrictions be clarified in documentation/CSR since they appear to be new?

10. filemap.cpp
check_archive
Do some of the return false paths skip performing os::close(fd)?

and get_base_archive_name_from_header
Does the first return false path fail to os::free(dynamic_header)

lines 753-754: two FIXME comments

Could you delete commented out line 1087 in filemap.cpp ?

11. filemap.hpp
line 214: TODO left in

12. metaspace.cpp
line 1418 FIXME left in

13. java.cpp
FIXME: is this the right place?
For starting the DynamicArchive::dump

Please check with David Holmes on that one

14. dynamicArchive.hpp
line 55 (and others): MetsapceObj -> MetaspaceObj

15. dynamicArchive.cpp
line 285 rel-ayout -> re-layout

lines 277 && 412
Do we archive array klasses in the base archive but not in the dynamic archive?
Is that a potential RFE?
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"

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?

thanks,
Karen

> On Apr 11, 2019, at 5:18 PM, Calvin Cheung <calvin.cheung at oracle.com> wrote:
> 
> This is a follow-up on the preliminary code review sent by Jiangli in January[1].
> 
> Highlights of changes since then:
> 1. New vm option for dumping a dynamic archive (-XX:ArchiveClassesAtExit=<dynamic archive>) and enhancement to the existing -XX:SharedArchiveFile option. Please refer to the corresponding CSR[2] for details.
> 2. New way to run existing AppCDS tests in dynamic CDS archive mode. At the jtreg command line, the user can run many existing AppCDS tests in dynamic CDS archive mode by specifying the following:
>    -vmoptions:-Dtest.dynamic.cds.archive=true <jdk>/open/test/hotspot/jtreg:hotspot_appcds_dynamic
>   We will have a follow-up RFE to determine in which tier the above tests should be run.
> 3. Added more tests.
> 4. Various bug fixes to improve stability.
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8207812
> webrev: http://cr.openjdk.java.net/~ccheung/8207812_dynamic_cds_archive/webrev.00/
> 
> (The webrev is based on top of the following rev: http://hg.openjdk.java.net/jdk/jdk/rev/805584336738)
> 
> Testing:
>    - mach5 tiers 1- 3 (including the new tests)
>    - AppCDS tests in dynamic CDS archive mode on linux-x64 (a few tests require more investigation)
> 
> thanks,
> Calvin
> 
> [1] https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2019-January/032176.html
> [2] https://bugs.openjdk.java.net/browse/JDK-8221706