[PING] RFR: 8231111: Cgroups v2: Rework Metrics in java.base so as to recognize unified hierarchy

[Severin Gehwolf]

Hi Mandy,

On Thu, 2020-01-16 at 17:43 -0800, Mandy Chung wrote:
> Hi Bob, Severin,
> 
> On 1/9/20 11:51 AM, Severin Gehwolf wrote:
> > Thanks for the review! Should all be fixed now. Updated webrev:
> > 
> > incremental: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/07/incremental/webrev/
> > full: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/07/webrev/
>  
> This patch may be more appropriate to be reviewed by the
> serviceability group (cc'ed) as this is monitoring-related.

Thanks for the review, Mandy.

> jdk.internal.platform.Metrics is an internal API that you are free to
> change the API as appropriate.  Given that 13 out of 38 metrics
> defined in Metrics are no longer supported by cgroups v2, it's
> cleaner to refactor Metrics interface to be implementable by cgroups
> v1 and v2 and then define a cgroups version-specific metrics to
> extend Metrics, which means that it seems reasonable to make it
> linux-only sub-interface.  Client can cast to cgroup v1 metrics
> interface if needed.  Sorry for not chiming in earlier and I am not
> following the cgroups v2 changes.  This should be a straight-forward
> change which will make the implementation cleaner.  You would no
> longer need the new *_UNLIMITED and *_NOT_SUPPORTED constants.

If I understand you correctly, then we'd have to change how
-XshowSettings:system works currently with it. Not all metrics as
currently printed via LauncherHelper.java are supported in both worlds
(cgroup v1 & v2). It would make handling of cgroup v1 and cgroup v2
specific metrics a little more awkward. Adding instanceof checks and
downcasting as you suggest the consequence for a client would be.

See:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8231111/08/webrev/src/java.base/share/classes/sun/launcher/LauncherHelper.java.sdiff.html

Also, in order to not break cross platform promises we'd also have to
make the version specific interfaces available in shared code (or we'd
risk requiring the user to use reflection) to get the proper sub-
interface logic called.

I'll experiment with it, but I'm not convinced this makes the API any
nicer, really.

> A couple of quick comments when skimming on the new files:
>  CgroupSubsystemController.java
>    s/parm/param (including javadoc @param tag)
> 
> CgroupInfo.java
>    cgroupPath is not used??

Thanks, will update that in the next webrev.

Cheers,
Severin