Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On Aug 25, 2012, at 12:57 AM, Mandy Chung mandy.ch...@oracle.com wrote:
On 8/24/2012 3:44 PM, David Holmes wrote:
My other query with these changes is whether we are certain that using the
specified loader rather than the boot loader will always be correct.
Yes I'm to my best knowledge but I'm looking to the reviewers to tell me
otherwise :)
To the best of my knowledge too.
The class being loaded is either part of the same module or expressed in its
module dependency.
And for that reason it may be best to use the three arg method call [*] as much
as possible for code in the JDK code base. If code is shuffled around it then
becomes clearer if the constraint, either part of the same module or expressed
in its module dependency, breaks.
Paul.
[*] That is not a negative on the stuff below.
I ran the JCK tests in module mode with the jigsaw modular JDK that uncovered
these files to be changed. There are other places in the JDK that I didn't
touch since they were not found by my testing.
BTW, I have created a new CR:
7194006: A new Class.forName(String cn, boolean initialize) method
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.02/
This include Paul's suggestion and slightly improved the javadoc
for Class.forName you suggested [1]:
- * @param initialize whether the class must be initialized
+ * @param initialize if {@code true} the class will be initialized.
+ * See Section 12 of emThe Java Language
Specification/em.
Thanks
Mandy
[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2012-May/002534.html
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Hi Mandy,
--- old/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug
23 12:29:01 2012
+++ new/src/share/classes/com/sun/jmx/remote/internal/IIOPHelper.java Thu Aug
23 12:29:00 2012
@@ -52,7 +52,7 @@
AccessController.doPrivileged(new PrivilegedActionIIOPProxy() {
public IIOPProxy run() {
try {
-Class? c = Class.forName(IMPL_CLASS, true, null);
+Class? c = Class.forName(IMPL_CLASS);
Why not:
Class? c = Class.forName(IMPL_CLASS, true,
IIOPHelper.class.getClassLoader());
to avoid traversing up the call stack to obtain the calling class.
Paul.
On Aug 23, 2012, at 9:33 PM, Mandy Chung mandy.ch...@oracle.com wrote:
On 8/23/2012 11:58 AM, Alan Bateman wrote:
On 23/08/2012 18:43, Mandy Chung wrote:
This change is to bring the jdk modularization changes from jigsaw repo [1]
to jdk8. This allows the jdk modularization changes to be exposed for
testing as early as possible and minimize the amount of changes carried
in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9.
Webrev at:
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/
This looks good to me and it's good to have these changes in jdk8.
One suggestion for ReflectUtil is to add a private static boolean isAncestor
method as I think that would make the checks in needsPackageAccessCheck
easier to read. Also in ClassLoader you could use just use
needsClassLoaderPermissionCheck(from,to).
Done. This is a good cleanup I considered to do too but missed in the
previous webrev.
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/
Thanks for the review.
Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi On 08/24/2012 07:21 AM, serguei.spit...@oracle.com wrote: I was thinking about the same. But a CCC request is needed for such a change in public API. It can be done separately if any other API changes are necessary. Thanks, Serguei On 8/23/12 6:27 PM, David Holmes wrote: Hi Mandy, While I understand what you are doing and that it works it is far from obvious upon code inspection that where you replace null with Foo.class.getClassLoader(), that the result would always be null. Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. David On 24/08/2012 3:43 AM, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 24/08/2012 11:25, Rémi Forax wrote: I also vote for adding this overload, a lot of the usage of Class.forName with three parameters are in fact just a way to load a class without initialize it. Rémi I think this is worth looking at too. It would only change two or three usages in this patch but there likely be a lot more as other areas of the JDK are updated. -Alan.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 25/08/2012 1:53 AM, Mandy Chung wrote: On 8/23/2012 6:27 PM, David Holmes wrote: Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. Paul and Remi also suggest that during the jigsaw code review on the jigsaw-dev mailing list. It's worth considering. I plan to look at the usage of Class.forName in the JDK and add this new overload of Class.forName method if there are many use of it but I haven't got to it yet. I prefer to push this changeset as it and file a new CR and target it for JDK 8. My other query with these changes is whether we are certain that using the specified loader rather than the boot loader will always be correct. David Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 8/24/2012 3:44 PM, David Holmes wrote:
My other query with these changes is whether we are certain that using
the specified loader rather than the boot loader will always be correct.
Yes I'm to my best knowledge but I'm looking to the reviewers to tell me
otherwise :)
The class being loaded is either part of the same module or expressed in
its module dependency. I ran the JCK tests in module mode with the
jigsaw modular JDK that uncovered these files to be changed. There are
other places in the JDK that I didn't touch since they were not found by
my testing.
BTW, I have created a new CR:
7194006: A new Class.forName(String cn, boolean initialize) method
http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.02/
This include Paul's suggestion and slightly improved the javadoc
for Class.forName you suggested [1]:
- * @param initialize whether the class must be initialized
+ * @param initialize if {@code true} the class will be initialized.
+ * See Section 12 of emThe Java Language
Specification/em.
Thanks
Mandy
[1] http://mail.openjdk.java.net/pipermail/jigsaw-dev/2012-May/002534.html
Review Request: 7193339 Prepare system classes be defined by a non-null module loader
This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ This looks good to me and it's good to have these changes in jdk8. One suggestion for ReflectUtil is to add a private static boolean isAncestor method as I think that would make the checks in needsPackageAccessCheck easier to read. Also in ClassLoader you could use just use needsClassLoaderPermissionCheck(from,to). -Alan.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 8/23/2012 11:58 AM, Alan Bateman wrote: On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ This looks good to me and it's good to have these changes in jdk8. One suggestion for ReflectUtil is to add a private static boolean isAncestor method as I think that would make the checks in needsPackageAccessCheck easier to read. Also in ClassLoader you could use just use needsClassLoaderPermissionCheck(from,to). Done. This is a good cleanup I considered to do too but missed in the previous webrev. http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/ Thanks for the review. Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Mandy, It looks good. Just a question below. || *src/share/classes/java/lang/management/ManagementFactory.java* 596 String intfName = mxbeanInterface.getName(); 599 is not an instance of + mxbeanInterface); Did you want this?: 596 String intfName = cls.getName(); 599 is not an instance of + cls); Thanks, Serguei On 8/23/12 10:43 AM, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 8/23/2012 12:48 PM, serguei.spit...@oracle.com wrote: Mandy, It looks good. Thanks. Just a question below. || *src/share/classes/java/lang/management/ManagementFactory.java* 596 String intfName = mxbeanInterface.getName(); 599 is not an instance of + mxbeanInterface); Did you want this?: 596 String intfName = cls.getName(); 599 is not an instance of + cls); It doesn't make any difference and so leave it as it is will be fine. Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
On 23/08/2012 20:33, Mandy Chung wrote: : Done. This is a good cleanup I considered to do too but missed in the previous webrev. http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/ Thanks for the review. Mandy That looks much better, so looks good to me. -Alan
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Mandy, 1. You replace null with getClassLoader() calls in couple of places. getClassLoader requires special permissions - RuntimePermission(getClassLoader) so probably you need to use doPrivileget() there. Did you test your changes with SecurityManager/No permissions for the test ? 2. Did you consider moving sm.checkPermission(SecurityConstants.GET_CLASSLOADER_PERMISSION); inside ClassLoader.needsClassLoaderPermissionCheck(ccl, cl); ? 3. ManagementFactory.java Could you explain the reason of changes (except 588) ? -Dmitry On 2012-08-23 23:33, Mandy Chung wrote: On 8/23/2012 11:58 AM, Alan Bateman wrote: On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ This looks good to me and it's good to have these changes in jdk8. One suggestion for ReflectUtil is to add a private static boolean isAncestor method as I think that would make the checks in needsPackageAccessCheck easier to read. Also in ClassLoader you could use just use needsClassLoaderPermissionCheck(from,to). Done. This is a good cleanup I considered to do too but missed in the previous webrev. http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/ Thanks for the review. Mandy -- Dmitry Samersoff Java Hotspot development team, SPB04 * There will come soft rains ...
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Looks good. Thanks, Serguei On 8/23/12 12:33 PM, Mandy Chung wrote: On 8/23/2012 11:58 AM, Alan Bateman wrote: On 23/08/2012 18:43, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ This looks good to me and it's good to have these changes in jdk8. One suggestion for ReflectUtil is to add a private static boolean isAncestor method as I think that would make the checks in needsPackageAccessCheck easier to read. Also in ClassLoader you could use just use needsClassLoaderPermissionCheck(from,to). Done. This is a good cleanup I considered to do too but missed in the previous webrev. http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.01/ Thanks for the review. Mandy
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
Hi Mandy, While I understand what you are doing and that it works it is far from obvious upon code inspection that where you replace null with Foo.class.getClassLoader(), that the result would always be null. Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. David On 24/08/2012 3:43 AM, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
Re: Review Request: 7193339 Prepare system classes be defined by a non-null module loader
I was thinking about the same. But a CCC request is needed for such a change in public API. It can be done separately if any other API changes are necessary. Thanks, Serguei On 8/23/12 6:27 PM, David Holmes wrote: Hi Mandy, While I understand what you are doing and that it works it is far from obvious upon code inspection that where you replace null with Foo.class.getClassLoader(), that the result would always be null. Another way to simplify this would be to add a new overload of Class.forName(): public static Class? forName(String name, boolean initialize) That way the loader argument could be dropped completely from a number of the use-cases. David On 24/08/2012 3:43 AM, Mandy Chung wrote: This change is to bring the jdk modularization changes from jigsaw repo [1] to jdk8. This allows the jdk modularization changes to be exposed for testing as early as possible and minimize the amount of changes carried in the jigsaw repo that helps sync up jigsaw with jdk8/jdk9. Webrev at: http://cr.openjdk.java.net/~mchung/jdk8/webrevs/7193339/webrev.00/ Summary: The VM bootstrap class loader (null) has been the defining class loader for all system classes (i.e. rt.jar and any classes on the bootclasspath). In modular world, the system classes are going to be modularized into multiple modulesand and many of which will be loaded by its own (non-null) module loader. The JDK implementation has the assumption that the defining class loader of system classes is null and it'll no longer be true when running as modules. Typical patterns in the JDK code include: Class.forName(classname, false, null) should be changed to: Class.forName(classname, false,loader of the current class) if (loader == null) condition check should be modified to check if the loader is responsible for loading system classes. This is the first set of change for this problem we identified. Similar change in other components will be made in the future. Testing: run all jdk core test targets on all platforms. Thanks Mandy P.S. I'm cc'ing servicebility-dev as this patch modifies 2 files in the java.lang.management.
