Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
Mandy Chung wrote: CheckSomeMXBeanImplPackage.java line 45-50 & 58-60: should be called unconditionally since they should pass if java.management is present. The method "check" checks that an MBean implementation must be from "com.sun.management.internal", so even we look for an MXBean with its standard MXBean class (java.lang.management.*), do "check" still needs jdk.management module to be present. We need more unit tests for the case when jdk.management is absent. Thanks, Shanliang
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
On 4/2/15 12:58 PM, Shanliang Jiang wrote: Hi, I have to ask the review again because I need to modify: langtools/src/jdk.dev/share/classes/com/sun/tools/jdeps/Profile.java The issue was found when langtools tests were added into my test list. The new version is: http://cr.openjdk.java.net/~sjiang/JDK-8042901/02/ Thanks for catching the jdeps change. Profile.java change looks okay and this list is temporary. jdeps will be updated to determine the exported APIs at runtime when the module system is moving along. which integrated also the Mandy's comments in the following mail. CheckSomeMXBeanImplPackage.java line 45-50 & 58-60: should be called unconditionally since they should pass if java.management is present. Okay to push once the test is updated. Mandy
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
Hi,
I have to ask the review again because I need to modify:
langtools/src/jdk.dev/share/classes/com/sun/tools/jdeps/Profile.java
The issue was found when langtools tests were added into my test list.
The new version is:
http://cr.openjdk.java.net/~sjiang/JDK-8042901/02/
which integrated also the Mandy's comments in the following mail.
Thanks,
Shanliang
Mandy Chung wrote:
On 3/31/2015 9:39 AM, shanliang wrote:
Please review this fix:
Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/
Thank you for doing this big change to separate com.sun.management
API from java.management module. Looking really good.
Also thanks for fixing the tests to eliminate the unnecessary use of
JDK internal APIs.
modules.xml change looks good to me.
sun/management/ThreadImpl.java
35 /**
36 * Implementation class for the thread subsystem.
37 * Standard and committed hotspot-specific metrics if any.
38 *
39 * ManagementFactory.getThreadMXBean() returns an instance
40 * of this class.
41 */
42 // the implementation for com.sun.management.ThreadMXBean can
43 // be moved to jdk.management in the future.
What about:
Implementation for java.lang.management.ThreadMXBean as well as
providing the supporting method for com.sun.management.ThreadMXBean.
The supporting method for com.sun.management.ThreadMXBean can
be moved to jdk.management in the future.
CheckSomeMXBeanImplPackage.java
Thanks for adding this test.
Iterating jrt file system to find jdk.management module is one way.
Another simpler alternative is to call:
Class.forName("com.sun.management.GarbageCollectorMXBean");
and catch CNFE to determine if it's present or not.
You should also call ManagementFactory.getPlatformMXBeans on
com.sun.management.GarbageCollectorMXBean if present.
VMOptionOpenDataTest.java
copyright header year is wrong.
PlatformMBeanProviderConstructorCheck.java
The test needs to restore the original policy and security manager
before the test exits in case this case runs in agent vm mode.
The static permName and accepted variables are more appropriate
in MyPolicy class. Perhaps rename "accepted" to an instance
"denied" or "allowed" variable of MyPolicy class to reflect
what it intends to mean.
I'm okay if you make the change before the push. No need for a new
webrev.
Mandy
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
Looks good, thanks! /Erik On 2015-04-01 20:03, shanliang wrote: Erik Joelsson wrote: Hello, (Adding build-dev since this touches makefiles and jigsaw-dev since this touches modules.xml) In general, the build changes look pretty good. I much appreciate moving the OS specific source into OS specific source dirs. A few notes however. Though I realize you followed existing patterns, we have some more current best practices that I would like to incorporate in new code. jdk/make/lib/Lib-jdk.management.gmk: The variables BUILD_LIBJDKMANAGEMENT_SRC and BUILD_LIBJDKMANAGEMENT_CFLAGS should lose the "BUILD_" prefix. While it works, it makes them unnecessarily long and it risks conflicting with internal variables created in the SetupNativeCompilation call. Done. BUILD_LIBJDKMANAGEMENT_EXCLUDES is unused and should just be removed. The EXCLUDE_FILES parameter too. Done. LIBJDKMANAGEMENT_MAPFILE should be removed. This was a special construct for libmanagement used for a while until cmm was split into a separate module. Just inline the mapfile line into the macro call. Done. jdk/make/lib/Lib-java.management.gmk: BUILD_LIBMANAGEMENT_EXCLUDES is unused here as well. Removed. LIBMANAGEMENT_MAPFILE should be inlined here as well. Done. While you are at it, might as well fix the BUILD_ prefix on the SRC and CFLAGS variables here too if you don't mind. Done. Is the need for low optimization when debug symbols are active still valid for both libmanagement and libmanagement_ext? Sorry I do not know, I did not touch that flag stting in libmanagement, and I copied it for libmanagement_ext. Here is the new version: http://cr.openjdk.java.net/~sjiang/JDK-8042901/01/ Thanks for the review. Shanliang /Erik On 2015-03-31 18:39, shanliang wrote: Please review this fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8042901 Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/ Some code within the module java.management is separated and moved to the new module jdk.management, the new module takes the implementation code for Oracle Corporation's platform extension to the implementation of the java.lang.management API and also the management interface for some other components for the platform. Thanks, Shanliang
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
On 3/31/2015 9:39 AM, shanliang wrote:
Please review this fix:
Bug: https://bugs.openjdk.java.net/browse/JDK-8042901
Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/
Thank you for doing this big change to separate com.sun.management
API from java.management module. Looking really good.
Also thanks for fixing the tests to eliminate the unnecessary use of
JDK internal APIs.
modules.xml change looks good to me.
sun/management/ThreadImpl.java
35 /**
36 * Implementation class for the thread subsystem.
37 * Standard and committed hotspot-specific metrics if any.
38 *
39 * ManagementFactory.getThreadMXBean() returns an instance
40 * of this class.
41 */
42 // the implementation for com.sun.management.ThreadMXBean can
43 // be moved to jdk.management in the future.
What about:
Implementation for java.lang.management.ThreadMXBean as well as
providing the supporting method for com.sun.management.ThreadMXBean.
The supporting method for com.sun.management.ThreadMXBean can
be moved to jdk.management in the future.
CheckSomeMXBeanImplPackage.java
Thanks for adding this test.
Iterating jrt file system to find jdk.management module is one way.
Another simpler alternative is to call:
Class.forName("com.sun.management.GarbageCollectorMXBean");
and catch CNFE to determine if it's present or not.
You should also call ManagementFactory.getPlatformMXBeans on
com.sun.management.GarbageCollectorMXBean if present.
VMOptionOpenDataTest.java
copyright header year is wrong.
PlatformMBeanProviderConstructorCheck.java
The test needs to restore the original policy and security manager
before the test exits in case this case runs in agent vm mode.
The static permName and accepted variables are more appropriate
in MyPolicy class. Perhaps rename "accepted" to an instance
"denied" or "allowed" variable of MyPolicy class to reflect
what it intends to mean.
I'm okay if you make the change before the push. No need for a new
webrev.
Mandy
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
Erik Joelsson wrote: Hello, (Adding build-dev since this touches makefiles and jigsaw-dev since this touches modules.xml) In general, the build changes look pretty good. I much appreciate moving the OS specific source into OS specific source dirs. A few notes however. Though I realize you followed existing patterns, we have some more current best practices that I would like to incorporate in new code. jdk/make/lib/Lib-jdk.management.gmk: The variables BUILD_LIBJDKMANAGEMENT_SRC and BUILD_LIBJDKMANAGEMENT_CFLAGS should lose the "BUILD_" prefix. While it works, it makes them unnecessarily long and it risks conflicting with internal variables created in the SetupNativeCompilation call. Done. BUILD_LIBJDKMANAGEMENT_EXCLUDES is unused and should just be removed. The EXCLUDE_FILES parameter too. Done. LIBJDKMANAGEMENT_MAPFILE should be removed. This was a special construct for libmanagement used for a while until cmm was split into a separate module. Just inline the mapfile line into the macro call. Done. jdk/make/lib/Lib-java.management.gmk: BUILD_LIBMANAGEMENT_EXCLUDES is unused here as well. Removed. LIBMANAGEMENT_MAPFILE should be inlined here as well. Done. While you are at it, might as well fix the BUILD_ prefix on the SRC and CFLAGS variables here too if you don't mind. Done. Is the need for low optimization when debug symbols are active still valid for both libmanagement and libmanagement_ext? Sorry I do not know, I did not touch that flag stting in libmanagement, and I copied it for libmanagement_ext. Here is the new version: http://cr.openjdk.java.net/~sjiang/JDK-8042901/01/ Thanks for the review. Shanliang /Erik On 2015-03-31 18:39, shanliang wrote: Please review this fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8042901 Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/ Some code within the module java.management is separated and moved to the new module jdk.management, the new module takes the implementation code for Oracle Corporation's platform extension to the implementation of the java.lang.management API and also the management interface for some other components for the platform. Thanks, Shanliang
Re: RFR 8042901: Allow com.sun.management to be in a different module to java.lang.management
Hello, (Adding build-dev since this touches makefiles and jigsaw-dev since this touches modules.xml) In general, the build changes look pretty good. I much appreciate moving the OS specific source into OS specific source dirs. A few notes however. Though I realize you followed existing patterns, we have some more current best practices that I would like to incorporate in new code. jdk/make/lib/Lib-jdk.management.gmk: The variables BUILD_LIBJDKMANAGEMENT_SRC and BUILD_LIBJDKMANAGEMENT_CFLAGS should lose the "BUILD_" prefix. While it works, it makes them unnecessarily long and it risks conflicting with internal variables created in the SetupNativeCompilation call. BUILD_LIBJDKMANAGEMENT_EXCLUDES is unused and should just be removed. The EXCLUDE_FILES parameter too. LIBJDKMANAGEMENT_MAPFILE should be removed. This was a special construct for libmanagement used for a while until cmm was split into a separate module. Just inline the mapfile line into the macro call. jdk/make/lib/Lib-java.management.gmk: BUILD_LIBMANAGEMENT_EXCLUDES is unused here as well. LIBMANAGEMENT_MAPFILE should be inlined here as well. While you are at it, might as well fix the BUILD_ prefix on the SRC and CFLAGS variables here too if you don't mind. Is the need for low optimization when debug symbols are active still valid for both libmanagement and libmanagement_ext? /Erik On 2015-03-31 18:39, shanliang wrote: Please review this fix: Bug: https://bugs.openjdk.java.net/browse/JDK-8042901 Webrev: http://cr.openjdk.java.net/~sjiang/JDK-8042901/00/ Some code within the module java.management is separated and moved to the new module jdk.management, the new module takes the implementation code for Oracle Corporation's platform extension to the implementation of the java.lang.management API and also the management interface for some other components for the platform. Thanks, Shanliang
