RFR(S) 8218751 Do not store original classfiles inside the CDS archive
Ioi Lam
ioi.lam at oracle.com
Wed Feb 20 04:30:59 UTC 2019
On 2/19/19 5:32 PM, Jiangli Zhou wrote:
> Hi Ioi,
>
> This looks good to me. Thanks for adding the command-line option. The
> option name sounds good to me.
>
> Please see two minor comments/questions below:
>
> - src/hotspot/share/classfile/klassFactory.cpp
>
> The following new code could be eliminated if you want, because the
> name of the path at the path_index should be the same as the one from
> 'cfs' when cfs is not NULL.
> 94 if (cfs != NULL) {
> 95 pathname = cfs->source();
> 96 } else if (path_index < 0) {
The purpose of doing this is to avoid the
(mod_entry->location()->as_C_string()) call below.
> - src/hotspot/share/memory/filemap.cpp
> 507 if (_classpath_entries_for_jvmti != NULL) {
> 508 os::free(_classpath_entries_for_jvmti);
> 509 }
> When _classpath_entries_for_jvmti would not be NULL when we get here?
> How do we handle two archives when base archive + dynamic archive are
> involved? Or, this is already based on the case when the base archive
> is validated before the top layer dynamic archive?
>
This is forward-compatible code -- when the top layer archive is loaded,
this code will be called again, and we may need a longer
_classpath_entries_for_jvmti.
BTW, to clarify one point you raised below when OD is not used:
"Runtime footprint increase (causes more memory fragmentation
even the memory is freed after use)"
This is not true. The decoded classfile data is allocated using
ResourceMark, which uses scoped allocation. There's no fragmentation
after you free the memory.
Thanks
- Ioi
> Thanks,
> Jiangli
>
> On Mon, Feb 18, 2019 at 10:58 PM Ioi Lam <ioi.lam at oracle.com
> <mailto:ioi.lam at oracle.com>> wrote:
>
> Hi Jiangli,
>
> I think you're right that apps that use CFLH but rarely redefine
> classes might see a slight speed up with the OD space. I don't
> know how prevalent such uses cases are, but I won't object to
> making the OD space optional.
>
> So, I added a new flag to enable the OD space. Instead of the name
> you suggested, I used EnableOptionalDataSharedSpace so that it's
> related to other XXXSharedSpace flags. I also added a sanity test
> case, and fixed a Misplaced ResourceMark in klassFactory.cpp
>
> http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02/
> http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v02-delta/
>
> Please let me know what you think.
>
> Thanks
> - Ioi
>
> On 2/18/19 8:12 PM, Jiangli Zhou wrote:
>>
>> On Mon, Feb 18, 2019 at 11:33 AM Ioi Lam <ioi.lam at oracle.com
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>>
>>
>> On 2/15/19 7:11 PM, Jiangli Zhou wrote:
>>> On Fri, Feb 15, 2019 at 6:56 PM Ioi Lam <ioi.lam at oracle.com
>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>
>>> On 2/15/19 4:54 PM, Jiangli Zhou wrote:
>>>> Hi,
>>>>
>>>> To answer your use case question, one of the case
>>>> reported last year in OpenJDK was JRebel (please go
>>>> back to the hotspot-dev mail list 2018 Oct. archive).
>>>> That is an existing example as I've tried to point out
>>>> in my earlier email.
>>>>
>>> First of all, if you read the email carefully, CDS was
>>> not even in their usual test matrix. Also, if you're
>>> modifying classes on the fly, you're slowing down
>>> start-up big time. A few ms spent in decoding the
>>> classfile data will be completely drown out by the
>>> overhead of the classfile parsing and patching code in
>>> the JVMTI agent.
>>>
>>> So no, JRebel will not start up a few % faster if CDS
>>> stores the classfile data. It will most likely be not
>>> measurable.
>>>
>>>
>>> My understanding of the case is that their users generated a
>>> CDS archive and used it together with JRebel. That's why the
>>> issue was unnoticed until their users reported. There are
>>> use cases out there in the community that we simply are not
>>> aware of, so it would not be wise to assume there is no usage.
>>>
>> Just to confirm one thing -- you do not dispute my claim that
>> this "optimization" has no benefits even for those that
>> actually use CDS and CFLH together. Correct?
>>
>>
>> That's actually the part that I have a different opinion. With
>> the proposed change in the current webrev, the performance hit is
>> across the board for loading shared classes regardless if there
>> is a class being redefined/retransformed, as long as the
>> can_generate_all_class_hook_events capability is enabled.
>>
>>
>> We have plenty of nice-to-have harmless optimizations in
>> HotSpot that probably weren't vigorously validated. However,
>> this is not one of them.
>>
>> Here we have clear evidence of harm (50% footprint increase),
>> with no evidence of benefit (theoretical or real-life). This
>> code was checked into HotSpot without proper validation. That
>> was wrong, and that's why I am taking it out now.
>>
>> It's easy to prove me wrong. Just supply a proper benchmark
>> that shows benefits, and I will change my mind. That's the
>> minimal standard for an optimization that has harmful side
>> effects. I have created JDK-8219255 for tracking this.
>>
>>
>> Looks like there is a deadlock in the discussion. To summarize
>> and help move this forward. Here are your reasons for dropping
>> the 'od' space:
>>
>> * static footprint increase due to 'od' space
>> * no benefit
>> * maintenance of the code
>>
>> Regarding static footprint, the change proposed in the current
>> webrev is a short-term and temporary solution. For a long-term
>> solution, the original class files can be removed from the final
>> image created by jlink when jlink can work with CDS to produce a
>> single runtime image.
>>
>> On the benefit side, the 'od' space provides both startup speedup
>> and runtime memory saving (when
>> can_generate_all_class_hook_events capability is enabled), which
>> I've already pointed those out in earlier emails.
>>
>> For maintenance, it is also not an issue with the support from
>> the community (I'll for sure to continue contributing to it).
>> What's more important is to continue keeping CDS/AppCDS moving in
>> the right direction by providing more performance & memory
>> benefits and making it easier to use.
>>
>> So I think providing a command-line option,
>> -XX:EnableOptionalDataSpace that allows user to enable/disable
>> the 'od' space at both dump time and runtime is the best choice
>> that can work with the short-term fix proposed in the current
>> webrev.
>>
>> Thanks,
>> Jiangli
>>
>> Thanks,
>> Jiangli
>>
>>
>> Thanks
>> - Ioi
>>
>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>>> The issue with the current change is that it's only
>>>> targeted for reducing the static footprint (static
>>>> footprint reduction could be achieved with alternative
>>>> approaches such as file compression). The removal of
>>>> the 'od' space and the benefits (both the startup and
>>>> runtime footprint) is not backed up by a clear
>>>> requirement here and the change should not go in as is.
>>>>
>>> As part of the JVM development, we constantly review and
>>> remove unnecessary code to keep the JVM healthy. That's
>>> a clear requirement and mandate on Oracle as the major
>>> contributor to the JDK.
>>>
>>> In this case, we are removing a premature optimization
>>> in the original design. The classfile data stored in the
>>> CDS archive are rarely used (99.99+%), and when they are
>>> used, the speed optimization is irrelevant because the
>>> user of this data (JVMTI agent) is so much slower. So
>>> everyone is taking a footprint hit with no benefit to
>>> anyone.
>>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>>> Just to summarize the drawbacks of the current change:
>>>>
>>>> * Runtime footprint increase (causes more memory
>>>> fragmentation even the memory is freed after use)
>>>> * Startup time regression when
>>>> can_generate_all_class_hook_events capability is
>>>> enabled
>>>> * May cause issue with future optimization
>>>>
>>>> Thanks,
>>>> Jiangli
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, Feb 15, 2019 at 2:07 PM Ioi Lam
>>>> <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>> wrote:
>>>>
>>>> Hi Jiangli,
>>>>
>>>> Thanks for volunteering on this. I think it's best
>>>> to do that (optional support for storing
>>>> uncompressed classfile data into the CDS archive)
>>>> as a separate bug than the current issue.
>>>>
>>>> If you decide to provide a patch for that, I think
>>>> it would be best to find actual users or use cases
>>>> that would find it beneficial. Otherwise it will
>>>> appear to be a solution looking for a problem.
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>>
>>>>
>>>> On 2/15/19 10:35 AM, Jiangli Zhou wrote:
>>>>> I'm willing to continue contributing to the
>>>>> support and maintaining of CDS/AppCDS. Often
>>>>> startup improvements with measurable gain (>4% in
>>>>> this case) are non-trivial. The support for 'od'
>>>>> is not overly complex comparing to the performance
>>>>> gain. In a rosier picture if Jlink/AOT/CDS can
>>>>> work together in harmony to create a single image
>>>>> in the future, class files (and even JAR files)
>>>>> can be eliminated and no class file data can be
>>>>> loaded at runtime from a JAR file. We should
>>>>> consider all aspects and not make a decision lightly.
>>>>>
>>>>> For the dynamic archiving, this is not a blocking
>>>>> issue. I can step in an help with making 'od'
>>>>> optional if needed.
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>> On Thu, Feb 14, 2019 at 7:53 PM Ioi Lam
>>>>> <ioi.lam at oracle.com <mailto:ioi.lam at oracle.com>>
>>>>> wrote:
>>>>>
>>>>> That means we have to add a new -XX option to
>>>>> enable this, and need to add extra test cases
>>>>> and maintenance.
>>>>>
>>>>> We are talking about only single digit %
>>>>> benefits in very narrow use cases, and it's
>>>>> not clear whether performance is important in
>>>>> such cases (or if people would bother to use
>>>>> this -XX flag to gain a few %). Without user
>>>>> input, it's hard to justify keep spending time
>>>>> on an optimization that literally no one had
>>>>> asked for.
>>>>>
>>>>> I just don't think we have time to keep
>>>>> maintaining this. If someone is willing to
>>>>> step up and provide support for this, feel free.
>>>>>
>>>>> Thanks
>>>>>
>>>>> - Ioi
>>>>>
>>>>>
>>>>> On 2/14/19 1:20 PM, Jiangli Zhou wrote:
>>>>>> Based on the issues raised in OpenJDK last
>>>>>> year, there appears to be existing use cases
>>>>>> with AppCDS + CLFH. When designing the
>>>>>> platform, we should think from the user
>>>>>> perspective. Making 'od' optional is not
>>>>>> complex and gives the control to the Java
>>>>>> users. It also avoids potential waste of
>>>>>> effort for removing&putting back the
>>>>>> optimization.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>>>> On Thu, Feb 14, 2019 at 11:46 AM Ioi Lam
>>>>>> <ioi.lam at oracle.com
>>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>
>>>>>> Such optional support increase the
>>>>>> complexity of the VM. My preference would
>>>>>> be to wait until there's an actual need
>>>>>> for this.
>>>>>>
>>>>>> The JDK is release under 6 months
>>>>>> cadence, so if the removal turns out to
>>>>>> be a bad idea, we can reinstate the code
>>>>>> in the next release. Or, someone can just
>>>>>> make their own JDK build with a simple
>>>>>> anti-delta of this patch.
>>>>>>
>>>>>> If we are too reluctant to remove
>>>>>> anything, the JDK will eventually become
>>>>>> a hopeless mess.
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> - Ioi
>>>>>>
>>>>>> On 2/14/19 10:43 AM, Jiangli Zhou wrote:
>>>>>>> Sorry for the slow turnaround. Hopefully
>>>>>>> it will get better after this week. As
>>>>>>> there is no enough user data/requirement
>>>>>>> to determine which optimization
>>>>>>> direction is more important in this
>>>>>>> case, it might be reasonable to make the
>>>>>>> 'optional data' space truly optional,
>>>>>>> which can be controlled by a
>>>>>>> command-line option. If the performance
>>>>>>> is more important (in case
>>>>>>> should_post_class_file_load_hook is
>>>>>>> required for a particular use case),
>>>>>>> user can enable dumping out the 'od'
>>>>>>> space with archived class file data,
>>>>>>> otherwise the data can be loaded at
>>>>>>> runtime. What's your thought on this?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>>>>>>
>>>>>>> On Wed, Feb 13, 2019 at 12:03 PM Ioi Lam
>>>>>>> <ioi.lam at oracle.com
>>>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2/13/19 11:24 AM, Jiangli Zhou wrote:
>>>>>>>> Hi Ioi,
>>>>>>>>
>>>>>>>> Thanks for the additional
>>>>>>>> information and performance data.
>>>>>>>> Please see more comments below.
>>>>>>>>
>>>>>>>> On Wed, Feb 13, 2019 at 5:33 AM Ioi
>>>>>>>> Lam <ioi.lam at oracle.com
>>>>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>>>
>>>>>>>> Hi Jiangli,
>>>>>>>>
>>>>>>>> The main reason for doing this
>>>>>>>> is to reduce the size of the
>>>>>>>> CDS file.
>>>>>>>> For example, when archiving
>>>>>>>> Eclipse IDE with JDK-8215311
>>>>>>>> (Dynamic Class
>>>>>>>> Metadata Archive), the CDS
>>>>>>>> archive is reduced from 100MB
>>>>>>>> to about 67MB.
>>>>>>>>
>>>>>>>> We have not heard any
>>>>>>>> requirement for high
>>>>>>>> performance with
>>>>>>>> CDS+ClassFileLoadHook. It
>>>>>>>> doesn't seem right for everyone
>>>>>>>> to take a 50%
>>>>>>>> file size penalty for an
>>>>>>>> optimization that no one has
>>>>>>>> asked for.
>>>>>>>>
>>>>>>>>
>>>>>>>> Jut to clarify, it is ~30% not 50%
>>>>>>>> (possibly a typo?), correct?
>>>>>>>
>>>>>>>
>>>>>>> Depending on how you look at it.
>>>>>>> Going from 67MB to 100MB is an
>>>>>>> increase of 33MB which is 50%.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Here are some perf numbers
>>>>>>>> (running "java -version" with
>>>>>>>> an agent that
>>>>>>>> installs a CFLH that doesn't
>>>>>>>> nothing). Yes, it's somewhat
>>>>>>>> slower as
>>>>>>>> expected, but it doesn't seem
>>>>>>>> to be catastrophic. Using CDS is
>>>>>>>> nevertheless much faster than
>>>>>>>> without CDS anyway.
>>>>>>>>
>>>>>>>> After No CFLH 0.0476 seconds
>>>>>>>> time elapsed ( +- 0.20% )
>>>>>>>> After with CFLH 0.0513 seconds
>>>>>>>> time elapsed ( +- 0.18% ) 7.773%
>>>>>>>> slower
>>>>>>>>
>>>>>>>> Before no CFLH 0.0472 seconds
>>>>>>>> time elapsed ( +- 0.21% )
>>>>>>>> Before with CFLH 0.0492 seconds
>>>>>>>> time elapsed ( +- 0.18% ) 4.237%
>>>>>>>> slower
>>>>>>>>
>>>>>>>>
>>>>>>>> Comparing 'After with CFLH' (0.0513
>>>>>>>> seconds) with 'Before with CFLH'
>>>>>>>> (0.0492 seconds), there is about
>>>>>>>> 4.27% performance degradation
>>>>>>>> observed from your data.
>>>>>>>>
>>>>>>>> Another disadvantage of this
>>>>>>>> proposed change is the increasing
>>>>>>>> of runtime memory. When the class
>>>>>>>> file data is stored in the shared
>>>>>>>> archive file and mapped as RO
>>>>>>>> (read-only), it can be shared among
>>>>>>>> multiple JVM processes. For the
>>>>>>>> default CDS archive case, it's
>>>>>>>> shared by all JVM processes running
>>>>>>>> simultaneously. With the proposed
>>>>>>>> change in the webrev, the memory
>>>>>>>> for the class file data is
>>>>>>>> allocated at runtime. It moves the
>>>>>>>> cost from static footprint to
>>>>>>>> runtime memory footprint, which is
>>>>>>>> a higher price because it needs to
>>>>>>>> be multiplied by the number of
>>>>>>>> running JVM processes.
>>>>>>>
>>>>>>>
>>>>>>> With this patch, the decoded
>>>>>>> classdata is freed after the CFLH is
>>>>>>> called, so there's no resident
>>>>>>> memory cost.
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> CDS Off with CFLH 0.0869
>>>>>>>> seconds time elapsed ( +- 0.18% )
>>>>>>>> CDS Off without CFLH 0.0852
>>>>>>>> seconds time elapsed ( +- 0.14%
>>>>>>>> ) 1.995%
>>>>>>>> slower
>>>>>>>>
>>>>>>>> I am not sure if there's a
>>>>>>>> performance critical case for CFLH.
>>>>>>>>
>>>>>>>>
>>>>>>>> I have the similar question. It
>>>>>>>> would be good to go back and
>>>>>>>> re-investigate the original
>>>>>>>> requirements for supporting CFLH
>>>>>>>> use cases.
>>>>>>>
>>>>>>>
>>>>>>> I think when we added CFLH support
>>>>>>> for CDS, the goal was "to not lose
>>>>>>> all benefit of CDS when CFLH is
>>>>>>> enabled". I think we still achieve
>>>>>>> that after this patch. There really
>>>>>>> was no requirement "to make CFLH
>>>>>>> blazing fast at all cost".
>>>>>>>
>>>>>>> Thanks
>>>>>>>
>>>>>>> - Ioi
>>>>>>>
>>>>>>>> I suppose
>>>>>>>> if performance is critical you
>>>>>>>> would rewrite the classes
>>>>>>>> statically and
>>>>>>>> rebuild your app, instead of
>>>>>>>> patching its bytecodes at
>>>>>>>> runtime. However,
>>>>>>>> if there were indeed a
>>>>>>>> performance critical case, I
>>>>>>>> think it's better to
>>>>>>>> change JVMTI to allow a 2-level
>>>>>>>> filtering for CFLH:
>>>>>>>>
>>>>>>>> I believe the overwhelming use
>>>>>>>> case, where performance is
>>>>>>>> critical, the
>>>>>>>> CFLH will just patch a small
>>>>>>>> number of class files. I can't
>>>>>>>> fathom any
>>>>>>>> use case where someone wants to
>>>>>>>> patch EVERY loaded class.
>>>>>>>>
>>>>>>>> The current CFLH passes both
>>>>>>>> the name of the class, as well
>>>>>>>> as the
>>>>>>>> classfile data, in a single
>>>>>>>> call. This forces CDS to decode
>>>>>>>> the
>>>>>>>> classfile data for every hook
>>>>>>>> call. However, in most cases,
>>>>>>>> the CFLH
>>>>>>>> will just examine the class
>>>>>>>> name, and do nothing unless the
>>>>>>>> name matches
>>>>>>>> a certain pattern, so we end up
>>>>>>>> wasting the decoding effort.
>>>>>>>>
>>>>>>>> My suggested improvement is to
>>>>>>>> add a new filtering call in
>>>>>>>> JVMTI that
>>>>>>>> passes only the name. If the
>>>>>>>> CFLH wants to patch the class,
>>>>>>>> it will then
>>>>>>>> request the classfile data, at
>>>>>>>> which point CDS will decode it
>>>>>>>> from the
>>>>>>>> modules file or JAR file.
>>>>>>>>
>>>>>>>>
>>>>>>>> That probably is the right direction.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jiangli
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>>> - Ioi
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/12/19 5:18 PM, Jiangli
>>>>>>>> Zhou wrote:
>>>>>>>> > Hi Ioi,
>>>>>>>> >
>>>>>>>> > I'd like to understand the
>>>>>>>> performance impact with this
>>>>>>>> change. Do you have
>>>>>>>> > any performance numbers when
>>>>>>>> JvmtiExport::should_post_class_file_load_hook()
>>>>>>>> > is required? This is a
>>>>>>>> performance vs footprint
>>>>>>>> trade-off. For some users,
>>>>>>>> > performance is more important
>>>>>>>> than static footprint.
>>>>>>>> >
>>>>>>>> > Could you also please provide
>>>>>>>> some background/motivation for
>>>>>>>> this change?
>>>>>>>> >
>>>>>>>> > Thanks,
>>>>>>>> > Jiangli
>>>>>>>> >
>>>>>>>> > On Mon, Feb 11, 2019 at 9:24
>>>>>>>> AM Ioi Lam <ioi.lam at oracle.com
>>>>>>>> <mailto:ioi.lam at oracle.com>> wrote:
>>>>>>>> >
>>>>>>>> >>
>>>>>>>> http://cr.openjdk.java.net/~iklam/jdk13/8218751-dont-store-classfiles-in-cds.v01/
>>>>>>>> >>
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8218751
>>>>>>>> >>
>>>>>>>> >> For JVMTI ClassFileLoadHook
>>>>>>>> support, the CDS archive
>>>>>>>> currently stores
>>>>>>>> >> the original classfile data
>>>>>>>> of all archived classes.
>>>>>>>> >>
>>>>>>>> >> However, this consists of
>>>>>>>> over 30% of the archive size.
>>>>>>>> Because all
>>>>>>>> >> original classfile data are
>>>>>>>> already available in other
>>>>>>>> files (such as the
>>>>>>>> >> JDK lib/modules file, or JAR
>>>>>>>> files in the classpath), we can
>>>>>>>> simply read
>>>>>>>> >> from these locations when
>>>>>>>> needed by JVMTI.
>>>>>>>> >>
>>>>>>>> >> For the default CDS archive
>>>>>>>> (included as part of the JDK
>>>>>>>> distribution),
>>>>>>>> >> the size is reduced from
>>>>>>>> about 18.5MB to 12.1MB on
>>>>>>>> Linux/x64.
>>>>>>>> >>
>>>>>>>> >> Thanks
>>>>>>>> >>
>>>>>>>> >> - Ioi
>>>>>>>> >>
>>>>>>>> >>
>>>>>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list