Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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: http://cr.openjdk.java.net/~dtitov/8226575/webrev.06/
I reviewed Metrics and Subsystem in this version.
I think it's simpler to have unwrapIOExceptionAndRethrow handling the
InternalError case.
+ List lines = subsystem.readMatchingLines(param);
+ for (String line : lines) {
if (line.startsWith(match)) {
retval = conversion.apply(line);
break;
}
}This can simply call Metrics::readFilePrivileged and process on the
Stream. return Metrics::readFilePrivileged(Paths.get(subsystem.path(), param))
.filter(line -> line.startsWith(match))
.map(conversion::apply)
.findFirst().orElseGet(() ->retval);
I don't need to see a new webrev.
Mandy
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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 getTotalSwapSpaceSize() {
56 if (containerMetrics != null) {
57 long limit = containerMetrics.getMemoryAndSwapLimit();
58 // The memory limit metrics is not available if JVM
runs on Linux host ( not in a docker container)
59 // or if a docker container was started without
specifying a memory limit ( without '--memory='
60 // Docker option). In latter case there is no limit on
how much memory the container can use and
61 // it can use as much memory as the host's OS allows.
62 long memLimit = containerMetrics.getMemoryLimit();
63 if (limit >= 0 && memLimit >= 0) {
64 return limit - memLimit;
65 }
66 }
67 return getTotalSwapSpaceSize0();
68 }
Unneeded space after brackets '('.
Do we need to check if the (limit - memLimit) value is negative?
The same question is for getFreeSwapSpaceSize():
memSwapLimit - memLimit - (memSwapUsage - memUsage)
and getFreeMemorySize():
101 return limit - usage;
81 // If this happens just retry the loop for
a few iterations
Dot is missed at the end of comment.
http://cr.openjdk.java.net/~dtitov/8226575/webrev.05/test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java.html
34 System.out.println(String.format("Runtime.availableProcessors:
%d", Runtime.getRuntime().availableProcessors()));
35
System.out.println(String.format("OperatingSystemMXBean.getAvailableProcessors:
%d", osBean.getAvailableProcessors()));
36
System.out.println(String.format("OperatingSystemMXBean.getTotalMemorySize:
%d", osBean.getTotalMemorySize()));
37
System.out.println(String.format("OperatingSystemMXBean.getTotalPhysicalMemorySize:
%d", osBean.getTotalPhysicalMemorySize()));
38
System.out.println(String.format("OperatingSystemMXBean.getFreeMemorySize:
%d", osBean.getFreeMemorySize()));
39
System.out.println(String.format("OperatingSystemMXBean.getFreePhysicalMemorySize:
%d", osBean.getFreePhysicalMemorySize()));
40
System.out.println(String.format("OperatingSystemMXBean.getTotalSwapSpaceSize:
%d", osBean.getTotalSwapSpaceSize()));
41
System.out.println(String.format("OperatingSystemMXBean.getFreeSwapSpaceSize:
%d", osBean.getFreeSwapSpaceSize()));
42
System.out.println(String.format("OperatingSystemMXBean.getCpuLoad: %f",
osBean.getCpuLoad()));
43
System.out.println(String.format("OperatingSystemMXBean.getSystemCpuLoad:
%f", osBean.getSystemCpuLoad()));
To make the above lines a little bit shorter I'd suggest to define a
log() method like this:
private static void log(String msg) ( System.out.println(msg(; }
34 log(String.format("Runtime.availableProcessors: %d",
Runtime.getRuntime().availableProcessors()));
35 log(String.format("OperatingSystemMXBean.getAvailableProcessors:
%d", osBean.getAvailableProcessors()));
36 log(String.format("OperatingSystemMXBean.getTotalMemorySize: %d",
osBean.getTotalMemorySize()));
37
log(String.format("OperatingSystemMXBean.getTotalPhysicalMemorySize:
%d", osBean.getTotalPhysicalMemorySize()));
38 log(String.format("OperatingSystemMXBean.getFreeMemorySize: %d",
osBean.getFreeMemorySize()));
39
log(String.format("OperatingSystemMXBean.getFreePhysicalMemorySize: %d",
osBean.getFreePhysicalMemorySize()));
40 log(String.format("OperatingSystemMXBean.getTotalSwapSpaceSize:
%d", osBean.getTotalSwapSpaceSize()));
41 log(String.format("OperatingSystemMXBean.getFreeSwapSpaceSize:
%d", osBean.getFreeSwapSpaceSize()));
42 log(String.format("OperatingSystemMXBean.getCpuLoad: %f",
osBean.getCpuLoad()));
43 log(String.format("OperatingSystemMXBean.getSystemCpuLoad: %f",
osBean.getSystemCpuLoad()));
Thanks,
Serguei
On 12/6/19 17:41, Daniil Titov wrote:
Hi David, Mandy, and Bob,
Thank you for reviewing this fix.
Please review a new version of the fix [1] that includes the following changes
comparing to the previous version of the webrev ( webrev.04)
1. The changes in Javadoc made in the webrev.04 comparing to webrev.03 and to
CSR [3] were discarded.
2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
was also reverted to webrev.03 version that return host's values if the
metrics are unavailable or cannot be properly read.
I would like to mention that currently the native implementation of
these methods de-facto may return -1 at some circumstances,
but I agree that the changes proposed in the previous version of the
webrev increase such probability.
I filed the follow-up issue [4] as
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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: http://cr.openjdk.java.net/~dtitov/8226575/webrev.06/
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575
[3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428
Thank you,
Daniil
On 12/9/19, 1:55 PM, "Mandy Chung" wrote:
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
bufferedReader.readLine could throw IOException so for simplicity I just put
> the whole code block in doPrivileged. On the other side I don't believe
that BufferedReader.readline() requires FilePermission checks ( and tests
proved that)
> so we could change this implementation to the following:
>
> public static String getStringValue(SubSystem subsystem, String
parm) {
> if (subsystem == null) return null;
>
> try (BufferedReader bufferedReader =
>
AccessController.doPrivileged((PrivilegedExceptionAction) () ->
{
> return
Files.newBufferedReader(Paths.get(subsystem.path(), parm));
> })) {
> return bufferedReader.readLine();
> } catch (PrivilegedActionException | IOException e) {
> return null;
> }
> }
>
> Could you please advise are you OK with it or you would like to proceed
with the approach Mandy suggested to unwrap
> PrivilegedActionException exception and throw the cause instead?
>
I think it's simpler to read and understand if the doPrivileged call is
moved out as a separate method that will throw IOException as the
expected functionality as suggested above.
For SubSystem::getStringValue, one suggestion would be:
diff --git
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
---
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
@@ -29,7 +29,11 @@
import java.io.IOException;
import java.math.BigInteger;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.nio.file.Paths;
+import java.security.AccessController;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
@@ -90,9 +94,8 @@
public static String getStringValue(SubSystem subsystem, String
parm) {
if (subsystem == null) return null;
-try(BufferedReader bufferedReader =
Files.newBufferedReader(Paths.get(subsystem.path(), parm))) {
-String line = bufferedReader.readLine();
-return line;
+try {
+return subsystem.readStringValue(parm);
}
catch (IOException e) {
return null;
@@ -100,6 +103,24 @@
}
+private String readStringValue(String param) throws IOException {
+PrivilegedExceptionAction pea = () ->
Files.newBufferedReader(Paths.get(path(), param));
+try (BufferedReader bufferedReader =
AccessController.doPrivileged(pea)) {
+String line = bufferedReader.readLine();
+return line;
+} catch (PrivilegedActionException e) {
+Throwable x = e.getCause();
+if (x instanceof IOException)
+throw (IOException)x;
+if (x instanceof RuntimeException)
+throw (RuntimeException)x;
+if (x instanceof Error)
+throw (Error)x;
+
+throw new InternalError(x);
+}
+}
+
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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 bufferedReader.readLine
could throw IOException so for simplicity I just put
the whole code block in doPrivileged. On the other side I don't believe that
BufferedReader.readline() requires FilePermission checks ( and tests proved
that)
so we could change this implementation to the following:
public static String getStringValue(SubSystem subsystem, String parm) {
if (subsystem == null) return null;
try (BufferedReader bufferedReader =
AccessController.doPrivileged((PrivilegedExceptionAction) () ->
{
return
Files.newBufferedReader(Paths.get(subsystem.path(), parm));
})) {
return bufferedReader.readLine();
} catch (PrivilegedActionException | IOException e) {
return null;
}
}
Could you please advise are you OK with it or you would like to proceed with
the approach Mandy suggested to unwrap
PrivilegedActionException exception and throw the cause instead?
I think it's simpler to read and understand if the doPrivileged call is
moved out as a separate method that will throw IOException as the
expected functionality as suggested above.
For SubSystem::getStringValue, one suggestion would be:
diff --git
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
---
a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
+++
b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java
@@ -29,7 +29,11 @@
import java.io.IOException;
import java.math.BigInteger;
import java.nio.file.Files;
+import java.nio.file.Path;
import java.nio.file.Paths;
+import java.security.AccessController;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
@@ -90,9 +94,8 @@
public static String getStringValue(SubSystem subsystem, String
parm) {
if (subsystem == null) return null;
- try(BufferedReader bufferedReader =
Files.newBufferedReader(Paths.get(subsystem.path(), parm))) {
- String line = bufferedReader.readLine();
- return line;
+ try {
+ return subsystem.readStringValue(parm);
}
catch (IOException e) {
return null;
@@ -100,6 +103,24 @@
}
+ private String readStringValue(String param) throws IOException {
+ PrivilegedExceptionAction pea = () ->
Files.newBufferedReader(Paths.get(path(), param));
+ try (BufferedReader bufferedReader =
AccessController.doPrivileged(pea)) {
+ String line = bufferedReader.readLine();
+ return line;
+ } catch (PrivilegedActionException e) {
+ Throwable x = e.getCause();
+ if (x instanceof IOException)
+ throw (IOException)x;
+ if (x instanceof RuntimeException)
+ throw (RuntimeException)x;
+ if (x instanceof Error)
+ throw (Error)x;
+
+ throw new InternalError(x);
+ }
+ }
+
Re: RFR: 8235530: Removed duplicated threadByName methods in nsk/jdi tests
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 Holmes wrote: +1 on both counts Not sure JDITestRuntimeException is really necessary/useful versus just using RuntimeException, but that's a different issue. Thanks, David On 8/12/2019 2:30 pm, serguei.spit...@oracle.com wrote: Hi Leonid, The fix looks good. Thank you for taking care about it! I agree, it is an awful duplication. Thanks, Serguei On 12/7/19 18:17, Leonid Mesnik wrote: Hi Could you please review following fix which just remove duplicated threadByName methods and JDITestRuntimeException exceptions in nsk/jdi tests. I don't see any reason to have so many copies of them. The method threadByName is added nsk.share.jdi.Debugee class as 'threadByNameOrThrow' because slightly different 'threadByName' already exist there. I filed another sub-task https://bugs.openjdk.java.net/browse/JDK-8235544 to review usage and merge these 2 methods later. This fix affects about ~4000 lines and I want to keep it as straight-forward as possible. webrev: http://cr.openjdk.java.net/~lmesnik/8235530/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8235530 The next planned steps are in: https://bugs.openjdk.java.net/browse/JDK-8233830 Leonid
Re: RFR: 8235530: Removed duplicated threadByName methods in nsk/jdi tests
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 RuntimeException, but that's a different issue. Thanks, David On 8/12/2019 2:30 pm, serguei.spit...@oracle.com wrote: Hi Leonid, The fix looks good. Thank you for taking care about it! I agree, it is an awful duplication. Thanks, Serguei On 12/7/19 18:17, Leonid Mesnik wrote: Hi Could you please review following fix which just remove duplicated threadByName methods and JDITestRuntimeException exceptions in nsk/jdi tests. I don't see any reason to have so many copies of them. The method threadByName is added nsk.share.jdi.Debugee class as 'threadByNameOrThrow' because slightly different 'threadByName' already exist there. I filed another sub-task https://bugs.openjdk.java.net/browse/JDK-8235544 to review usage and merge these 2 methods later. This fix affects about ~4000 lines and I want to keep it as straight-forward as possible. webrev: http://cr.openjdk.java.net/~lmesnik/8235530/webrev.00/ bug: https://bugs.openjdk.java.net/browse/JDK-8235530 The next planned steps are in: https://bugs.openjdk.java.net/browse/JDK-8233830 Leonid
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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
get_cpuload_internal(...)
calls get_totalticks() and get_jvmticks() functions that update these counters.
But on the first call, yes, it compares the newly received counters with the
garbage.
It has the code that seems to be written to somehow mitigate it and in worse
case just return
0 or 1.0. But it also could be that there is some other problem this code
tries to solve so I'm not sure
we should remove these workarounds as a part of the current fix.
274 // seems like we sometimes end up with less kernel ticks when
275 // reading /proc/self/stat a second time, timing issue between
cpus?
276 if (pticks->usedKernel < tmp.usedKernel) {
277 kdiff = 0;
278 } else {
279 kdiff = pticks->usedKernel - tmp.usedKernel;
280 }
281 tdiff = pticks->total - tmp.total;
282 udiff = pticks->used - tmp.used;
283
284 if (tdiff == 0) {
285 user_load = 0;
286 } else {
287 if (tdiff < (udiff + kdiff)) {
288 tdiff = udiff + kdiff;
289 }
290 *pkernelLoad = (kdiff / (double)tdiff);
291 // BUG9044876, normalize return values to sane values
292 *pkernelLoad = MAX(*pkernelLoad, 0.0);
293 *pkernelLoad = MIN(*pkernelLoad, 1.0);
294
295 user_load = (udiff / (double)tdiff);
296 user_load = MAX(user_load, 0.0);
297 user_load = MIN(user_load, 1.0);
298 }
299 }
Best regards,
Daniil
On 12/8/19, 8:49 PM, "David Holmes" wrote:
Hi Daniil,
On 7/12/2019 11:41 am, Daniil Titov wrote:
> Hi David, Mandy, and Bob,
>
> Thank you for reviewing this fix.
>
> Please review a new version of the fix [1] that includes the following
changes comparing to the previous version of the webrev ( webrev.04)
> 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03
and to CSR [3] were discarded.
Okay.
> 2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
> was also reverted to webrev.03 version that return host's values if
the metrics are unavailable or cannot be properly read.
Okay.
> I would like to mention that currently the native implementation
of these methods de-facto may return -1 at some circumstances,
> but I agree that the changes proposed in the previous version of
the webrev increase such probability.
> I filed the follow-up issue [4] as Mandy suggested.
I added a comment to the bug. This is potentially a difficult problem to
resolve - it all depends on the likelihood of any errors and what they
really indicate.
> 3. The legacy methods were renamed as David suggested.
Thanks!
>
>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>> ! static int initialized=1;
>>
>> Am I reading this right that the code currently fails to actually do
the
>> initialization because of this ???
>
> Yes, currently the code fails to do the initialization but it was
unnoticed since method
> get_cpuload_internal(...) was never called for a specific CPU, the first
parameter "which"
> was always -1.
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?
>> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
>>
>> System.out.println(String.format(...)
>>
>> Why not simply
>>
>> System.out.printf(..)
>
> As I tried explain it earlier it would make the tests unstable.
> System.out.printf(...) just delegates the call to System.out.format(...)
that doesn't emit the string atomically.
> Instead it parses the format string into a list of FormatString objects
and then iterates over the list.
> As a result, the other traces occasionally got printed between these
iterations and break the pattern the test is expected to find
> in the output.
Sorry I missed the earlier explanation. I find it somewhat surprising
that format() works that way, but without unlimited buffering there will
always be a need to flush the outputstream at some point.
Thanks,
David
-
> For example, here is the sample of a such output that has the trace
message printe
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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)
() -> Files.newBufferedReader(Paths.get(subsystem.path(),
parm {
return bufferedReader.readLine();
} catch (PrivilegedActionException | IOException e) {
return null;
}
}
Best regards,
Daniil
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
bufferedReader.readLine could throw IOException so for simplicity I just put
the whole code block in doPrivileged. On the other side I don't believe
that BufferedReader.readline() requires FilePermission checks ( and tests
proved that)
so we could change this implementation to the following:
public static String getStringValue(SubSystem subsystem, String parm) {
if (subsystem == null) return null;
try (BufferedReader bufferedReader =
AccessController.doPrivileged((PrivilegedExceptionAction) () ->
{
return
Files.newBufferedReader(Paths.get(subsystem.path(), parm));
})) {
return bufferedReader.readLine();
} catch (PrivilegedActionException | IOException e) {
return null;
}
}
Could you please advise are you OK with it or you would like to proceed
with the approach Mandy suggested to unwrap
PrivilegedActionException exception and throw the cause instead?
Thank you,
Daniil
On 12/9/19, 9:48 AM, "Mandy Chung" wrote:
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
AccessController.doPrivileged((PrivilegedExceptionAction>)
() -> Files.lines(path));
} catch (PrivilegedActionException e) {
Throwable x = e.getCause();
if (x instanceof IOException)
throw (IOException)x;
if (x instanceof RuntimeException)
throw (RuntimeException)x;
if (x instanceof Error)
throw (Error)x;
throw new InternalError(x);
}
}
On 12/9/19 7:17 AM, Bob Vandette wrote:
> 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 fix.
>>
>> Please review a new version of the fix [1] that includes the
following changes comparing to the previous version of the webrev ( webrev.04)
>> 1. The changes in Javadoc made in the webrev.04 comparing to
webrev.03 and to CSR [3] were discarded.
>> 2. The implementation of methods getFreeMemorySize,
getTotalMemorySize, getFreeSwapSpaceSize and getTotalSwapSpaceSize
>> was also reverted to webrev.03 version that return host's
values if the metrics are unavailable or cannot be properly read.
>> I would like to mention that currently the native
implementation of these methods de-facto may return -1 at some circumstances,
>> but I agree that the changes proposed in the previous version
of the webrev increase such probability.
>> I filed the follow-up issue [4] as Mandy suggested.
>> 3. The legacy methods were renamed as David suggested.
>>
>>
>>>
src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>>> ! static int initialized=1;
>>>
>>> Am I reading this right that the code currently fails to actually
do the
>>> initialization because of this ???
>> Yes, currently the code fails to do the initialization but it was
unnoticed since method
>> get_cpuload_internal(...) was never called for a specific CPU, the
first parameter "which"
>> was always -1.
>>
>>> test/hotspot/jtreg/containers/docker/Check
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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 IOException so for simplicity I just put
the whole code block in doPrivileged. On the other side I don't believe that
BufferedReader.readline() requires FilePermission checks ( and tests proved
that)
so we could change this implementation to the following:
public static String getStringValue(SubSystem subsystem, String parm) {
if (subsystem == null) return null;
try (BufferedReader bufferedReader =
AccessController.doPrivileged((PrivilegedExceptionAction) () ->
{
return
Files.newBufferedReader(Paths.get(subsystem.path(), parm));
})) {
return bufferedReader.readLine();
} catch (PrivilegedActionException | IOException e) {
return null;
}
}
Could you please advise are you OK with it or you would like to proceed with
the approach Mandy suggested to unwrap
PrivilegedActionException exception and throw the cause instead?
Thank you,
Daniil
On 12/9/19, 9:48 AM, "Mandy Chung" wrote:
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
AccessController.doPrivileged((PrivilegedExceptionAction>)
() -> Files.lines(path));
} catch (PrivilegedActionException e) {
Throwable x = e.getCause();
if (x instanceof IOException)
throw (IOException)x;
if (x instanceof RuntimeException)
throw (RuntimeException)x;
if (x instanceof Error)
throw (Error)x;
throw new InternalError(x);
}
}
On 12/9/19 7:17 AM, Bob Vandette wrote:
> 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 fix.
>>
>> Please review a new version of the fix [1] that includes the following
changes comparing to the previous version of the webrev ( webrev.04)
>> 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03
and to CSR [3] were discarded.
>> 2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
>> was also reverted to webrev.03 version that return host's values if
the metrics are unavailable or cannot be properly read.
>> I would like to mention that currently the native implementation
of these methods de-facto may return -1 at some circumstances,
>> but I agree that the changes proposed in the previous version of
the webrev increase such probability.
>> I filed the follow-up issue [4] as Mandy suggested.
>> 3. The legacy methods were renamed as David suggested.
>>
>>
>>> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
>>> ! static int initialized=1;
>>>
>>> Am I reading this right that the code currently fails to actually do the
>>> initialization because of this ???
>> Yes, currently the code fails to do the initialization but it was
unnoticed since method
>> get_cpuload_internal(...) was never called for a specific CPU, the first
parameter "which"
>> was always -1.
>>
>>> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
>>>
>>> System.out.println(String.format(...)
>>>
>>> Why not simply
>>>
>>> System.out.printf(..)
>> As I tried explain it earlier it would make the tests unstable.
>> System.out.printf(...) just delegates the call to System.out.format(...)
that doesn't emit the string atomically.
>> Instead it parses the format string into a list of FormatString objects
and then iterates over the list.
>> As a result, the other traces occasionally got printed between these
iterations and break the pattern the test is expected to find
>> in the output.
>>
>> For example, here is the sample of a such output that has the trace
message printed between " OperatingSystemMXBean.getFreePhysicalMemorySize:"
>> and "1030762496".
>>
>>
>> [0.304s][trace][os,container] Memory Usage is: 42983424
>> OperatingSystemMXBean.getFreeMemorySize: 1030758400
>> [0.3
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"
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 wrote: On 12/5/19 6:45 PM, David Holmes wrote: Hi Serguei, On 6/12/2019 11:31 am, serguei.spit...@oracle.com wrote: Hi Chris and Alex, (I've also included Dan, David and Dean to the mailing list) We have to reach a consensus about this. This is just part of a much broader issue with JVM TI that I tried to have a discussion started based on Richard Reingruber's proposals around Escape Analysis: http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-September/029285.html Unfortunately that discussion did not get much traction. Hmm. I have the emails that precede yours above, but not that one. Not sure how what happened. Just read through it and it did give me one thought. Consider a model where the program is designed drive behavior of the agent, triggering the agent to do certain things by having the program do certain things. Normally an agent monitors the application, but in this case the application is purposefully controlling actions performed by the agent. If code is elided from the program, then the agent no longer performs as expected. It's a kind of backwards jvmti programming model, and you may ask why would anyone do this. I'm not sure if there's a good reason for it, but should it be expected to work given how the spec is written? My interpretation is that the current JVM TI PopFrame behavior does not break this model. The spec says: "any changes to the arguments, which occurred in the called method, remain;" As the code was eliminated by the compiler then no changes to this argument occurred. So, the PopFrame behavior follows the spec. So, I think, the option #2 is not right. But it depends on our basic philosophy. If the developer wants to control the agent then the program has to be designed to do something meaningful that is not going to be optimized out by the JIT compiler. You misunderstood my point. What I'm saying is that someone might do something like assign to a local with the specific intent of having that trigger a jmvti event, with the specific intent of having the agent perform some expected action as a result. Think of it as being a trigger for the agent, not as the agent monitoring the app. For example, you could right a program + agent, and setting a specific local in the program triggers the agent to turn on a light, and setting some other local turns it off. Absurd, but possible, and maybe there are less absurd applications. I think, I understood your point correctly. Your point is that the code that can be eliminated (e.g. local++) is not that meaningless as it seems to be. My point is that there are still other more reliable ways to trigger the agent. So that relying on something that can be eliminated by JIT compilers is not important to support. You are making the assumption that the agent author understands what Java code/variables *might* be eliminated by the JIT compiler. I don't think that's a good assumption. I might have code that does a really complicated thing in a local variable that is only useful to the agent itself. The JIT will see that the local variable cannot escape the function and is not used outside the function (as far as it can see) so it will elide the local variable and the code that was used to generated the local result in the variable. If that local result happens to be some computation that the agent needed to see to do its next operation... Thank you for sharing your point. I'm not insisting on my assumptions here, just not sure this is more important than allowing optimizations. Do you actually think this use case needs to be supported? In general, to identify our philosophy about interaction between JIT compiler code elimination and JVM TI we need to make some assumptions. Let's temporarily put JVM TI out of scope. Are there any assumptions when JIT compilers eliminate some code? Is it based on some vision what code is observable? If it was decided some code can be eliminated then is it JVM TI only that breaks such assumptions about observability? If so, then such optimizations can be disabled at some level. Then we end up debugging/profiling/monitoring, and finally, observing a slightly different application. Are we Okay with this? Do we need any compromises here? Maybe we need more flags to control the JIT compiler behavior. I would say that "in general" (not Java specific) there is an implicit assumption that compiler optimization and "debugging" are diametrically opposed to each other, and thus one cannot assume/expect that either can transparently co-exist, you either optimize or you debug, but there is a sliding scale between the two extremes, fully optimized and no opti
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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
AccessController.doPrivileged((PrivilegedExceptionAction>)
() -> Files.lines(path));
} catch (PrivilegedActionException e) {
Throwable x = e.getCause();
if (x instanceof IOException)
throw (IOException)x;
if (x instanceof RuntimeException)
throw (RuntimeException)x;
if (x instanceof Error)
throw (Error)x;
throw new InternalError(x);
}
}
On 12/9/19 7:17 AM, Bob Vandette wrote:
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 fix.
Please review a new version of the fix [1] that includes the following changes
comparing to the previous version of the webrev ( webrev.04)
1. The changes in Javadoc made in the webrev.04 comparing to webrev.03 and to
CSR [3] were discarded.
2. The implementation of methods getFreeMemorySize, getTotalMemorySize,
getFreeSwapSpaceSize and getTotalSwapSpaceSize
was also reverted to webrev.03 version that return host's values if the
metrics are unavailable or cannot be properly read.
I would like to mention that currently the native implementation of these
methods de-facto may return -1 at some circumstances,
but I agree that the changes proposed in the previous version of the
webrev increase such probability.
I filed the follow-up issue [4] as Mandy suggested.
3. The legacy methods were renamed as David suggested.
src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c
! static int initialized=1;
Am I reading this right that the code currently fails to actually do the
initialization because of this ???
Yes, currently the code fails to do the initialization but it was unnoticed
since method
get_cpuload_internal(...) was never called for a specific CPU, the first parameter
"which"
was always -1.
test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java
System.out.println(String.format(...)
Why not simply
System.out.printf(..)
As I tried explain it earlier it would make the tests unstable.
System.out.printf(...) just delegates the call to System.out.format(...) that
doesn't emit the string atomically.
Instead it parses the format string into a list of FormatString objects and
then iterates over the list.
As a result, the other traces occasionally got printed between these iterations
and break the pattern the test is expected to find
in the output.
For example, here is the sample of a such output that has the trace message printed
between " OperatingSystemMXBean.getFreePhysicalMemorySize:"
and "1030762496".
[0.304s][trace][os,container] Memory Usage is: 42983424
OperatingSystemMXBean.getFreeMemorySize: 1030758400
[0.305s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
[0.305s][trace][os,container] Memory Usage is: 42979328
[0.306s][trace][os,container] Path to /memory.usage_in_bytes is
/sys/fs/cgroup/memory/memory.usage_in_bytes
OperatingSystemMXBean.getFreePhysicalMemorySize: [0.306s][trace][os,container]
Memory Usage is: 42975232
1030762496
OperatingSystemMXBean.getTotalSwapSpaceSize: 499122176
java.lang.RuntimeException: 'OperatingSystemMXBean\\.getFreePhysicalMemorySize:
[1-9][0-9]+' missing from stdout/stderr
at
jdk.test.lib.process.OutputAnalyzer.shouldMatch(OutputAnalyzer.java:306)
at
TestMemoryAwareness.testOperatingSystemMXBeanAwareness(TestMemoryAwareness.java:151)
at TestMemoryAwareness.main(TestMemoryAwareness.java:73)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at
com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
at java.base/java.lang.Thread.run(Thread.java:832)
Testing: Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker tests
passed. Tier4-tier6 tests are still running.
[1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.05
[2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575
[3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428
[4] https://bugs.openjdk.java.net/browse/JDK-8235522
Thank you,
Daniil
On 12/6/19, 1:38 PM, "
Re: RFR: 8226575: OperatingSystemMXBean should be made container aware
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 fix. > > Please review a new version of the fix [1] that includes the following > changes comparing to the previous version of the webrev ( webrev.04) > 1. The changes in Javadoc made in the webrev.04 comparing to webrev.03 and to > CSR [3] were discarded. > 2. The implementation of methods getFreeMemorySize, getTotalMemorySize, > getFreeSwapSpaceSize and getTotalSwapSpaceSize > was also reverted to webrev.03 version that return host's values if the > metrics are unavailable or cannot be properly read. > I would like to mention that currently the native implementation of > these methods de-facto may return -1 at some circumstances, > but I agree that the changes proposed in the previous version of the > webrev increase such probability. > I filed the follow-up issue [4] as Mandy suggested. > 3. The legacy methods were renamed as David suggested. > > >> src/jdk.management/linux/native/libmanagement_ext/UnixOperatingSystem.c >> ! static int initialized=1; >> >> Am I reading this right that the code currently fails to actually do the >> initialization because of this ??? > > Yes, currently the code fails to do the initialization but it was unnoticed > since method > get_cpuload_internal(...) was never called for a specific CPU, the first > parameter "which" > was always -1. > >> test/hotspot/jtreg/containers/docker/CheckOperatingSystemMXBean.java >> >> System.out.println(String.format(...) >> >> Why not simply >> >> System.out.printf(..) > > As I tried explain it earlier it would make the tests unstable. > System.out.printf(...) just delegates the call to System.out.format(...) that > doesn't emit the string atomically. > Instead it parses the format string into a list of FormatString objects and > then iterates over the list. > As a result, the other traces occasionally got printed between these > iterations and break the pattern the test is expected to find > in the output. > > For example, here is the sample of a such output that has the trace message > printed between " OperatingSystemMXBean.getFreePhysicalMemorySize:" > and "1030762496". > > > [0.304s][trace][os,container] Memory Usage is: 42983424 > OperatingSystemMXBean.getFreeMemorySize: 1030758400 > [0.305s][trace][os,container] Path to /memory.usage_in_bytes is > /sys/fs/cgroup/memory/memory.usage_in_bytes > [0.305s][trace][os,container] Memory Usage is: 42979328 > [0.306s][trace][os,container] Path to /memory.usage_in_bytes is > /sys/fs/cgroup/memory/memory.usage_in_bytes > OperatingSystemMXBean.getFreePhysicalMemorySize: > [0.306s][trace][os,container] Memory Usage is: 42975232 > 1030762496 > OperatingSystemMXBean.getTotalSwapSpaceSize: 499122176 > > > java.lang.RuntimeException: > 'OperatingSystemMXBean\\.getFreePhysicalMemorySize: [1-9][0-9]+' missing from > stdout/stderr > > at > jdk.test.lib.process.OutputAnalyzer.shouldMatch(OutputAnalyzer.java:306) > at > TestMemoryAwareness.testOperatingSystemMXBeanAwareness(TestMemoryAwareness.java:151) > at TestMemoryAwareness.main(TestMemoryAwareness.java:73) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at java.base/java.lang.reflect.Method.invoke(Method.java:564) > at > com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298) > at java.base/java.lang.Thread.run(Thread.java:832) > > Testing: Mach5 tier1-tier3 and open/test/hotspot/jtreg/containers/docker > tests passed. Tier4-tier6 tests are still running. > > [1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.05 > [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575 > [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428 > [4] https://bugs.openjdk.java.net/browse/JDK-8235522 > > Thank you, > Daniil > > On 12/6/19, 1:38 PM, "Mandy Chung" wrote: > > > >On 12/6/19 5:59 AM, Bob Vandette wrote: >>> On Dec 6, 2019, at 2:49 AM, David Holmes wrote: >>> >>> >>> src/jdk.management/share/classes/com/sun/management/OperatingSystemMXBean.java >>> >>> The changes to allow for a return of -1 are somewhat more extensive than we >>> have previously discussed. These methods previously were (per the spec) >>> guaranteed to return some (assumably) meaningful value but now they are >>> effectively allowed to fail by returning -1. No existing code is expecting >>> to have to handle a return of -1 so I see this as a significa
RE: RFR (M) 8234510: Remove file seeking requirement for writing a heap dump
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 DumperSupport::dump_class_and_array_classes(Klass*) > should assert that Klass* is an InstanceKlass; or, even better, use > InstanceKlass* > as parameter. A former version of the patch had a lot of the Klass* types replaced by InstanceKlass* where appropriate. I removed those changes ultimately because they had not much to with making the heap dump streamable. But a later patch could change this. > DumpWriter::start_dump_entry(): It took me a while to understand how the > segment size is updated if the entry is huge, since by the time we finish the > entry the segment header will already be flushed out. The answer is, I think, > that this is not needed since we only write one record so the initial size we > wrote > into the segment header is still valid. Correct. > Proposed comment change: > > -// Will be fixed up later if we can add more entries. > +// Seed segment size with size of its first record. Should we add more > records later, >we will update the segment size (see >finish_dump_segment()) I’ve changed the comment to make it more clear. Best regards, Ralf
