[7u12] Request for approval for CR 8000999: backport of JSR 292 to 7u

[Andrew Hughes]

----- Original Message -----
> 
> On Oct 18, 2012, at 6:48 PM, Seán Coffey <sean.coffey at oracle.com>
> wrote:
> 
> > Christian,
> > 
> > It is quite a big code change for 7u, one worthy of more testing
> > before integration perhaps. Doesn't this code introduce new
> > dependencies on the VM code ?  How are changes progressing for
> > that ? Does hsx24 need to be in jdk7u-dev first ?
> 
> hsx24 already has the changes that are required for the new JSR 292
> implementation.  In other words:  the JDK changes are required
> otherwise jdk7u doesn't work when hsx24 is integrated.
> 
> > 
> > Have the proposed backports been reviewed for jdk7u ?  Even though
> > the fixes are more or less identical to jdk8, I think it's best to
> > get peer reviews in the context of such code fitting into jdk7u.
> 
> I didn't get reviews yet and I'm not sure anyone will (given the size
> of the patch).
> 
> > 
> > Why won't you backport the changes in separate changesets ? This
> > has been common practice in 7u and it makes tracing of code
> > changes easier to analyze. This change is a bulk change for jdk7u
> > [1] - something I can't recall seeing for the jdk repo in the
> > past.
> 
> I tried.  A changeset-by-changeset backport was non trivial and the
> deadline for PIT testing came closer so we decided to do it as one
> changeset.
> 
> In the end the code is the same anyway because we need exactly the
> same classes (except the small diff I posted) in 7u as we have in 8.
> 
> If we are willing to postpone to next week's PIT I can bring the
> original changesets over.  And if that means no reviews and quick
> approvals then it might be worth it.
> 

Please delay.  I don't see the need for the rush and I'd rather all of us
didn't have to deal with this humongous changeset.

> -- Chris
> 
> > 
> > I understand some more PIT testing is going to be run - Let's see
> > how test results look before proceeding.
> > 
> > regards,
> > Sean.
> > 
> > [1] http://openjdk.java.net/projects/jdk7u/bulkchanges.html
> > 
> > On 18/10/2012 12:22, Christian Thalinger wrote:
> >> On Oct 18, 2012, at 12:17 PM, Christian Thalinger
> >> <christian.thalinger at oracle.com> wrote:
> >> 
> >>> On Oct 18, 2012, at 12:00 PM, mark.reinhold at oracle.com wrote:
> >>> 
> >>>> 2012/10/18 11:54 -0700, christian.thalinger at oracle.com:
> >>>>> Webrev: http://cr.openjdk.java.net/~twisti/8000999/
> >>>>> 8000999: backport of JSR 292 to 7u
> >>>>> 
> >>>>> This is an umbrella bug for these changes (which are backported
> >>>>> in one
> >>>>> changeset):
> >>>>> 
> >>>>> ...
> >>>> Um, isn't this an awfully big change for what's supposed to be a
> >>>> low-risk
> >>>> update release?
> >>> It's big, indeed.  But actually it's a bug fix and it only
> >>> touches JSR 292 related files and functionality.
> >>> 
> >>> Since PR decided that HS24 will be used for 7u12 (which I applaud
> >>> to) we need these changes in 7.  Otherwise it doesn't work.
> >> I forgot to add the diff between 7 and 8 after this change goes
> >> in.  There is also a review request on hotspot-dev.
> >> 
> >> -- Chris
> >> 
> >> The backport is just copying over the files from JDK 8.  That's
> >> why the webrev is so big and pretty useless.  The real changes
> >> between 8 and 7 are these:
> >> 
> >> diff -Nur
> >> jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java
> >> jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java
> >> ---
> >> jdk8/src/share/classes/java/lang/invoke/MethodHandleStatics.java
> >>    2012-10-15 12:21:52.806052959 -0700
> >> +++
> >> jdk7u/src/share/classes/java/lang/invoke/MethodHandleStatics.java
> >>   2012-10-16 10:48:29.728257304 -0700
> >> @@ -94,10 +94,14 @@
> >> 
> >>     // handy shared exception makers (they simplify the common
> >>     case code)
> >>     /*non-public*/ static InternalError newInternalError(String
> >>     message, Throwable cause) {
> >> -        return new InternalError(message, cause);
> >> +        InternalError e = new InternalError(message);
> >> +        e.initCause(cause);
> >> +        return e;
> >>     }
> >>     /*non-public*/ static InternalError newInternalError(Throwable
> >>     cause) {
> >> -        return new InternalError(cause);
> >> +        InternalError e = new InternalError();
> >> +        e.initCause(cause);
> >> +        return e;
> >>     }
> >>     /*non-public*/ static RuntimeException
> >>     newIllegalStateException(String message) {
> >>         return new IllegalStateException(message);
> >> diff -Nur
> >> jdk8/src/share/classes/sun/invoke/util/ValueConversions.java
> >> jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java
> >> --- jdk8/src/share/classes/sun/invoke/util/ValueConversions.java
> >>        2012-10-16 10:49:36.081911283 -0700
> >> +++ jdk7u/src/share/classes/sun/invoke/util/ValueConversions.java
> >>       2012-10-16 10:48:19.626424849 -0700
> >> @@ -1211,9 +1211,13 @@
> >> 
> >>     // handy shared exception makers (they simplify the common
> >>     case code)
> >>     private static InternalError newInternalError(String message,
> >>     Throwable cause) {
> >> -        return new InternalError(message, cause);
> >> +        InternalError e = new InternalError(message);
> >> +        e.initCause(cause);
> >> +        return e;
> >>     }
> >>     private static InternalError newInternalError(Throwable cause)
> >>     {
> >> -        return new InternalError(cause);
> >> +        InternalError e = new InternalError();
> >> +        e.initCause(cause);
> >> +        return e;
> >>     }
> >> }
> >> diff --git a/src/share/classes/sun/misc/Unsafe.java
> >> b/src/share/classes/sun/misc/Unsafe.java
> >> --- a/src/share/classes/sun/misc/Unsafe.java
> >> +++ b/src/share/classes/sun/misc/Unsafe.java
> >> @@ -678,6 +678,14 @@
> >>     public native Object staticFieldBase(Field f);
> >> 
> >>     /**
> >> +     * Detect if the given class may need to be initialized. This
> >> is often
> >> +     * needed in conjunction with obtaining the static field base
> >> of a
> >> +     * class.
> >> +     * @return false only if a call to {@code
> >> ensureClassInitialized} would have no effect
> >> +     */
> >> +    public native boolean shouldBeInitialized(Class c);
> >> +
> >> +    /**
> >>      * Ensure the given class has been initialized. This is often
> >>      * needed in conjunction with obtaining the static field base
> >>      of a
> >>      * class.
> >> 
> > 
> 
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: 248BDC07 (https://keys.indymedia.org/)
Fingerprint = EC5A 1F5E C0AD 1D15 8F1F  8F91 3B96 A578 248B DC07