Re: RFR: 8203359: Container level resources events [v7]
On Wed, 21 Apr 2021 11:49:10 GMT, Erik Gahlin wrote: >> Ok. So what would be a good rule-of-thumb for when one should introduce a >> flag? > > I think we want to limit the number flags/options There are already too many, > preferably we would have none, but some of the ones we have today, like gc > and exception, are necessary, because the impact of have them on by default > would be catastrophic (long stop the world pauses and possibly million of > events per second). Thanks! Removed the flag. - PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v7]
On Wed, 14 Apr 2021 08:28:01 GMT, Jaroslav Bachorik wrote: >> src/jdk.jfr/share/conf/jfr/default.jfc line 1051: >> >>> 1049: false >>> 1050: >>> 1051: true >> >> I don't think we should create "flag" for "Container Events". Instead we >> should treat them like CPU and other OS events, always on. Since JFR can be >> used outside a container, it seems wrong to have this as an option. > > Ok. So what would be a good rule-of-thumb for when one should introduce a > flag? I think we want to limit the number flags/options There are already too many, preferably we would have none, but some of the ones we have today, like gc and exception, are necessary, because the impact of have them on by default would be catastrophic (long stop the world pauses and possibly million of events per second). - PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v7]
On Wed, 14 Apr 2021 08:28:37 GMT, Jaroslav Bachorik
wrote:
>> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line
>> 163:
>>
>>> 161: private static void initializeContainerEvents() {
>>> 162: containerMetrics = Container.metrics();
>>> 163: if (containerMetrics != null) {
>>
>> I understand this will reduce startup time, but it's contrary to how we
>> treat other events.
>>
>> We register events, even if they can't be used. We want users to see what
>> events are available (and their metadata) and use JMC recording wizard or
>> other means to configure a .jfc file without actually being connected to a
>> containerized process. We want the same events to minimize (subtle) platform
>> dependent bugs.
>>
>> I think we should try to find other means to reduce the startup time. It's
>> better to have consistent behaviour, but an initial implementation than
>> isn't as performant, than inconsistent behavior and somewhat faster
>> implementation.
>>
>> At some point we will need to address the startup cost of registering Java
>> events anyway. For example, we could generate metadata at build time in a
>> binary format, similar to what we already do with native events. Could even
>> be the same file. Then we can have hundreds of Java events without the cost
>> of reflection and unnecessary class loading at startup. We could add a
>> simple check so that bytecode for the container events (commit() etc) are
>> not generated unless in a container environment. A couple of (cached) checks
>> in JVMUpcalls may be sufficient to prevent instrumentation cost.
>
> Right. So, for the initial drop I will just leave the container events
> registered unconditionally.
I think that is fine.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v7]
On Mon, 12 Apr 2021 18:53:07 GMT, Erik Gahlin wrote:
>> Jaroslav Bachorik has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove trailing spaces
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 163:
>
>> 161: private static void initializeContainerEvents() {
>> 162: containerMetrics = Container.metrics();
>> 163: if (containerMetrics != null) {
>
> I understand this will reduce startup time, but it's contrary to how we treat
> other events.
>
> We register events, even if they can't be used. We want users to see what
> events are available (and their metadata) and use JMC recording wizard or
> other means to configure a .jfc file without actually being connected to a
> containerized process. We want the same events to minimize (subtle) platform
> dependent bugs.
>
> I think we should try to find other means to reduce the startup time. It's
> better to have consistent behaviour, but an initial implementation than isn't
> as performant, than inconsistent behavior and somewhat faster implementation.
>
> At some point we will need to address the startup cost of registering Java
> events anyway. For example, we could generate metadata at build time in a
> binary format, similar to what we already do with native events. Could even
> be the same file. Then we can have hundreds of Java events without the cost
> of reflection and unnecessary class loading at startup. We could add a simple
> check so that bytecode for the container events (commit() etc) are not
> generated unless in a container environment. A couple of (cached) checks in
> JVMUpcalls may be sufficient to prevent instrumentation cost.
Right. So, for the initial drop I will just leave the container events
registered unconditionally.
> src/jdk.jfr/share/conf/jfr/default.jfc line 1051:
>
>> 1049: false
>> 1050:
>> 1051: true
>
> I don't think we should create "flag" for "Container Events". Instead we
> should treat them like CPU and other OS events, always on. Since JFR can be
> used outside a container, it seems wrong to have this as an option.
Ok. So what would be a good rule-of-thumb for when one should introduce a flag?
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v7]
On Fri, 2 Apr 2021 12:33:03 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:
>
> Remove trailing spaces
src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 163:
> 161: private static void initializeContainerEvents() {
> 162: containerMetrics = Container.metrics();
> 163: if (containerMetrics != null) {
I understand this will reduce startup time, but it's contrary to how we treat
other events.
We register events, even if they can't be used. We want users to see what
events are available (and their metadata) and use JMC recording wizard or other
means to configure a .jfc file without actually being connected to a
containerized process. We want the same events to minimize (subtle) platform
dependent bugs.
I think we should try to find other means to reduce the startup time. It's
better to have consistent behaviour, but an initial implementation than isn't
as performant, than inconsistent behavior and somewhat faster implementation.
At some point we will need to address the startup cost of registering Java
events anyway. For example, we could generate metadata at build time in a
binary format, similar to what we already do with native events. Could even be
the same file. Then we can have hundreds of Java events without the cost of
reflection and unnecessary class loading at startup. We could add a simple
check so that bytecode for the container events (commit() etc) are not
generated unless in a container environment. A couple of (cached) checks in
JVMUpcalls may be sufficient to prevent instrumentation cost.
src/jdk.jfr/share/conf/jfr/default.jfc line 1051:
> 1049: false
> 1050:
> 1051: true
I don't think we should create "flag" for "Container Events". Instead we should
treat them like CPU and other OS events, always on. Since JFR can be used
outside a container, it seems wrong to have this as an option.
-
PR: https://git.openjdk.java.net/jdk/pull/3126
Re: RFR: 8203359: Container level resources events [v7]
> 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: Remove trailing spaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/3126/files - new: https://git.openjdk.java.net/jdk/pull/3126/files/78dc85d3..f4372e23 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3126&range=05-06 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 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
