[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-hotspot-runtime-dev
Subject: Re: RFR(S): 8075253: Multiversion JAR feature: CDS does not support MV-JARs
From: Jiangli Zhou <jiangli.zhou () oracle ! com>
Date: 2016-03-31 15:05:02
Message-ID: B1344FCD-4AF8-4E1A-B593-94D44D1CA375 () oracle ! com
[Download RAW message or body]
Hi Calvin,
The update looks good.
Thanks,
Jiangli
> On Mar 30, 2016, at 10:31 PM, Calvin Cheung <calvin.cheung@oracle.com> wrote:
>
> Hi Jiangli,
>
> On 3/30/16, 6:06 PM, Jiangli Zhou wrote:
> > Hi Calvin,
> >
> > Looks good.
> Thanks for your quick review.
> > I have a minor comment. I would suggest to rename ‘_is_boot_entry' to \
> > ‘_is_boot_append' or ‘_is_boot_append_entry'.
> I've picked '_is_boot_append'.
> Updated webrev:
> http://cr.openjdk.java.net/~ccheung/8075253/webrev.03/
>
> Just the changes between webrev.02 and 03:
> http://cr.openjdk.java.net/~ccheung/8075253/webrev.02-03/
>
> thanks,
> Calvin
> >
> > Thanks,
> > Jiangli
> >
> > > On Mar 30, 2016, at 5:07 PM, Calvin Cheung<calvin.cheung@oracle.com> wrote:
> > >
> > > Hi,
> > >
> > > I needed to update the webrev to handle the -Xbootclasspath/a.
> > > To be consistent with non-CDS case, the versioned entries in a multi-release \
> > > jar will be ignored if the jar is found in the -Xbootclasspath/a. Summary of \
> > > changes:
> > > - added a bool _is_boot_entry to the ClassPathZipEntry;
> > > - added a bool parameter to the functions (create_class_path_entry, \
> > > create_class_path_zip_entry, etc.) which creates a \
> > > ClassPathZipEntry;
> > > - open_versioned_entry and is_multiple_versioned are CDS only functions.
> > >
> > > webrev: http://cr.openjdk.java.net/~ccheung/8075253/webrev.02/
> > >
> > > Testing:
> > > JPRT
> > > RBT tests on hotspot_runtime
> > >
> > > thanks,
> > > Calvin
> > >
> > > On 3/25/16, 6:42 AM, Coleen Phillimore wrote:
> > > > Hi Calvin,
> > > > I didn't really review this but could you add a // INCLUDE_CDS on the #endif \
> > > > in this code.
> > > > http://cr.openjdk.java.net/~ccheung/8075253/webrev.01/src/share/vm/classfile/classLoader.cpp.udiff.html
> > > >
> > > > Thanks,
> > > > Coleen
> > > >
> > > > On 3/24/16 8:40 PM, Calvin Cheung wrote:
> > > > > Hi Jiangli,
> > > > >
> > > > > I agreed with both of your comments.
> > > > >
> > > > > On 3/23/16, 4:24 PM, Jiangli Zhou wrote:
> > > > > > Hi Calvin,
> > > > > >
> > > > > > I have two comments below.
> > > > > >
> > > > > > - I'd suggest putting the new block of code (added in open_stream()) in a \
> > > > > > separate function. The new function should be defined for CDS_ONLY.
> > > > > > - In the bug report, it is suggested that the search for versioned class \
> > > > > > stops at 8. The changes in the webrev search all versions down to 6. \
> > > > > > Could you please verify what the spec says. The VM behavior should be the \
> > > > > > same a the JDK library code.
> > > > > I've checked that the base version is set to 8 in java.util.jar.JarFile.
> > > > >
> > > > > Here's an updated webrev: \
> > > > > http://cr.openjdk.java.net/~ccheung/8075253/webrev.01/
> > > > > thanks,
> > > > > Calvin
> > > > >
> > > > > > Thanks,
> > > > > > Jiangli
> > > > > >
> > > > > > > On Mar 18, 2016, at 1:51 PM, Calvin Cheung<calvin.cheung@oracle.com> \
> > > > > > > wrote:
> > > > > > >
> > > > > > > This fix was reviewed in Aug 2015[1] though most of the review comments \
> > > > > > > was via internal mailing list. Recently, the JEP 238 (Multiple-Release \
> > > > > > > jar files) has been checked into jdk9. So it is time to make the \
> > > > > > > corresponding changes in hotspot.
> > > > > > > JBS: bug: https://bugs.openjdk.java.net/browse/JDK-8075253
> > > > > > > (unfortunately the bug was marked as "confidential")
> > > > > > >
> > > > > > > webrev: http://cr.openjdk.java.net/~ccheung/8075253/webrev.00/
> > > > > > >
> > > > > > > Some adjustments need to be made due to:
> > > > > > > - attribute name in the jar manifest has been changed to \
> > > > > > > "Multi-release";
> > > > > > > - system property has been changed to jdk.util.jar.enableMultiRelease \
> > > > > > > and it has value of "true", "force" or "false".
> > > > > > > The diff between this patch and the reviewed patch is as follows:
> > > > > > >
> > > > > > > 11c11
> > > > > > > < + const char* multi_ver = \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > \
> > > > > > > ---
> > > > > > > > + const char* multi_ver = \
> > > > > > > > Arguments::get_property("jdk.util.jar.multiversion");
> > > > > > > 14c14
> > > > > > > < + strcmp(multi_ver, "true") == 0 ||
> > > > > > > ---
> > > > > > > > + strcmp(multi_ver, "enable") == 0 ||
> > > > > > > 64c64
> > > > > > > < @@ -296,6 +345,17 @@
> > > > > > > ---
> > > > > > > > @@ -272,6 +321,17 @@
> > > > > > > 72c72
> > > > > > > < + if (strstr(buffer, "Multi-Release: true") != NULL) {
> > > > > > > ---
> > > > > > > > + if (strstr(buffer, "Multiversion: true") != NULL) {
> > > > > > > thanks,
> > > > > > > Calvin
> > > > > > >
> > > > > > > [1]: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2015-August/015589.html
> > > > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic