Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Fri, 24 May 2024 15:45:07 GMT, Erik Gahlin wrote: >> Collapsing the extra layer of methods and combining the test into >> >> if (jfrTracing && FileReadEvent.enabled()) >> >> would indeed keep things a little neater. >> >> I'm still questioning the need for `jfrTracing` at all. There's the matter >> of how this boolean gets set and unset, and whether this is done in a way >> that comports with the memory model. Setting this aside, is the only benefit >> that it avoids loading an extra class at JVM startup time (without JFR)? In >> this case that would be the `FileReadEvent` class, which is the stub class >> in `jdk.internal.event`. Wouldn't this class be in the CDS archive, making >> it not worth the effort of bringing up this new `jfrTracing` mechanism >> simply to avoid loading it unnecessarily? >> >> I observe that in JDK 22 some (but not all) of the event classes in >> `jdk.internal.event` seem to be present in the CDS archive. These include >> various virtual thread events. > > I don't think the issue is so much the overhead of loading (one or more) > additional classes without JFR, even if it causes an extremely small > regression, but the added overhead to JFR when trying to fix the regression. > The cost of a JVMTI retransformation is perhaps 100 - 1000 times that of > loading a class in the CDS archive. > > I did an experiment with: > > `if (FlightRecorder.enabled && FileReadEvent.enabled())` > > and it passes JFR tests and tier1/tier2. I don't think > `FlightRecorder.enabled` needs to be used on every event class, but it would > be good to use on event classes loaded during startup, both for safety > reasons (ClassCircularityError) and to lower overhead of JFR startup. The > `jfrTracing` flag works as well, but it is perhaps harder to comprehend and > requires an additional static boolean in every class, which does clutter > things up. > > I can push Alan's suggestion of uniting the checks into one if-statement. It > may help to see how it looks. > > Virtual thread events are typically loaded in main, after JFR has started, > and not an issue unless `jcmd JFR.start` is used, which is not that common. Updated with Alan's suggestion to make the code more readable. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1617228065
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Tue, 21 May 2024 22:41:12 GMT, Stuart Marks wrote: >> I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable >> as it would avoid calling going through the traceXXX methods when the flag >> is enabled but the specific event is not enabled. > > Collapsing the extra layer of methods and combining the test into > > if (jfrTracing && FileReadEvent.enabled()) > > would indeed keep things a little neater. > > I'm still questioning the need for `jfrTracing` at all. There's the matter of > how this boolean gets set and unset, and whether this is done in a way that > comports with the memory model. Setting this aside, is the only benefit that > it avoids loading an extra class at JVM startup time (without JFR)? In this > case that would be the `FileReadEvent` class, which is the stub class in > `jdk.internal.event`. Wouldn't this class be in the CDS archive, making it > not worth the effort of bringing up this new `jfrTracing` mechanism simply to > avoid loading it unnecessarily? > > I observe that in JDK 22 some (but not all) of the event classes in > `jdk.internal.event` seem to be present in the CDS archive. These include > various virtual thread events. I don't think issue is so much the overhead of loading (one or more) additional classes without JFR, even if it causes a extremely small regression, but the added overhead added to JFR when trying to fix the regression. I did an experiment with: `if (FlightRecorder.enabled && FileReadEvent.enabled())` and it passes JFR tests and tier1/tier2. I don't think `FlightRecorder.enabled` needs to be used on every event class, but it would be good to use on event classes loaded during startup, both for safety reasons and to lower overhead of JFR startup. The `jfrTracing` flag works as well, but it is perhaps harder to comprehend and requires an additional static boolean in every class, which does clutter things up. I can push Alan's suggestion of uniting the checks into one if-statement. It may help to see how it looks. Virtual thread events are typically loaded in main, after JFR has started, and not an issue unless `jcmd JFR.start `is used, which is not that common. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1613687099
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Fri, 17 May 2024 09:26:03 GMT, Alan Bateman wrote: >> My main concern here is the addition of clutter (checking two flags, and >> creating two levels of nested "impl" methods) at every call site. We may >> need to rearrange our internal API for JFR (jdk.internal.events) in order to >> clean up the call sites without loading additional classes unnecessarily. >> >> I think the main performance comparison to make is the impact on startup >> time of loading the additional FileReadEvent class. That is, to compare the >> startup time of the baseline with code that loads the FileReadEvent class, >> with JFR disabled in both cases. I don't know how to do this. Anyway, if >> loading of additional classes imposes unacceptable overhead, then that >> justifies doing more work to avoid it. That's separate from whether adding >> the `jfrTracing` flag as done in this PR is an effective way to do it. > > I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable > as it would avoid calling going through the traceXXX methods when the flag is > enabled but the specific event is not enabled. Collapsing the extra layer of methods and combining the test into if (jfrTracing && FileReadEvent.enabled()) would indeed keep things a little neater. I'm still questioning the need for `jfrTracing` at all. There's the matter of how this boolean gets set and unset, and whether this is done in a way that comports with the memory model. Setting this aside, is the only benefit that it avoids loading an extra class at JVM startup time (without JFR)? In this case that would be the `FileReadEvent` class, which is the stub class in `jdk.internal.event`. Wouldn't this class be in the CDS archive, making it not worth the effort of bringing up this new `jfrTracing` mechanism simply to avoid loading it unnecessarily? I observe that in JDK 22 some (but not all) of the event classes in `jdk.internal.event` seem to be present in the CDS archive. These include various virtual thread events. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1609024695
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Mon, 13 May 2024 21:00:10 GMT, Stuart Marks wrote: >>> If an event class is loaded before JFR is started, the event class needs to >>> be retransformed, but if it is loaded later, we can add instrumentation on >>> class load and avoid the retransformation. More happens when an event class >>> is loaded compared to ordinary class load, for example, a startTime field >>> is added. >>> >>> I did a JMH run and the difference between Event::enabled() and a boolean >>> flag was a fraction of nanosecond. >> >> There are instances of FIS/FOS created in initPhase1 for the standard >> streams so loading as few classes and executing as minimal as possible is >> good. RAF will typically be used early too as the zip code opens zip files >> with a RAF. So doing as little as possible is good. > > My main concern here is the addition of clutter (checking two flags, and > creating two levels of nested "impl" methods) at every call site. We may need > to rearrange our internal API for JFR (jdk.internal.events) in order to clean > up the call sites without loading additional classes unnecessarily. > > I think the main performance comparison to make is the impact on startup time > of loading the additional FileReadEvent class. That is, to compare the > startup time of the baseline with code that loads the FileReadEvent class, > with JFR disabled in both cases. I don't know how to do this. Anyway, if > loading of additional classes imposes unacceptable overhead, then that > justifies doing more work to avoid it. That's separate from whether adding > the `jfrTracing` flag as done in this PR is an effective way to do it. I think `if (jfrTracing && FileReadEvent.enabled())` would be more readable as it would avoid calling going through the traceXXX methods when the flag is enabled but the specific event is not enabled. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1604627812
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Sun, 12 May 2024 13:35:42 GMT, Erik Gahlin wrote: > I guess it's not 100% safe if the JIT decides to store the read value > elsewhere over several event checks, but it seems unlikely. Event settings > checks (i.e., Event::isEnabled()) have always used plain reads, so it is not > more unreliable than any other event. > > I'm fine with using a volatile too. I used it for the exception events, but I > now think a plain write/read of jfrTracing is safe for all practical purposes. I'd prefer not introduce another volatile read into all these call paths. As you say, the issue has always been there so maybe it's okay for now and it can be examined later. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1604619014
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Sun, 12 May 2024 07:27:26 GMT, Alan Bateman wrote: >> If an event class is loaded before JFR is started, the event class needs to >> be retransformed, but if it is loaded later, we can add instrumentation on >> class load and avoid the retransformation. More happens when an event class >> is loaded compared to ordinary class load, for example, a startTime field is >> added. >> >> I did a JMH run and the difference between Event::enabled() and a boolean >> flag was a fraction of nanosecond. > >> If an event class is loaded before JFR is started, the event class needs to >> be retransformed, but if it is loaded later, we can add instrumentation on >> class load and avoid the retransformation. More happens when an event class >> is loaded compared to ordinary class load, for example, a startTime field is >> added. >> >> I did a JMH run and the difference between Event::enabled() and a boolean >> flag was a fraction of nanosecond. > > There are instances of FIS/FOS created in initPhase1 for the standard streams > so loading as few classes and executing as minimal as possible is good. RAF > will typically be used early too as the zip code opens zip files with a RAF. > So doing as little as possible is good. My main concern here is the addition of clutter (checking two flags, and creating two levels of nested "impl" methods) at every call site. We may need to rearrange our internal API for JFR (jdk.internal.events) in order to clean up the call sites without loading additional classes unnecessarily. I think the main performance comparison to make is the impact on startup time of loading the additional FileReadEvent class. That is, to compare the startup time of the baseline with code that loads the FileReadEvent class, with JFR disabled in both cases. I don't know how to do this. Anyway, if loading of additional classes imposes unacceptable overhead, then that justifies doing more work to avoid it. That's separate from whether adding the `jfrTracing` flag as done in this PR is an effective way to do it. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1599077056
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Sat, 11 May 2024 15:00:09 GMT, Alan Bateman wrote:
>> A compromise between performance and readability is:
>>
>> if (JFRTracing.isEnabled()) {
>> ...
>> }
>>
>> One additional class is loaded, but it's more clear where it comes from. I
>> didn't want to do that for the ThrowableTracer class since it had a clinit.
>>
>> This could potentially cause problems if JFRTracing is loaded early from
>> Throwable or other class in the future. The static boolean flag is more
>> safe, so probably better.
>
> One thing that isn't clear (to me anyway) is how it works with the memory
> model. It's plain read at the use sites, looks like the set when recording is
> turned on is also a plain write. Would it be possible to summarise what else
> happens when recording is turned on?
During JFR initialization, jfrTracing flag is set to true by the JFRTracing
class. Then, when the recording starts, a safepoint occurs, and all Java
threads are stopped at a poll site. When they wake up and eventually read the
memory of the jfrTracing field, it will be true because of memory fences
related to the safepoint.
I guess it's not 100% safe if the JIT decides to store the read value elsewhere
over several event checks, but it seems unlikely. Event settings checks (i.e.,
Event::isEnabled()) have always used plain reads, so it is not more unreliable
than any other event.
I'm fine with using a volatile too. I used it for the exception events, but I
now think a plain write/read of jfrTracing is safe for all practical purposes.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597639174
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Sat, 11 May 2024 19:31:34 GMT, Erik Gahlin wrote: > If an event class is loaded before JFR is started, the event class needs to > be retransformed, but if it is loaded later, we can add instrumentation on > class load and avoid the retransformation. More happens when an event class > is loaded compared to ordinary class load, for example, a startTime field is > added. > > I did a JMH run and the difference between Event::enabled() and a boolean > flag was a fraction of nanosecond. There are instances of FIS/FOS created in initPhase1 for the standard streams so loading as few classes and executing as minimal as possible is good. RAF will typically be used early too as the zip code opens zip files with a RAF. So doing as little as possible is good. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597570831
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Fri, 10 May 2024 00:43:32 GMT, Stuart Marks wrote:
>> Its purpose is to avoid loading the FileReadEvent class. When the class is
>> loaded, JFR will add fields and in some circumstances do other things. I
>> don't think the cost is high, but it may add up if the number of events
>> increases. Most Java applications don't run with JFR enabled, so this is to
>> prevent them from seeing a negative impact.
>>
>> I can update the text.
>
> Hm, I think this setup requires more discussion. The approach we had settled
> on was that at the call sites in the libraries, something like the following
> was done:
>
> public R operation(...) {
> if (SomeEvent.enabled()) {
> // perform operation0 with tracing
> // emit event if SomeEvent.shouldCommit(...) is true
> } else {
> return operation0(...); // perform operation without tracing
> }
> }
>
> private R operation0(...) { /* do the actual work */ }
>
> Now it looks like there's this additional flag `jfrTracing` that's set
> reflectively, and this flag is checked in a new layer of intermediate method
> calls:
>
> public R operation(...) {
> if (jfrTracing) {
> return traceOperation0(...);
> } else {
> return operation0(...);
> }
> }
>
> private R traceOperation0(...) {
> // stuff moved from public operation(...) above
> }
>
> That is, the former body of the public `operation(...)` method is moved into
> the new `traceOperation0(...)` method.
>
> I understand this is intended to help optimize startup time, but it adds
> clutter at each call site, and I'm wondering if it actually helps anything.
> The first time the application calls the `operation()` method, it's going to
> load a bunch of classes; the loading of this additional class is amortized
> over the loading and initialization of all the other classes in this area of
> the library. In addition, in the non-JFR case, the `enabled()` method
> implementation is simply `return false;` which can be inlined and which
> facilitiates dead code elimination.
>
> With `jfrTracing` added to the mix, it causes a load from a non-final boolean
> field that needs to be checked repeatedly. Maybe the JIT can optimize for the
> common case, but there's possibly an expense that needs to be paid here.
>
> At some point we should measure startup overhead for each case. I guess this
> can occur before or after this PR is integrated, depending on the urgency of
> things, but we should keep an eye on this issue.
If an event class is loaded before JFR is started, the event class needs to be
retransformed, but if it is loaded later, we can add instrumentation on class
load and avoid the retransformation. More happens when an event class is loaded
compared to ordinary class load, for example, a startTime field is added.
I did a JMH run and the difference between Event::enabled() and a boolean flag
was a fraction of nanosecond.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597496811
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 16:02:02 GMT, Erik Gahlin wrote:
>> The field is only used once and a VarHandle implementation loads three
>> additional classes during startup and in my measurements add about 0.6 ms to
>> startup.
>
> A compromise between performance and readability is:
>
> if (JFRTracing.isEnabled()) {
> ...
> }
>
> One additional class is loaded, but it's more clear where it comes from. I
> didn't want to do that for the ThrowableTracer class since it had a clinit.
>
> This could potentially cause problems if JFRTracing is loaded early from
> Throwable or other class in the future. The static boolean flag is more safe,
> so probably better.
One thing that isn't clear (to me anyway) is how it works with the memory
model. It's plain read at the use sites, looks like the set when recording is
turned on is also a plain write. Would it be possible to summarise what else
happens when recording is turned on?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597461416
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 11:19:14 GMT, Erik Gahlin wrote:
>> Hi,
>>
>> Could I have a review of a change that moves the jdk.FileRead and
>> jdk.FileWrite events to java.base to remove the use of the ASM
>> instrumentation.
>>
>> Testing: jdk/jdk/jfr
>>
>> Thanks
>> Erik
>
> Erik Gahlin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move methods
src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 419:
> 417:
> 418: private long implWrite(ByteBuffer[] srcs, int offset, int length)
> throws IOException
> 419: {
Style nit here, the "{" can move to the end of L418.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1597461653
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 14:59:34 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/java/io/FileInputStream.java line 63:
>>
>>> 61: private static final int DEFAULT_BUFFER_SIZE = 8192;
>>> 62:
>>> 63: // Flag that determines if file reads should be traced by JFR
>>
>> It could be good to also note what will modify this flag - here and in other
>> classes.
>> I'm going to guess that the purpose of this flag is purely performance, to
>> avoid even loading the event class, `FileReadEvent` here, during
>> startup/bootstrap and when JFR is not enabled, as read and write methods are
>> highly performance sensitive? Otherwise the flag could live in the event
>> class itself? Is it substantially faster to check this flag compared to
>> `FileReadEvent.enabled()`?
>
> Its purpose is to avoid loading the FileReadEvent class. When the class is
> loaded, JFR will add fields and in some circumstances do other things. I
> don't think the cost is high, but it may add up if the number of events
> increases. Most Java applications don't run with JFR enabled, so this is to
> prevent them from seeing a negative impact.
>
> I can update the text.
Hm, I think this setup requires more discussion. The approach we had settled on
was that at the call sites in the libraries, something like the following was
done:
public R operation(...) {
if (SomeEvent.enabled()) {
// perform operation0 with tracing
// emit event if SomeEvent.shouldCommit(...) is true
} else {
return operation0(...); // perform operation without tracing
}
}
private R operation0(...) { /* do the actual work */ }
Now it looks like there's this additional flag `jfrTracing` that's set
reflectively, and this flag is checked in a new layer of intermediate method
calls:
public R operation(...) {
if (jfrTracing) {
return traceOperation0(...);
} else {
return operation0(...);
}
}
private R traceOperation0(...) {
// stuff moved from public operation(...) above
}
That is, the former body of the public `operation(...)` method is moved into
the new `traceOperation0(...)` method.
I understand this is intended to help optimize startup time, but it adds
clutter at each call site, and I'm wondering if it actually helps anything. The
first time the application calls the `operation()` method, it's going to load a
bunch of classes; the loading of this additional class is amortized over the
loading and initialization of all the other classes in this area of the
library. In addition, in the non-JFR case, the `enabled()` method
implementation is simply `return false;` which can be inlined and which
facilitiates dead code elimination.
With `jfrTracing` added to the mix, it causes a load from a non-final boolean
field that needs to be checked repeatedly. Maybe the JIT can optimize for the
common case, but there's possibly an expense that needs to be paid here.
At some point we should measure startup overhead for each case. I guess this
can occur before or after this PR is integrated, depending on the urgency of
things, but we should keep an eye on this issue.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1596129912
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 15:41:42 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/jdk/internal/event/JFRTracing.java line 51:
>>
>>> 49: field.setAccessible(true);
>>> 50: field.setBoolean(null, true);
>>> 51: }
>>
>> Using reflection with `Field` seems expedient - a more modern way could be
>> to use `VarHandle` but I guess it would require more setup to obtain a
>> `Lookup` with the proper capabilities?
>
> The field is only used once and a VarHandle implementation loads three
> additional classes during startup and in my measurements add about 0.6 ms to
> startup.
A compromise between performance and readability is:
if (JFRTracing.isEnabled()) {
...
}
One additional class is loaded, but it's more clear where it comes from. I
didn't want to do that for the ThrowableTracer class since it had a clinit.
This could potentially cause problems if JFRTracing is loaded early from
Throwable or other class in the future. The static boolean flag is more safe,
so probably better.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595656857
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 14:29:13 GMT, Daniel Fuchs wrote: >> Erik Gahlin has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move methods > > src/java.base/share/classes/jdk/internal/event/JFRTracing.java line 51: > >> 49: field.setAccessible(true); >> 50: field.setBoolean(null, true); >> 51: } > > Using reflection with `Field` seems expedient - a more modern way could be to > use `VarHandle` but I guess it would require more setup to obtain a `Lookup` > with the proper capabilities? The field is only used once and a VarHandle implementation loads three additional classes during startup and in my measurements add about 0.6 ms to startup. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595627039
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 14:25:27 GMT, Daniel Fuchs wrote: >> Erik Gahlin has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Move methods > > src/java.base/share/classes/java/io/FileInputStream.java line 63: > >> 61: private static final int DEFAULT_BUFFER_SIZE = 8192; >> 62: >> 63: // Flag that determines if file reads should be traced by JFR > > It could be good to also note what will modify this flag - here and in other > classes. > I'm going to guess that the purpose of this flag is purely performance, to > avoid even loading the event class, `FileReadEvent` here, during > startup/bootstrap and when JFR is not enabled, as read and write methods are > highly performance sensitive? Otherwise the flag could live in the event > class itself? Is it substantially faster to check this flag compared to > `FileReadEvent.enabled()`? Its purpose is to avoid loading the FileReadEvent class. When the class is loaded, JFR will add fields and in some circumstances do other things. I don't think the cost is high, but it may add up if the number of events increases. Most Java applications don't run with JFR enabled, so this is to prevent them from seeing a negative impact. I can update the text. - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595570427
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 11:19:14 GMT, Erik Gahlin wrote: >> Hi, >> >> Could I have a review of a change that moves the jdk.FileRead and >> jdk.FileWrite events to java.base to remove the use of the ASM >> instrumentation. >> >> Testing: jdk/jdk/jfr >> >> Thanks >> Erik > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Move methods src/java.base/share/classes/java/io/FileInputStream.java line 63: > 61: private static final int DEFAULT_BUFFER_SIZE = 8192; > 62: > 63: // Flag that determines if file reads should be traced by JFR It could be good to also note what will modify this flag - here and in other classes. I'm going to guess that the purpose of this flag is purely performance, to avoid even loading the event class, `FileReadEvent` here, during startup/bootstrap and when JFR is not enabled, as read and write methods are highly performance sensitive? Otherwise the flag could live in the event class itself? Is it substantially faster to check this flag compared to `FileReadEvent.enabled()`? src/java.base/share/classes/jdk/internal/event/JFRTracing.java line 51: > 49: field.setAccessible(true); > 50: field.setBoolean(null, true); > 51: } Using reflection with `Field` seems expedient - a more modern way could be to use `VarHandle` but I guess it would require more setup to obtain a `Lookup` with the proper capabilities? - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595525764 PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595530833
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
On Thu, 9 May 2024 07:33:22 GMT, Erik Gahlin wrote: >> src/java.base/share/classes/sun/nio/ch/FileChannelImpl.java line 78: >> >>> 76: >>> 77: // Flag that determines if file reads/writes should be traced by JFR >>> 78: private static boolean jfrTracing; >> >> Should the force method be changed to test this flag too? >> >> I'm also wondering about the transferXXX methods. We might want to think >> about these for a separate PR as they have more potential to be outliers >> than the read/write methods. > > I think it would be good to use the flag for all events, but I rather do it > as separate PR so this is mostly a mechanical change to remove ASM. It makes > it easier to track regressions or improvements. > > I can file an enhancement for the transferXXX methods. "JFR: Add file event support for transfer methods" https://bugs.openjdk.org/browse/JDK-8331995 "JFR: Add boolean check before loading event classes" https://bugs.openjdk.org/browse/JDK-8331996 - PR Review Comment: https://git.openjdk.org/jdk/pull/19129#discussion_r1595353143
Re: RFR: 8331876: JFR: Move file read and write events to java.base [v3]
> Hi, > > Could I have a review of a change that moves the jdk.FileRead and > jdk.FileWrite events to java.base to remove the use of the ASM > instrumentation. > > Testing: jdk/jdk/jfr > > Thanks > Erik Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Move methods - Changes: - all: https://git.openjdk.org/jdk/pull/19129/files - new: https://git.openjdk.org/jdk/pull/19129/files/9d02bf63..fc54e205 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19129=02 - incr: https://webrevs.openjdk.org/?repo=jdk=19129=01-02 Stats: 587 lines in 4 files changed: 293 ins; 294 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/19129.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19129/head:pull/19129 PR: https://git.openjdk.org/jdk/pull/19129
