Hi Severin, On Thu, 2019-07-18 at 18:37 +0200, Severin Gehwolf wrote: > Hi Andrew, > > src/jdk.management/share/classes/module-info.java > > I don't think you need to explicitly require java.base. Everything > depends on java.base. Is there a specific reason why that was needed? >
Ah, you're right that that's not needed. I figured java.base should be implicitly depended upon, but the first run through failed due to module visibility. The actual fix needed was the other module-info change in the patch, ie: + exports jdk.internal.platform to + jdk.management; in the java.base module-info. I've gone ahead and removed the unnecessary addition. > I've noticed that this test fails with your patch: > > FAILED: > com/sun/management/OperatingSystemMXBean/GetSystemCpuLoad.java > > Fails with: > java.lang.RuntimeException: getSystemCpuLoad() returns > 173995.48501979 which is not in the [0.0,1.0] interval > at GetSystemCpuLoad.main(GetSystemCpuLoad.java:43) What did you run to get this test to execute? I hadn't hit that failure in running `make test TEST=tier1`, but I can verify the failure if I run specifically that class, so I'm unsure what larger suite or tier it's included in. I'll need to go back and rethink how this system load calculation can be done. > > This makes me wonder what the effect of this patch is on Linux > *outside* a container. Have you verified that Metric values > correspond > to what whas previously returned via native methods? Could you > verify, > please and tell us the before/after values? Thanks! Other than the system load, the other memory-related values appear to be correct both on bare metal and in Docker. Manually verifying the tests in com/sun/management/OperatingSystemMXBean with the "trace" argument and the patch applied (system load change removed), the values printed match my host machine's /proc/meminfo. Running jtreg:test/hotspot/jtreg/containers/docker tests also shows that the Docker tests succeed and the reported values within a container are as expected as well. I can't see that there are any tests to verify the Metrics implementation itself. Am I just missing them or have none been written? > > I wonder if we should split the OperatingSystemMXBean tests into it's > own (and not piggy-back on > TestCPUAwareness.java/TestMemoryAwareness.java). Have you considered > that? > Sure, I could do that for the next review round. Thanks, -- Andrew Azores Software Engineer, OpenJDK Team Red Hat
