RFR(S) 8218751 Do not store original classfiles inside the CDS archive
Ioi Lam
ioi.lam at oracle.com
Mon Feb 18 19:34:11 UTC 2019
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?
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.
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