RFR: 8207812: Implement Dynamic CDS Archive
Calvin Cheung
calvin.cheung at oracle.com
Tue Apr 23 20:26:26 UTC 2019
On 4/23/19, 11:16 AM, Karen Kinnear wrote:
> Calvin,
>
> I added to the CSR a comment from my favorite customer - relative to
> the user model for the command-line flags.
> He likes the proposal to reduce the number of steps a customer has to
> perform to get startup and footprint benefits
> from the archive.
>
> The comment was that it would be very helpful if the user only needed
> to change their scripts once - so
> a single command-line argument would create a dynamic archive if one
> did not exist, and use it if it
> already existed.
>
> Is there a way to evolve the ArchiveClassesAtExit=<dynamic archive> to
> have that functionality?
One drawback of this proposal is that the ArchiveClassesAtExit option
has 2 meanings which I find confusing.
Maybe eventually we can do some kind of automatic CDS archive dumping
without having to specify any command line option. Such as when a java
app is run at the first time, there will be some CDS archive created
with a unique name. Subsequent run of the same app will make use of the
archive.
>
> thanks,
> Karen
>
> p.s. I think it makes more sense to put performance numbers in the
> implementation RFE comments rather than the JEP
> comments
We found a bug yesterday and will run more performance tests. I'll put
the performance numberss in the implementation RFE.
thanks,
Calvin
>
>> On Apr 22, 2019, at 5:16 PM, Jiangli Zhou <jianglizhou at google.com
>> <mailto:jianglizhou at google.com>> wrote:
>>
>> Hi Calvin,
>>
>> Can you please also publish the final performance numbers in the JEP
>> 350 (or the implementation RFE) comment section?
>>
>> Thanks,
>> Jiangli
>>
>> On Mon, Apr 22, 2019 at 10:07 AM Calvin Cheung
>> <calvin.cheung at oracle.com <mailto:calvin.cheung at oracle.com>> wrote:
>>
>> Hi Karen,
>>
>> Thanks for your review!
>> Please see my replies in-line below.
>>
>> On 4/19/19, 9:29 AM, Karen Kinnear wrote:
>> > 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?
>> Below are some performance numbers from Eric, each number is for
>> 50 runs:
>> (base: using the default CDS archive,
>> test: using the dynamic archive,
>> Eric will get some numbers with a single archive which I think that's
>> what you're looking for)
>>
>> Lambda-noop:
>> base:
>> 0.066441427 seconds time elapsed
>> test:
>> 0.075428824 seconds time elapsed
>>
>> Noop:
>> base:
>> 0.057614537 seconds time elapsed
>> test:
>> 0.066061557 seconds time elapsed
>>
>> Netty:
>> base:
>> 0.827013307 seconds time elapsed
>> test:
>> 0.604982805 seconds time elapsed
>>
>> Spring:
>> base:
>> 2.376707358 seconds time elapsed
>> test:
>> 1.927618893 seconds time elapsed
>>
>> The first 2 apps only have 2 to 3 classes in the dynamic archive.
>> So the
>> overhead is likely due to having to open and map the dynamic
>> archive and
>> performs checking on header, etc. For small apps, I think it's
>> better to
>> use a single archive. The Netty app has around 1400 classes in the
>> dynamic archive; the Spring app has about 3700 classes in the dynamic
>> archive.
>>
>> I also used our LotsOfClasses test to collect some perf numbers.
>> This is
>> more like runtime performance, not startup performance.
>>
>> With dynamic archive (100 runs each):
>> real 2m37.191s
>> real 2m36.003s
>> Total loaded classes = 24254
>> Loaded from base archive = 1186
>> Loaded from top archive = 23042
>> Loaded from jrt:/ (runtime module) = 26
>>
>> With single archive (100 runs each):
>> real 2m38.346s
>> real 2m36.947s
>> Total loaded classes = 24254
>> Loaded from archive = 24228
>> Loaded from jrt:/ (runtime module) = 26
>>
>> >
>> > 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?
>> Currently, the default CDS archive contains around 1187 classes. With
>> the -XX:ArchiveClassesAtExit option, if the classes are not found
>> in the
>> default CDS archive, they will be archived in the dynamic
>> archive. The
>> above LotsOfClasses example shows some distributions between various
>> archives.
>> > - 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.
>> As the above numbers indicated, there's not much difference in
>> terms of
>> execution time using a dynamic vs a single archive with a large
>> number
>> of classes loaded. The numbers from Netty and Spring apps show an
>> improvement over default CDS archive.
>> >
>> > 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
>> I did some rough measurements with the LotsOfClasses test with around
>> 15000 classes in the classlist.
>>
>> Dynamic archive dumping (one run each):
>> real 0m19.756s
>> real 0m20.241s
>>
>> Static archive dumping (one run each):
>> real 0m17.725s
>> real 0m16.993s
>> >
>> > 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.
>> Sizes of the archives for the LotsOfClasses test in 1a.
>>
>> Single archive: 242962432
>> Default CDS archive: 12365824
>> Dynamic archive: 197525504
>>
>> >
>> > 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?
>> I used the LotsOfClasses test again. This time archiving about
>> half of
>> the classes which will be loaded during runtime.
>>
>> Dynamic archive (10 runs each):
>> real 0m30.214s
>> real 0m29.633s
>> Loaded classes = 24254
>> Loaded from dynamic archive: 13168
>>
>> Single archive (10 runs each):
>> real 0m32.383s
>> real 0m32.905s
>> Loaded classes = 24254
>> Loaded from single archive = 15063
>> >
>> > 4. Platform support
>> > Which platforms is this supported on?
>> > Which ones did you test? For example, did you run the tests on
>> Windows?
>> I ran the jtreg tests via mach5 on all 4 platforms (Linux, Mac,
>> Solaris,
>> Windows).
>> >
>> > Detailed feedback on the code: Just minor comments - I don’t
>> need to
>> > see an updated webrev:
>> I'm going to look into your detailed feedback below and may reply
>> in a
>> separate email.
>>
>> thanks,
>> Calvin
>> >
>> > 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 <mailto:calvin.cheung at oracle.com>
>> >> <mailto:calvin.cheung at oracle.com
>> <mailto: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/
>> <http://cr.openjdk.java.net/%7Eccheung/8207812_dynamic_cds_archive/webrev.00/>
>> >>
>> <http://cr.openjdk.java.net/%7Eccheung/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
>> >
>>
>
More information about the hotspot-runtime-dev
mailing list