RFR [9] 8137058: Clear out all non-Critical APIs from sun.reflect and move to jdk.unsupported
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. - Update tests, as appropriate, to use the new location. - Add the minimal set of critical APIs to jdk.unsupported/sun.reflect. These ultimately delegate to the internal versions. Additionally, a few new tests have been added to exercise these. -Chris. [1] https://bugs.openjdk.java.net/browse/JDK-8132928 [2] https://bugs.openjdk.java.net/browse/JDK-8153737
On 13/04/2016 16:43, Chris Hegarty 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
This looks good. A few comments: I assume the new sun.reflect.Reflection should have a private constructor to prevent it being instantiated. You've probably thought about this already but I assume the @Deprecated in Reflection.getCallerClass should reference the supported API. The changes to InetAddress don't seem unrelated but good. -Alan.
On 13/04/16 16:59, Alan Bateman wrote:
On 13/04/2016 16:43, Chris Hegarty 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
This looks good. A few comments:
I assume the new sun.reflect.Reflection should have a private constructor to prevent it being instantiated.
D'oh! of course. Updated in-place. http://cr.openjdk.java.net/~chegar/8137058/jdk/src/jdk.unsupported/share/cla...
You've probably thought about this already but I assume the @Deprecated in Reflection.getCallerClass should reference the supported API.
If it is ok, I'd like to address that separately. I think there is probably more we can do there, and it probably deserves its own discussion.
The changes to InetAddress don't seem unrelated but good.
They were sitting in my repo. Just some minor reformatting. -Chris.
On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty@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. 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. */ 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. StackStreamFactory.java shows an example: 1085 if (c.getName().startsWith("sun.reflect") && This should be changed to “jdk.internal.reflect”. 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) 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. FX will need to adjust for this patch (cc’ing Kevin to prepare for this) Mandy
Mandy, On 13/04/16 18:15, Mandy Chung wrote:
On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty@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. I'll send a note to the list once the webrev is updated. -Chris.
On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
Mandy,
On 13/04/16 18:15, Mandy Chung wrote:
On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty@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’m less concerned of a native library but its name led me to have a second thought. I can leave with keeping jdk.internal.reflect.Reflection::getCallerClass(int depth) for another reason.
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.
Thanks.
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.
This is just like an assertion that should never reach there. It throws an internal error if a class starts with sun.reflect but not a reflective method.
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.
It may be a check with several conditions or assertion like the above. Mandy
Since getCallerClass will be removed in 10, @Deprecated should also have its condemned=true Cheers, Paul On Wed, Apr 13, 2016 at 12:43 PM, Mandy Chung <mandy.chung@oracle.com> wrote:
On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
Mandy,
On 13/04/16 18:15, Mandy Chung wrote:
On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty@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’m less concerned of a native library but its name led me to have a second thought. I can leave with keeping jdk.internal.reflect.Reflection::getCallerClass(int depth) for another reason.
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.
Thanks.
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.
This is just like an assertion that should never reach there. It throws an internal error if a class starts with sun.reflect but not a reflective method.
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.
It may be a check with several conditions or assertion like the above.
Mandy
On 13 Apr 2016, at 19:03, Paul Benedict <pbenedict@apache.org> wrote:
Since getCallerClass will be removed in 10, @Deprecated should also have its condemned=true
`condemned` was renamed to `forRemoval` [1]. getCallerClass, in fact the whole class, will have `forRemoval=true`. -Chris. [1] http://mail.openjdk.java.net/pipermail/jdk9-dev/2016-April/003932.html
Cheers, Paul
On Wed, Apr 13, 2016 at 12:43 PM, Mandy Chung <mandy.chung@oracle.com <mailto:mandy.chung@oracle.com>> wrote:
On Apr 13, 2016, at 10:28 AM, Chris Hegarty <chris.hegarty@oracle.com <mailto:chris.hegarty@oracle.com>> wrote:
Mandy,
On 13/04/16 18:15, Mandy Chung wrote:
On Apr 13, 2016, at 8:43 AM, Chris Hegarty <chris.hegarty@oracle.com <mailto:chris.hegarty@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/ <http://cr.openjdk.java.net/~chegar/8137058/> https://bugs.openjdk.java.net/browse/JDK-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’m less concerned of a native library but its name led me to have a second thought. I can leave with keeping jdk.internal.reflect.Reflection::getCallerClass(int depth) for another reason.
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.
Thanks.
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.
This is just like an assertion that should never reach there. It throws an internal error if a class starts with sun.reflect but not a reflective method.
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.
It may be a check with several conditions or assertion like the above.
Mandy
Mandy, The webrev has been updated in-place http://cr.openjdk.java.net/~chegar/8137058/ http://cr.openjdk.java.net/~chegar/8137058/jdk_incremental.diffs All 'core', 'pit', and 'hotspot' testsets have been successfully run on Mac, Linux, Windows, and Solaris. On 13 Apr 2016, at 18:43, Mandy Chung <mandy.chung@oracle.com> wrote:
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 added the following note to the Risks and Assumptions section of JEP 260: Beyond the proposed critical APIs for `sun.reflect`, said package contains the machinery that implements the `java.lang(.reflect)` subsystem. That machinery will be moved to an internal, non-exported, package in the base module. Consequently, the stack trace of reflective calls will appear somewhat different. That is, stack frames that represent the reflective implementation will see their class name ( `StackTraceElement.getClassName()` ) change from `sun.reflect.XXX` to `jdk.internal.reflect.XXX`. Any code analysing, or filtering, based on the stack trace element's class name should be updated appropriately, to handle this. See [8137058](https://bugs.openjdk.java.net/browse/JDK-8137058) for further details. -Chris.
On Apr 14, 2016, at 10:38 AM, Chris Hegarty <chris.hegarty@oracle.com> wrote:
Mandy,
The webrev has been updated in-place http://cr.openjdk.java.net/~chegar/8137058/ http://cr.openjdk.java.net/~chegar/8137058/jdk_incremental.diffs
Looks good. Thanks for making the change.
All 'core', 'pit', and 'hotspot' testsets have been successfully run on Mac, Linux, Windows, and Solaris.
On 13 Apr 2016, at 18:43, Mandy Chung <mandy.chung@oracle.com> wrote:
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 added the following note to the Risks and Assumptions section of JEP 260:
Beyond the proposed critical APIs for `sun.reflect`, said package contains the machinery that implements the `java.lang(.reflect)` subsystem. That machinery will be moved to an internal, non-exported, package in the base module. Consequently, the stack trace of reflective calls will appear somewhat different. That is, stack frames that represent the reflective implementation will see their class name ( `StackTraceElement.getClassName()` ) change from `sun.reflect.XXX` to `jdk.internal.reflect.XXX`. Any code analysing, or filtering, based on the stack trace element's class name should be updated appropriately, to handle this. See [8137058](https://bugs.openjdk.java.net/browse/JDK-8137058) for further details.
Thanks for this. Mandy
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@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.
Hi Chris, FWIW changes looks good to me. cheers /Joel On Wed, 13 Apr 2016 at 17:43 Chris Hegarty <chris.hegarty@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.
- Update tests, as appropriate, to use the new location.
- Add the minimal set of critical APIs to jdk.unsupported/sun.reflect. These ultimately delegate to the internal versions. Additionally, a few new tests have been added to exercise these.
-Chris.
[1] https://bugs.openjdk.java.net/browse/JDK-8132928 [2] https://bugs.openjdk.java.net/browse/JDK-8153737
participants (6)
-
Alan Bateman
-
Chris Hegarty
-
Joel Borggrén-Franck
-
Kevin Rushforth
-
Mandy Chung
-
Paul Benedict