Hi Andrew, Note that we should first get the CSR and - what the CSR should entail - in order before we can pursue this further. I'll help you with that. More below.
On Thu, 2019-07-18 at 16:28 -0400, Andrew Azores wrote: > 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. OK, cool. > > 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've run: $ jtreg -verbose:summary -jdk:./build/linux-x86_64-server-release/images/jdk test/jdk/com/sun/management/OperatingSystemMXBean test/jdk/com/sun/management/UnixOperatingSystemMXBean test/jdk/java/lang/management/OperatingSystemMXBean > I'll need to go back and rethink how this system load calculation can > be done. Sure. > > 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. OK. > 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? There is MetricsTester.java in the test library which is beeing used by TestCgroupMetrics.java and TestSystemMetrics.java > > 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. Great, thanks! Cheers, Severin
