On Tue, 28 May 2024 12:27:46 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
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
> 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
> 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
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
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
>>
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
On Thu, 16 May 2024 12:10:46 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
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
> 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
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
>>
> 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 three additional
commits
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
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
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
>>
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
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
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
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
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:
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
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
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
> 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
> 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
On Thu, 9 May 2024 07:20:55 GMT, Alan Bateman 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
>
>
On Tue, 7 May 2024 19:32:57 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
On Tue, 7 May 2024 19:32:57 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
Marked as reviewed by mgronlun
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
-
Commit messages:
- Update comment
- Initial
Changes:
29 matches
Mail list logo
