RFR(S) 8218751 Do not store original classfiles inside the CDS archive
Ioi Lam
ioi.lam at oracle.com
Wed Feb 20 18:20:42 UTC 2019
On 2/20/19 9:24 AM, Calvin Cheung wrote:
> Hi Ioi,
>
> The updated webrev looks good.
>
> Is a CSR required for the new vm option?
I think so. I am not sure if we need to validate the benefits of a flag
before adding it. I've asked Joe Darcy and Karen Kinnear about this and
waiting for their answer.
https://wiki.openjdk.java.net/display/HotSpot/Hotspot+Command-line+Flags%3A+Kinds%2C+Lifecycle+and+the+CSR+Process
>
> What would be the output of the following if
> EnableOptionalDataSharedSpace isn't enabled?
> _od_region.print(total_reserved);
>
The print out looks like this:
mc space: 22288 [ 0.2% of total] out of 24576 bytes [ 90.7%
used] at 0x0000000800000000
rw space: 4318864 [ 34.6% of total] out of 4321280 bytes [ 99.9%
used] at 0x0000000800006000
ro space: 7360800 [ 59.0% of total] out of 7364608 bytes [ 99.9%
used] at 0x0000000800425000
md space: 2512 [ 0.0% of total] out of 4096 bytes [ 61.3%
used] at 0x0000000800b2b000
od space: 0 [ 0.0% of total] out of 0 bytes [ 0.0%
used] at 0x0000000800b2c000
ca0 space: 458752 [ 3.7% of total] out of 458752 bytes [100.0%
used] at 0x00000007bfc00000
oa0 space: 294912 [ 2.4% of total] out of 294912 bytes [100.0%
used] at 0x00000007bf800000
total : 12458128 [100.0% of total] out of 12468224 bytes [ 99.9% used]
Thanks
- Ioi
> 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