Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Mandy Chung
On 12/9/19 3:47 PM, Daniil Titov wrote: Hi Mandy and Bob, Please review a new version of the webrev [1] that moves doPrivileged calls in jdk.internal.platform.cgroupv1.SubSystem to separate methods that throw IOException, as Mandy suggested. Mach5 tests are still running. [1] Webrev:

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread serguei.spit...@oracle.com
Hi Daniil, It is not a full review, just some minor comments. In fact, I do not see real problems yet. http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/src/jdk.management/unix/classes/com/sun/management/internal/OperatingSystemImpl.java.frames.html   55 public long

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Daniil Titov
Hi Mandy and Bob, Please review a new version of the webrev [1] that moves doPrivileged calls in jdk.internal.platform.cgroupv1.SubSystem to separate methods that throw IOException, as Mandy suggested. Mach5 tests are still running. [1] Webrev:

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Mandy Chung
On 12/9/19 10:51 AM, Daniil Titov wrote: Hi Mandy and Bob, Why did you not change the exception caught in SubSystem.java:getStringValue to PrivilegedActionException from IOException so it’s consistent with the other get functions? In this method both Files.newBufferedReader and return

Re: RFR: 8235530: Removed duplicated threadByName methods in nsk/jdi tests

2019-12-09 Thread coleen . phillimore
Very nice! 4153 lines changed: 21 ins; 3841 del; 291 mod; 99816 unchg Coleen On 12/9/19 3:56 PM, Leonid Mesnik wrote: David, Serguei Thank you for review. I added comment about JDITestRuntimeException in https://bugs.openjdk.java.net/browse/JDK-8235544 Leonid On 12/7/19 9:19 PM, David

Re: RFR: 8235530: Removed duplicated threadByName methods in nsk/jdi tests

2019-12-09 Thread Leonid Mesnik
David, Serguei Thank you for review. I added comment about JDITestRuntimeException in https://bugs.openjdk.java.net/browse/JDK-8235544 Leonid On 12/7/19 9:19 PM, David Holmes wrote: +1 on both counts Not sure JDITestRuntimeException is really necessary/useful versus just using

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Daniil Titov
Hi David, > So we never try to access the uninitialized counters.cpus array which is >good but we still return garbage for counters.jvmTicks and > counters.cpuTicks - surely that should have been noticeable? It only affected the first time the CPU load was requested. Function

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Daniil Titov
A correction... We could even further simplify it as the following: public static String getStringValue(SubSystem subsystem, String parm) { if (subsystem == null) return null; try (BufferedReader bufferedReader = AccessController.doPrivileged((PrivilegedExceptionAction)

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Daniil Titov
Hi Mandy and Bob, > Why did you not change the exception caught in SubSystem.java:getStringValue > to PrivilegedActionException from IOException > so it’s consistent with the other get functions? In this method both Files.newBufferedReader and return bufferedReader.readLine could throw

Re: RFR: JDK-8215196: [Graal] vmTestbase/nsk/jvmti/PopFrame/popframe003/TestDescription.java fails with "changes for the arguments of the popped frame's method, did not remain current argument values"

2019-12-09 Thread Laurence Cable
inline... On 12/6/19 6:12 PM, serguei.spit...@oracle.com wrote: On 12/6/19 17:24, Daniel D. Daugherty wrote: On 12/6/19 6:26 PM, serguei.spit...@oracle.com wrote: On 12/6/19 13:52, Chris Plummer wrote: On 12/6/19 1:18 PM, serguei.spit...@oracle.com wrote: On 12/6/19 11:07, Chris Plummer

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Mandy Chung
Files:lines requires FilePermission check.  So it needs to be wrapped with doPrivileged.  The readFilePrivileged can unwrap and throw the cause instead like this:     static Stream readFilePrivileged(Path path) throws IOException { try { return

Re: RFR: 8226575: OperatingSystemMXBean should be made container aware

2019-12-09 Thread Bob Vandette
Why did you not change the exception caught in SubSystem.java:getStringValue to PrivilegedActionException from IOException so it’s consistent with the other get functions? Bob. > On Dec 6, 2019, at 8:41 PM, Daniil Titov wrote: > > Hi David, Mandy, and Bob, > > Thank you for reviewing this

RE: RFR (M) 8234510: Remove file seeking requirement for writing a heap dump

2019-12-09 Thread Schmelter, Ralf
Hi Thomas, thanks for the feedback. > In DumpWriter, _current_entry_left and _entry_ended seem only to be needed for > asserting. Please enclose their definition in DEBUG_ONLY, and initialize them > in the ctor. Good catch. I made them debug only. > (not your patch): since