RFR [9] 8137058: Clear out all non-Critical APIs from sun.reflect and move to jdk.unsupported

[Kevin Rushforth]

Hi,


Chris Hegarty wrote:
> Mandy,
>
> On 13/04/16 18:15, Mandy Chung wrote:
>>
>>> On Apr 13, 2016, at 8:43 AM, Chris Hegarty 
>>> <chris.hegarty at oracle.com> wrote:
>>>
>>> This review is for the second significant part of the changes for JEP
>>> 260 [1]. When created, the jdk.unsupported module [2] initially
>>> contained the sun.misc package. This issue is will move all the
>>> non-Critical APIs out of sun.reflect, leaving only the critical 
>>> ones, at
>>> which point sun.reflect can be moved to the jdk.unsupported module.
>>>
>>> http://cr.openjdk.java.net/~chegar/8137058/
>>> https://bugs.openjdk.java.net/browse/JDK-8137058
>>>
>>> Summary of the changes:
>>>
>>> - Move all existing sun.reflect types to jdk.internal.reflect, and
>>> fix up references in the libraries, and the VM, to use the new internal
>>> location.
>>
>> I hope the getCallerClass(int depth) method is moved out of 
>> jdk.internal.reflect.Reflection. JDK is not using it and it’s 
>> retained for existing libraries to migrate to the new StackWalker 
>> API.  Of course the cost is to add a native library and the build 
>> infra work has made this straight-forward.
>
> In my patch jdk.internal.reflect.Reflection.getCallerClass(int)
> is retained only to avoid the need for an unsupported.so, but
> if you feel strongly that is should go, then I can make the
> change.
>
>> I agree with Alan that the @deprecated javadoc of 
>> sun.reflect.Reflection::getCallerClass should be updated to make it 
>> clear.  What about:
>>
>> /**
>>   * @deprecated This method is an internal API and will be removed in 
>> JDK 10.
>>   * Use {@link StackWalker} to walk the stack and obtain the caller 
>> class
>>   * with {@link StackWalker.StackFrame#getDeclaringClass} instead.
>>   */
>
> That seems reasonable. The version number should be present
> in the @Deprecated forRemoval element too.  I'll make the change.
>
>> This patch will likely impact existing libraries that filter out 
>> reflection frames (IIRC Groovy and log4j may be examples) doing 
>> Class::getName().startsWith(“sun.reflect”).  It may worth call out 
>> this incompatibility in JEP 260.
>
> I'll add a note.
>
>> StackStreamFactory.java shows an example:
>>
>> 1085         if (c.getName().startsWith("sun.reflect") &&
>>
>> This should be changed to “jdk.internal.reflect”.
>
> Ah I missed this. Strangely all tests are passing without
> this change. I'll make the change.
>
>> test/java/lang/StackWalker/EmbeddedStackWalkTest.java and maybe a few 
>> other stack walker tests.  You may want to check any other of any 
>> similar pattern in the JDK code/tests (I think I got them all)
>
> I did update some StackWalker tests, but missed this one ( it
> passes for me ).  I'll check all of them.
>
>> The hotspot change drops the package name prefix in javaClasses.hpp 
>> which is inconsistent with other classes.  I found including 
>> jdk_internal_reflect_ in the class name is useful.    Coleen and 
>> others from the hotspot team may have opinion on this.
>
> Ok.
>
>> FX will need to adjust for this patch (cc’ing Kevin to prepare for this)
>
> Ah ok, thanks for copying Kevin.

Yes, thanks Mandy. JavaFX uses sun.reflect.CallerSensitive and 
sun.reflect.Reflection (both in FXML) so I will file an FX bug to track 
the need to fix the module dependences.

-- Kevin

> I'll send a note to the list once the webrev is updated.
>
> -Chris.