Re: RFR: 8203359: Container level resources events [v9]
On Wed, 19 May 2021 17:21:26 GMT, Erik Gahlin wrote: > I think you need to add the hook, for the event metadata to be correct. > Otherwise, the "period" setting will not show up. Yes. The failed test log would indicate also the rest of the metadata not being in a good shape. But with the hook registered everything works fine. - PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Wed, 21 Apr 2021 22:38:32 GMT, Erik Gahlin wrote:
>> Jaroslav Bachorik has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR.
>
> I wonder if something similar to below could be added to
> jdk.jfr.internal.Utils:
>
> private static Metrics[] metrics;
> public static Metrics getMetrics() {
> if (metrics == null) {
> metrics = new Metrics[] { Metrics.systemMetrics() };
> }
> return metrics[0];
> }
>
> public static boolean shouldSkipBytecode(String eventName, Class
> superClass) {
> if (superClass != AbstractJDKEvent.class) {
> return false;
> }
> if (!eventName.startsWith("jdk.Container")) {
> return false;
> }
> return getMetrics() == null;
> }
>
> Then we could add checks to
> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
>
> eventName = ei.getEventName();
> if (Utils.shouldSkipBytecode(eventName, superClass))) {
> return oldBytes;
> }
>
> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
>
> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
> !Modifier.isAbstract(clazz.getModifiers())) {
> if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass()))
> {
> return oldBytes;
> }
>
> This way we would not pay for generating bytecode for events in a
> non-container environment.
>
> Not sure if it works, but could perhaps make startup faster? We would still
> pay for generating the event handlers during registration, but it's much
> trickier to avoid since we need to store the event type somewhere.
> @egahlin Unfortunately, I had to make one late change in the periodic event
> hook registration.
> If the events are registered conditionally only when running in a container
> the event metadata are not correct and `TestDefaultConfigurations.java` test
> will fail. When I register the hooks unconditionally, the metadata is
> correctly generated and the test passes.
> I will hold off integration until I hear back from you whether this is
> acceptable or I should try to find an alternative solution.
I think we should always register the metadata, even if you can't get the
event.
That's how we handle different GCs. Users must be able to explore events even
if they can't use them. For example, you must be able to configure container
events in JMC (with correct labels/descriptions) without actually connecting to
a JVM running in a Docker container.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Wed, 21 Apr 2021 22:38:32 GMT, Erik Gahlin wrote:
>> Jaroslav Bachorik has refreshed the contents of this pull request, and
>> previous commits have been removed. The incremental views will show
>> differences compared to the previous content of the PR.
>
> I wonder if something similar to below could be added to
> jdk.jfr.internal.Utils:
>
> private static Metrics[] metrics;
> public static Metrics getMetrics() {
> if (metrics == null) {
> metrics = new Metrics[] { Metrics.systemMetrics() };
> }
> return metrics[0];
> }
>
> public static boolean shouldSkipBytecode(String eventName, Class
> superClass) {
> if (superClass != AbstractJDKEvent.class) {
> return false;
> }
> if (!eventName.startsWith("jdk.Container")) {
> return false;
> }
> return getMetrics() == null;
> }
>
> Then we could add checks to
> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
>
> eventName = ei.getEventName();
> if (Utils.shouldSkipBytecode(eventName, superClass))) {
> return oldBytes;
> }
>
> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
>
> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
> !Modifier.isAbstract(clazz.getModifiers())) {
> if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass()))
> {
> return oldBytes;
> }
>
> This way we would not pay for generating bytecode for events in a
> non-container environment.
>
> Not sure if it works, but could perhaps make startup faster? We would still
> pay for generating the event handlers during registration, but it's much
> trickier to avoid since we need to store the event type somewhere.
@egahlin Unfortunately, I had to make one late change in the periodic event
hook registration.
If the events are registered conditionally only when running in a container the
event metadata are not correct and `TestDefaultConfigurations.java` test will
fail. When I register the hooks unconditionally, the metadata is correctly
generated and the test passes.
I will hold off integration until I hear back from you whether this is
acceptable or I should try to find an alternative solution.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Thu, 22 Apr 2021 15:08:59 GMT, Jaroslav Bachorik
wrote:
>> I wonder if something similar to below could be added to
>> jdk.jfr.internal.Utils:
>>
>> private static Metrics[] metrics;
>> public static Metrics getMetrics() {
>> if (metrics == null) {
>> metrics = new Metrics[] { Metrics.systemMetrics() };
>> }
>> return metrics[0];
>> }
>>
>> public static boolean shouldSkipBytecode(String eventName, Class
>> superClass) {
>> if (superClass != AbstractJDKEvent.class) {
>> return false;
>> }
>> if (!eventName.startsWith("jdk.Container")) {
>> return false;
>> }
>> return getMetrics() == null;
>> }
>>
>> Then we could add checks to
>> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
>>
>> eventName = ei.getEventName();
>> if (Utils.shouldSkipBytecode(eventName, superClass))) {
>> return oldBytes;
>> }
>>
>> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
>>
>> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
>> !Modifier.isAbstract(clazz.getModifiers())) {
>> if (Utils.shouldSkipBytecode(clazz.getName(),
>> clazz.getSuperclass())) {
>> return oldBytes;
>> }
>>
>> This way we would not pay for generating bytecode for events in a
>> non-container environment.
>>
>> Not sure if it works, but could perhaps make startup faster? We would still
>> pay for generating the event handlers during registration, but it's much
>> trickier to avoid since we need to store the event type somewhere.
>
> @egahlin Sounds good.
> Any particular reason you are using `Metrics[]` array?
> @jbachorik Has this been tested on cgroups v1 and cgroups v2 Linux systems?
OK. I've tested the latest iteration on both (cgroup v2 and cgroup v1). Testing
looks good other than the `memoryPressure` issue.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Thu, 22 Apr 2021 15:08:59 GMT, Jaroslav Bachorik
wrote:
>> I wonder if something similar to below could be added to
>> jdk.jfr.internal.Utils:
>>
>> private static Metrics[] metrics;
>> public static Metrics getMetrics() {
>> if (metrics == null) {
>> metrics = new Metrics[] { Metrics.systemMetrics() };
>> }
>> return metrics[0];
>> }
>>
>> public static boolean shouldSkipBytecode(String eventName, Class
>> superClass) {
>> if (superClass != AbstractJDKEvent.class) {
>> return false;
>> }
>> if (!eventName.startsWith("jdk.Container")) {
>> return false;
>> }
>> return getMetrics() == null;
>> }
>>
>> Then we could add checks to
>> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
>>
>> eventName = ei.getEventName();
>> if (Utils.shouldSkipBytecode(eventName, superClass))) {
>> return oldBytes;
>> }
>>
>> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
>>
>> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
>> !Modifier.isAbstract(clazz.getModifiers())) {
>> if (Utils.shouldSkipBytecode(clazz.getName(),
>> clazz.getSuperclass())) {
>> return oldBytes;
>> }
>>
>> This way we would not pay for generating bytecode for events in a
>> non-container environment.
>>
>> Not sure if it works, but could perhaps make startup faster? We would still
>> pay for generating the event handlers during registration, but it's much
>> trickier to avoid since we need to store the event type somewhere.
>
> @egahlin Sounds good.
> Any particular reason you are using `Metrics[]` array?
@jbachorik Has this been tested on cgroups v1 and cgroups v2 Linux systems?
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Wed, 21 Apr 2021 22:38:32 GMT, Erik Gahlin wrote:
>> Jaroslav Bachorik has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix event metadata
>
> I wonder if something similar to below could be added to
> jdk.jfr.internal.Utils:
>
> private static Metrics[] metrics;
> public static Metrics getMetrics() {
> if (metrics == null) {
> metrics = new Metrics[] { Metrics.systemMetrics() };
> }
> return metrics[0];
> }
>
> public static boolean shouldSkipBytecode(String eventName, Class
> superClass) {
> if (superClass != AbstractJDKEvent.class) {
> return false;
> }
> if (!eventName.startsWith("jdk.Container")) {
> return false;
> }
> return getMetrics() == null;
> }
>
> Then we could add checks to
> jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
>
> eventName = ei.getEventName();
> if (Utils.shouldSkipBytecode(eventName, superClass))) {
> return oldBytes;
> }
>
> and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
>
> if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
> !Modifier.isAbstract(clazz.getModifiers())) {
> if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass()))
> {
> return oldBytes;
> }
>
> This way we would not pay for generating bytecode for events in a
> non-container environment.
>
> Not sure if it works, but could perhaps make startup faster? We would still
> pay for generating the event handlers during registration, but it's much
> trickier to avoid since we need to store the event type somewhere.
@egahlin Sounds good.
Any particular reason you are using `Metrics[]` array?
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
On Wed, 21 Apr 2021 13:34:28 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 incrementally with one
> additional commit since the last revision:
>
> Fix event metadata
I wonder if something similar to below could be added to jdk.jfr.internal.Utils:
private static Metrics[] metrics;
public static Metrics getMetrics() {
if (metrics == null) {
metrics = new Metrics[] { Metrics.systemMetrics() };
}
return metrics[0];
}
public static boolean shouldSkipBytecode(String eventName, Class
superClass) {
if (superClass.getClassLoader() != null) {
return false;
}
if (!eventName.startsWith("jdk.Container")) {
return false;
}
return getMetrics() == null;
}
Then we could add checks to
jdk.jfr.internal.JVMUpcalls::bytesForEagerInstrumentation(...)
eventName = ei.getEventName();
if (Utils.shouldSkipBytecode(eventName, superClass))) {
return oldBytes;
}
and jdk.jfr.internal.JVMUpcalls:onRetransform(...)
if (jdk.internal.event.Event.class.isAssignableFrom(clazz) &&
!Modifier.isAbstract(clazz.getModifiers())) {
if (Utils.shouldSkipBytecode(clazz.getName(), clazz.getSuperclass())) {
return oldBytes;
}
This way we would not pay for generating bytecode for events in a non-container
environment.
Not sure if it works, but could perhaps make startup faster? We would still pay
for generating the event handlers during registration, but it's much trickier
to avoid since we need to store the event type somewhere.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v9]
> 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 incrementally with one additional commit since the last revision: Fix event metadata - Changes: - all: https://git.openjdk.java.net/jdk/pull/3126/files - new: https://git.openjdk.java.net/jdk/pull/3126/files/67a61bd7..f766faf0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=07-08 Stats: 36 lines in 5 files changed: 0 ins; 5 del; 31 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
