RFR(S) 8218751 Do not store original classfiles inside the CDS archive

[Jiangli Zhou]

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> 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?


>
> 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.


> 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 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> 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
> >>
> >>
>