Hi,
Thanks for all your comments, here is the new version:
http://cr.openjdk.java.net/~sjiang/JDK-8065213/01/
Mandy Chung wrote:
ManagementFactory.java
line 760: would be helpful to add some comments describing
that the implementation of this method. You can use
MemoryManagerMXBean and GarageCollectorMXBean as the example.
Done.
Comments for the addProxyNames
Changed the method name to getProxyNames and added the comments
line 867: it would be more readable if you break this into
two statements.
List<PlatofrmMBeanProvider> providers =
AccessController.doPrivileged(...);
providers.stream()
.forEachOrdered(...);
Done
line 875-886: worths formatting
line 885 - instead of creating a HashMap and put entry in the
forEach call.
You could have do:
.collect(toMap(PlatformComponent::getObjectNamePattern,
Function.identity())
Yes, as:
.collect(toMap(PlatformComponent::getObjectNamePattern,
Function.identity(), (p1, p2) -> p1));
You could also have the getMap() be called to initialize the
componentMap
static field (i.e. initialize it in the static initializer rather
than lazily
so the providers must be loaded anyway.
componentMap is now initialized statically.
Can you add comments for the PlatformMBeanFinder methods?
Yes done
com/sun/management/internal/PlatformMBeanProviderImpl.java
line 43: does this mxbeanList have to be created lazily?
Would it be better to make it a final field and create it at the
constructor?
Done as final
line 58-59, 66-67, 111-112, 118-119: nit formatting
java/lang/management/DefaultPlatformMBeanProvider.java
line 42: should this mxbeanList be a final field?
Done
you can replace java.lang.management.MemoryManagerMXBean.class
with MemoryManagerMXBean.class as it's in the same package.
Same apply to other java.lang.management classes.
Done
com/sun/management/internal/PlatformMBeanProviderImpl.java
line 43: does this mxbeanList have to be created lazily?
Would it be better to make it a final field and create it at the
constructor?
These providers will need to be loaded and the mxbeanList will be
looked at except for the cases that we are sure that the MXBean only
comes from the default provider. I see the cost of constructing
mxbeanList involves loading several classes. If performance is an
issue, perhaps the fast-path would be in ManagementFactory to defer
loading providers and may be better to follow up the performance side
in the second phase (that I expect more changes to sun/management
classes)
You may want to consider using limited doPrivileged (that can be done
in the second phase).
OK, now they are final, I will add doPrivileged in next version.
Daniel Fuchs wrote:
ManagementFactory.java:
line 871: there's a stray debug trace that you probably
forgot to remove:
871 System.out.println("\n\n===== jsl adding: "+provider);
:)
lines 877-885:
I believe it would improved readability if the content of the
forEachOrdered statement was factored out in a private static
method. Something like:
.forEachOrdered(provider -> addToComponentMap(componentMap, provider))
private static void addToComponentMap(
Map<String,PaltformComponent<?>> cmpMap,
PlatformMBeanProvider provider) {
provider.getPlatformComponentList().stream()
.collect(toMap(p -> p.getObjectNamePattern(), p -> p,
(p1, p2) -> {
throw new InternalError(p1.getObjectNamePattern()
+ " has been used as key for " + p1
+ ", it cannot be reused for " + p2);
}))
.values()
.stream() // put all components into a map, "putIfAbsent"
.forEach(p ->
cmpMap.putIfAbsent(p.getObjectNamePattern(), p));
}
You must know that code was changed :)
PlatformMBeanProviderImpl.java:
105 * 3 OperatingSystemMXBean
Not sure what '3' means here - I suggest to remove it.
OK
Thanks,
Shanliang
