RFR(S) 8218751 Do not store original classfiles inside the CDS archive
Calvin Cheung
calvin.cheung at oracle.com
Wed Feb 20 17:24:51 UTC 2019
Hi Ioi,
The updated webrev looks good.
Is a CSR required for the new vm option?
What would be the output of the following if
EnableOptionalDataSharedSpace isn't enabled?
_od_region.print(total_reserved);
thanks,
Calvin
On 2/18/19, 10:58 PM, Ioi Lam 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