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

[Christian Thalinger]

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.

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