[8u] RFR: 8226575: OperatingSystemMXBean should be made container aware

[Severin Gehwolf]

Hi Andrew,

Thanks for the review!

On Thu, 2020-08-20 at 21:29 +0100, Andrew Hughes wrote:
> On 19:42 Thu 20 Aug     , Severin Gehwolf wrote:
> > They're unchanged:
> > 
> > http://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-July/012178.html
> > 
> > webrev:
> > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226575/jdk8/01/
> > 
> > Thanks,
> > Severin
> > 
> 
> Thanks. In future, it would help to tag the issue with jdk8u-needs-review
> and link to the RFR there, so it doesn't get lost in the mail. See
> the OpenJDK wiki [0] for further information and a link to the 8u review
> filter.

Fair enough.

> The mapping of the native files from 11u to 8u is rather confusing, as
> the changes are not 1:1.  A conversion mapping would have been helpful.
> 
> From what I can work out,
> 
> * src/jdk.management/aix/native/libmanagement_ext/UnixOperatingSystem.c
> => src/aix/native/sun/management/AixOperatingSystem.c
> 
> getSystemCpuLoad renamed to getSystemCpuLoad0 as noted.
> Copyright line missing.
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 missing.
> 
> * src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
> => src/solaris/native/sun/management/LinuxOperatingSystem.c
> 
> getSystemCpuLoad renamed to getSystemCpuLoad0 as noted.
> 
> Looks good, though there is an odd missing whitespace change near
> sysconf(_SC_NPROCESSORS_ONLN).
> 
> * src/jdk.management/macosx/native/libmanagement_ext/UnixOperatingSystem.c
> => src/solaris/native/sun/management/MacosxOperatingSystem.c
> 
> getSystemCpuLoad renamed to getSystemCpuLoad0 as noted.
> Copyright line missing.
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 missing.
> 
> * N/A
> => src/solaris/native/sun/management/OperatingSystemImpl.c
> 
> Addition of '0' suffix as noted.
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 implemented here under #ifndef linux.
> 
> * src/jdk.management/solaris/native/libmanagement_ext/UnixOperatingSystem.c
> => src/solaris/native/sun/management/SolarisOperatingSystem.c
> 
> getSystemCpuLoad renamed to getSystemCpuLoad0 as noted.
> Copyright line missing.
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 missing.
> 
> * N/A
> => src/windows/native/sun/management/OperatingSystemImpl.c
> 
> Addition of '0' suffix as noted.
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 implemented here as in UnixOperatingSystem.c
> Copyright line missing.
> 
> So AixOperatingSystem.c, MacosxOperatingSystem.c,
> LinuxOperatingSystem.c and SolarisOperatingSystem.c share common code
> from OperatingSystemImpl.c, and thus the default implementations of
> getHostConfiguredCpuCount0 & getSingleCpuLoad0 can be shared rather
> than appearing in each individual file. Is this correct?

Yes. See lines 290 through 311 which does exclusions for platforms not
the target platform:
http://hg.openjdk.java.net/jdk8u/jdk8u/jdk/file/ce1f37506608/make/lib/ServiceabilityLibraries.gmk#l290

> Again, clarification on these in the original RFR would have helped
> the review. If my assumption is correct, then the only fix needed
> for these files is to add the missing copyright updates in the non-Linux
> files.

Sorry, I didn't remember at the time and didn't take notes. It slipped
throught the cracks :(

> There is no Windows file in the 11u version. Why is it needed in 8u?
> In both, OperatingSystemImpl.java seems to be under
> src/jdk.management/unix/classes or src/solaris/classes/sun/management,
> rather than share/classes.

Good point. There is a Windows specific OperatingSystemImpl.java with
references to native methods. I missed that. Note to self: No need to
change the native method impls as they've not changed
in src/windows/classes/sun/management/OperatingSystemImpl.java either.
Reverted in the updated webrev.

Latest webrev here:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8226575/jdk8/02/webrev/

> Your description of the Subsystem.java changes is quite terse.
> Can you explain in more detail why the readMatchingLines change
> is not needed?

OpenJDK 8u doesn't have this bug which introduces readMatchingLines()
methods:

8217338: [Containers] Improve systemd slice memory limit support

Therefore, hunks which adapted those methods, or rather, refactored
them, aren't needed in this patch. Does that make sense?

> OperatingSystemImpl.java & OperatingSystemMXBean.java look fine.

Tested builds on Windows, AIX, Solaris and Linux with this updated
webrev.

Thanks,
Severin

> [0] https://wiki.openjdk.java.net/display/jdk8u/Main
> 
> Thanks,