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

[Andrew Hughes]

On 13:08 Fri 21 Aug     , Severin Gehwolf wrote:
> Hi Andrew,
> 
> Thanks for the review!
>

No problem.

snip...

> > 
> > 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
>

Thanks.

> > 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 :(

I tend to review backports by comparing the diff between the 11u and 8u
version (a pain in this instance where so much has moved). I also try
to write my RFRs from the same perspective, explaining why each change
is correct. I find you tend to spot these little oddities that way.

As I gather you must have worked all this out yourself during the patch,
telling the reviewer in the RFR avoids them having to do the same work
over again. It also makes it a bit more approachable, so you may get
a quicker review and so it helps both people.

At least, that's my 2p on the matter. Your own experience may vary.

> 
> > 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.

Thanks. It took me a while to spot it as well. At first, I thought Windows
was picking it up from one common shared Java file like the others, but
then I looked at the paths again and spotted the unix/solaris usage.

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

Thanks. JDK side looks good now. I'll wait for your response on
whether to do the other change first for the HotSpot side.

> > 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?
>

It does. I think I pretty much got there from the shorter comment,
but my brain wasn't sure enough that it had parsed it correctly.

> > OperatingSystemImpl.java & OperatingSystemMXBean.java look fine.
> 
> Tested builds on Windows, AIX, Solaris and Linux with this updated
> webrev.
>

You have access to AIX & Solaris builds? That might be useful for
other patches.

Out of interest, did the original pass on Windows?

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

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
OpenJDK Package Owner
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222