RFR: 8065213 Specify and implement PlatformMBeanProvider for looking for all platform MBeans

[shanliang]

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