RFR(S) 8218751 Do not store original classfiles inside the CDS archive
Ioi Lam
ioi.lam at oracle.com
Tue Feb 19 06:58:42 UTC 2019
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