Re: RFR: 8203359: Container level resources events [v10]
On Tue, 27 Apr 2021 09:40:01 GMT, Severin Gehwolf wrote:
>> Jaroslav Bachorik has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 13 additional
>> commits since the last revision:
>>
>> - Prevent event container bytecode generation if no container present
>> - Fix event metadata
>> - Roll back conditional registration of container events
>> - Remove container events flag
>> - Remove trailing spaces
>> - Doh
>> - Report container type and register events conditionally
>> - Remove unused test files
>> - Initial test support for JFR container events
>> - Update the JFR control files
>> - ... and 3 more:
>> https://git.openjdk.java.net/jdk/compare/3322e9ff...04c3f092
>
> test/hotspot/jtreg/containers/docker/TestJFREvents.java line 147:
>
>> 145: .addClassOptions(eventName,
>> "period=endChunk"))
>> 146: .shouldHaveExitValue(0)
>> 147: .shouldContain(memoryPressureFld)
>
> This test fails for me on cgroupv1 with:
>
>
> --System.err:(42/1407)--
> stdout: [= EventType: jdk.ContainerMemoryUsage
> startTime = 946400166
> duration = 0
> eventThread = {
> osName = "main"
> osThreadId = 6
> javaName = "main"
> javaThreadId = 1
> group = {
> parent = {
> parent = N/A
> name = "system"
> }
> name = "main"
> }
> }
>
> stackTrace = null
> memoryFailCount = 0
> memoryUsage = 57786368
> swapMemoryUsage = 57782272
> ];
> stderr: []
> exitValue = 0
>
> java.lang.RuntimeException: 'memoryPressure' missing from stdout/stderr
>
> at
> jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:206)
> at TestJFREvents.testMemoryUsage(TestJFREvents.java:147)
> at TestJFREvents.main(TestJFREvents.java:77)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
> at
> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
> at
> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:568)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
> at java.base/java.lang.Thread.run(Thread.java:831)
>
> JavaTest Message: Test threw exception: java.lang.RuntimeException:
> 'memoryPressure' missing from stdout/stderr
>
> JavaTest Message: shutting down test
>
>
> I think `memoryPressure` got removed from the code and, thus, should get
> removed from the test.
Will fix
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v10]
On Mon, 26 Apr 2021 21:20:57 GMT, Erik Gahlin wrote:
>> Jaroslav Bachorik has updated the pull request with a new target base due to
>> a merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains 13 additional
>> commits since the last revision:
>>
>> - Prevent event container bytecode generation if no container present
>> - Fix event metadata
>> - Roll back conditional registration of container events
>> - Remove container events flag
>> - Remove trailing spaces
>> - Doh
>> - Report container type and register events conditionally
>> - Remove unused test files
>> - Initial test support for JFR container events
>> - Update the JFR control files
>> - ... and 3 more:
>> https://git.openjdk.java.net/jdk/compare/c00a534f...04c3f092
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 729:
>
>> 727:
>> 728: public static boolean shouldSkipBytecode(String eventName, Class
>> superClass) {
>> 729: if
>> (!superClass.getName().equals("jdk.jfr.events.AbstractJDKEvent")) {
>
> Was there a problem checking against the class instance? If so, perhaps you
> could add a check that the class is in the boot class loader (null).
Yes, `AbstractJDKEvent` is package private so it is not accessible from here.
> src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 737:
>
>> 735: private static Metrics getMetrics() {
>> 736: if (metrics == null) {
>> 737: metrics = Metrics.systemMetrics();
>
> Will this not lead to a lookup every time in an non-container environment?
Yes. Now I see why you used `Metrics[]` - will fix.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v10]
On Thu, 22 Apr 2021 16:00:32 GMT, Jaroslav Bachorik
wrote:
>> With this change it becomes possible to surface various cgroup level metrics
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>>
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>>
>> For each of those subsystems a configuration data will be emitted as well.
>> The initial proposal is to emit the configuration data events at least once
>> per chunk and the metrics values at 30 seconds interval.
>> By using these values the emitted events seem to contain useful information
>> without increasing overhead (the metrics values are read from `/proc`
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 13 additional
> commits since the last revision:
>
> - Prevent event container bytecode generation if no container present
> - Fix event metadata
> - Roll back conditional registration of container events
> - Remove container events flag
> - Remove trailing spaces
> - Doh
> - Report container type and register events conditionally
> - Remove unused test files
> - Initial test support for JFR container events
> - Update the JFR control files
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/23dc6e83...04c3f092
@jbachorik The test needs fixing.
test/hotspot/jtreg/containers/docker/TestJFREvents.java line 147:
> 145: .addClassOptions(eventName,
> "period=endChunk"))
> 146: .shouldHaveExitValue(0)
> 147: .shouldContain(memoryPressureFld)
This test fails for me on cgroupv1 with:
--System.err:(42/1407)--
stdout: [= EventType: jdk.ContainerMemoryUsage
startTime = 946400166
duration = 0
eventThread = {
osName = "main"
osThreadId = 6
javaName = "main"
javaThreadId = 1
group = {
parent = {
parent = N/A
name = "system"
}
name = "main"
}
}
stackTrace = null
memoryFailCount = 0
memoryUsage = 57786368
swapMemoryUsage = 57782272
];
stderr: []
exitValue = 0
java.lang.RuntimeException: 'memoryPressure' missing from stdout/stderr
at
jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:206)
at TestJFREvents.testMemoryUsage(TestJFREvents.java:147)
at TestJFREvents.main(TestJFREvents.java:77)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at
java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:78)
at
java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:568)
at
com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:127)
at java.base/java.lang.Thread.run(Thread.java:831)
JavaTest Message: Test threw exception: java.lang.RuntimeException:
'memoryPressure' missing from stdout/stderr
JavaTest Message: shutting down test
I think `memoryPressure` got removed from the code and, thus, should get
removed from the test.
-
Changes requested by sgehwolf (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v10]
On Thu, 22 Apr 2021 16:00:32 GMT, Jaroslav Bachorik
wrote:
>> With this change it becomes possible to surface various cgroup level metrics
>> (available via `jdk.internal.platform.Metrics`) as JFR events.
>>
>> Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is
>> turned into JFR events to start with.
>> * CPU related metrics
>> * Memory related metrics
>> * I/O related metrics
>>
>> For each of those subsystems a configuration data will be emitted as well.
>> The initial proposal is to emit the configuration data events at least once
>> per chunk and the metrics values at 30 seconds interval.
>> By using these values the emitted events seem to contain useful information
>> without increasing overhead (the metrics values are read from `/proc`
>> filesystem so that should not be done too frequently).
>
> Jaroslav Bachorik has updated the pull request with a new target base due to
> a merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 13 additional
> commits since the last revision:
>
> - Prevent event container bytecode generation if no container present
> - Fix event metadata
> - Roll back conditional registration of container events
> - Remove container events flag
> - Remove trailing spaces
> - Doh
> - Report container type and register events conditionally
> - Remove unused test files
> - Initial test support for JFR container events
> - Update the JFR control files
> - ... and 3 more:
> https://git.openjdk.java.net/jdk/compare/904d9495...04c3f092
src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 729:
> 727:
> 728: public static boolean shouldSkipBytecode(String eventName, Class
> superClass) {
> 729: if
> (!superClass.getName().equals("jdk.jfr.events.AbstractJDKEvent")) {
Was there a problem checking against the class instance? If so, perhaps you
could add a check that the class is in the boot class loader (null).
src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java line 737:
> 735: private static Metrics getMetrics() {
> 736: if (metrics == null) {
> 737: metrics = Metrics.systemMetrics();
Will this not lead to a lookup every time in an non-container environment?
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v10]
> With this change it becomes possible to surface various cgroup level metrics > (available via `jdk.internal.platform.Metrics`) as JFR events. > > Only a subset of the metrics exposed by `jdk.internal.platform.Metrics` is > turned into JFR events to start with. > * CPU related metrics > * Memory related metrics > * I/O related metrics > > For each of those subsystems a configuration data will be emitted as well. > The initial proposal is to emit the configuration data events at least once > per chunk and the metrics values at 30 seconds interval. > By using these values the emitted events seem to contain useful information > without increasing overhead (the metrics values are read from `/proc` > filesystem so that should not be done too frequently). Jaroslav Bachorik has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Prevent event container bytecode generation if no container present - Fix event metadata - Roll back conditional registration of container events - Remove container events flag - Remove trailing spaces - Doh - Report container type and register events conditionally - Remove unused test files - Initial test support for JFR container events - Update the JFR control files - ... and 3 more: https://git.openjdk.java.net/jdk/compare/7babcb00...04c3f092 - Changes: - all: https://git.openjdk.java.net/jdk/pull/3126/files - new: https://git.openjdk.java.net/jdk/pull/3126/files/f766faf0..04c3f092 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=08-09 Stats: 36024 lines in 1257 files changed: 5443 ins; 26264 del; 4317 mod Patch: https://git.openjdk.java.net/jdk/pull/3126.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3126/head:pull/3126 PR: https://git.openjdk.java.net/jdk/pull/3126
