Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Wed, 8 Nov 2023 13:39:10 GMT, Daniel Fuchs wrote: >> I agree, and I have looked into it, but I think it's better to do that >> refactorization separately as it will impact other events. > > Just for my own understanding: in this particular case the time stamp is > meaningless because the duration is expected to be 0 (or close to it) since > nothing happens between the time the timestamp is taken and the time the > event is committed. Is that correct? Yes, the event has no duration. Before https://bugs.openjdk.org/browse/JDK-8239508 we wrote timestamp and duration for all Java events. For exception events, we wrote duration = 0. Now we can differentiate between durational and instant events, which means bytecode can be generated to take the timestamp in the commit method. - PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1386777042
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Wed, 8 Nov 2023 05:34:47 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 62:
>>
>>> 60: if (ExceptionThrownEvent.enabled()) {
>>> 61: long timestamp = ExceptionThrownEvent.timestamp();
>>> 62: ExceptionThrownEvent.commit(timestamp, message, clazz);
>>
>> Just a drive-by observation, but now you get the timestamp from the event it
>> seems strange to pull it out and then pass it back to commit, instead of
>> having a version of commit that automatically uses the internal timestamp.
>
> I agree, and I have looked into it, but I think it's better to do that
> refactorization separately as it will impact other events.
Just for my own understanding: in this particular case the time stamp is
meaningless because the duration is expected to be 0 (or close to it) since
nothing happens between the time the timestamp is taken and the time the event
is committed. Is that correct?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1386635624
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Wed, 8 Nov 2023 05:13:04 GMT, David Holmes wrote:
>> Erik Gahlin has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Rename field from tracing to jfrTracing
>
> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 62:
>
>> 60: if (ExceptionThrownEvent.enabled()) {
>> 61: long timestamp = ExceptionThrownEvent.timestamp();
>> 62: ExceptionThrownEvent.commit(timestamp, message, clazz);
>
> Just a drive-by observation, but now you get the timestamp from the event it
> seems strange to pull it out and then pass it back to commit, instead of
> having a version of commit that automatically uses the internal timestamp.
I agree, and I have looked into it, but I think it's better to do that
refactorization separately as it will impact other events.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1386016878
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Tue, 7 Nov 2023 11:11:31 GMT, Erik Gahlin wrote:
>> Could I have a review of a PR that removes the bytecode instrumentation for
>> the exception events.
>>
>> Testing: jdk/jdk/jfr + tier1 + tier2
>
> Erik Gahlin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rename field from tracing to jfrTracing
src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 62:
> 60: if (ExceptionThrownEvent.enabled()) {
> 61: long timestamp = ExceptionThrownEvent.timestamp();
> 62: ExceptionThrownEvent.commit(timestamp, message, clazz);
Just a drive-by observation, but now you get the timestamp from the event it
seems strange to pull it out and then pass it back to commit, instead of having
a version of commit that automatically uses the internal timestamp.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1385990281
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Tue, 7 Nov 2023 11:11:31 GMT, Erik Gahlin wrote: >> Could I have a review of a PR that removes the bytecode instrumentation for >> the exception events. >> >> Testing: jdk/jdk/jfr + tier1 + tier2 > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Rename field from tracing to jfrTracing The changes looks okay and will track the follow-on issue to look at SOE. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16493#pullrequestreview-1717977940
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
On Tue, 7 Nov 2023 11:11:31 GMT, Erik Gahlin wrote: >> Could I have a review of a PR that removes the bytecode instrumentation for >> the exception events. >> >> Testing: jdk/jdk/jfr + tier1 + tier2 > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Rename field from tracing to jfrTracing Marked as reviewed by mgronlun (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16493#pullrequestreview-1717679195
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v3]
> Could I have a review of a PR that removes the bytecode instrumentation for > the exception events. > > Testing: jdk/jdk/jfr + tier1 + tier2 Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Rename field from tracing to jfrTracing - Changes: - all: https://git.openjdk.org/jdk/pull/16493/files - new: https://git.openjdk.org/jdk/pull/16493/files/23a07a18..195c1086 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16493=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16493=01-02 Stats: 12 lines in 3 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/16493.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16493/head:pull/16493 PR: https://git.openjdk.org/jdk/pull/16493
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Tue, 7 Nov 2023 09:27:51 GMT, Alan Bateman wrote: >> I filed an issue to investigate if there is a problem with SOE, or if the >> OOM check is really needed now. >> https://bugs.openjdk.org/browse/JDK-8319579 >> >> Regardless of outcome, It would be good to document the results of the >> investigation in the code. > >> I filed an issue to investigate if there is a problem with SOE, or if the >> OOM check is really needed now. https://bugs.openjdk.org/browse/JDK-8319579 >> >> Regardless of outcome, It would be good to document the results of the >> investigation in the code. > > Okay, the scenario I was wondering about is where traceError fails with SOE > and whether that could trigger a loop too. I understand. - PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384714404
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Tue, 7 Nov 2023 09:24:20 GMT, Alan Bateman wrote: >> Erik Gahlin has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove SecurityException and IllegalArgumentException from throws clause > > src/java.base/share/classes/java/lang/Throwable.java line 125: > >> 123: * Flag that determines if exceptions should be traced by JFR >> 124: */ >> 125: static volatile boolean tracing; > > The use-sites in Error check Throwable.tracing so not immediately obvious > that it's JFR tracing. Maybe it should be renamed to jfrTracing or an > accessor of that name added so it's a bit clearer at the use-sites. Will change - PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384714812
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Tue, 7 Nov 2023 02:10:29 GMT, Erik Gahlin wrote: > I filed an issue to investigate if there is a problem with SOE, or if the OOM > check is really needed now. https://bugs.openjdk.org/browse/JDK-8319579 > > Regardless of outcome, It would be good to document the results of the > investigation in the code. Okay, the scenario I was wondering about is where traceError fails with SOE and whether that could trigger a loop too. - PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384617053
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Mon, 6 Nov 2023 22:52:50 GMT, Erik Gahlin wrote: >> Could I have a review of a PR that removes the bytecode instrumentation for >> the exception events. >> >> Testing: jdk/jdk/jfr + tier1 + tier2 > > Erik Gahlin has updated the pull request incrementally with one additional > commit since the last revision: > > Remove SecurityException and IllegalArgumentException from throws clause src/java.base/share/classes/java/lang/Throwable.java line 125: > 123: * Flag that determines if exceptions should be traced by JFR > 124: */ > 125: static volatile boolean tracing; The use-sites in Error check Throwable.tracing so not immediately obvious that it's JFR tracing. Maybe it should be renamed to jfrTracing or an accessor of that name added so it's a bit clearer at the use-sites. - PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384612334
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
On Mon, 6 Nov 2023 22:41:17 GMT, Erik Gahlin wrote:
>> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
>>
>>> 42:
>>> 43: public static void traceError(Class clazz, String message) {
>>> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
>>
>> StackOverflowError is likely problematic too, maybe it should be
>> VirtualMachineError.
>
> I remember asking the same question ten years ago, when Nils did the original
> implementation, but I think it was only needed for OOM, because it creates an
> infinite loop when the event object was allocated, which resulted in a
> StackOverflowError instead OOM.
>
> Some OOM tests failed with JFR enabled.
>
> The event object allocation has been removed, but I think we can run into the
> allocation recursion by other means. I looked into it a few years ago, but I
> don't remember exactly why it failed.
>
> If a SOE happens, I think we are fine. There is something that prevents
> infinite recursion when the SOE object is created. Perhaps it is
> preallocated? I prefer to not change the behavior, at least not in this PR.
I filed an issue to investigate if there is a problem with SOE, or if the OOM
check is really needed now.
https://bugs.openjdk.org/browse/JDK-8319579
Regardless of outcome, It would be good to document the results of the
investigation in the code.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384263589
Re: RFR: 8319374: JFR: Remove instrumentation for exception events [v2]
> Could I have a review of a PR that removes the bytecode instrumentation for > the exception events. > > Testing: jdk/jdk/jfr + tier1 + tier2 Erik Gahlin has updated the pull request incrementally with one additional commit since the last revision: Remove SecurityException and IllegalArgumentException from throws clause - Changes: - all: https://git.openjdk.org/jdk/pull/16493/files - new: https://git.openjdk.org/jdk/pull/16493/files/95a54349..23a07a18 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16493=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16493=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16493.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16493/head:pull/16493 PR: https://git.openjdk.org/jdk/pull/16493
Re: RFR: 8319374: JFR: Remove instrumentation for exception events
On Mon, 6 Nov 2023 15:49:02 GMT, Alan Bateman wrote:
>> Could I have a review of a PR that removes the bytecode instrumentation for
>> the exception events.
>>
>> Testing: jdk/jdk/jfr + tier1 + tier2
>
> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 37:
>
>> 35: private static final AtomicLong numThrowables = new AtomicLong();
>> 36:
>> 37: public static void enable() throws NoSuchFieldException,
>> SecurityException, IllegalArgumentException, IllegalAccessException {
>
> SecurityException and IllegaArgumentException are unchecked so don't need
> them in the throws declaration.
Will fix.
> src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
>
>> 42:
>> 43: public static void traceError(Class clazz, String message) {
>> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
>
> StackOverflowError is likely problematic too, maybe it should be
> VirtualMachineError.
I remember asking the same question ten years ago, when Nils did the original
implementation, but I think it was only needed for OOM, because it creates an
infinite loop when the event object was allocated, which resulted in a
StackOverflowError instead OOM.
Some OOM tests failed with JFR enabled.
The event object allocation has been removed, but I think we can run into the
allocation recursion by other means. I looked into it a few years ago, but I
don't remember exactly why it failed.
If a SOE happens, I think we are fine. There is something that prevents
infinite recursion when the SOE object is created. Perhaps it is preallocated?
I prefer to not change the behavior, at least not in this PR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384124648
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1384124213
Re: RFR: 8319374: JFR: Remove instrumentation for exception events
On Fri, 3 Nov 2023 12:19:07 GMT, Erik Gahlin wrote:
> Could I have a review of a PR that removes the bytecode instrumentation for
> the exception events.
>
> Testing: jdk/jdk/jfr + tier1 + tier2
src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 37:
> 35: private static final AtomicLong numThrowables = new AtomicLong();
> 36:
> 37: public static void enable() throws NoSuchFieldException,
> SecurityException, IllegalArgumentException, IllegalAccessException {
SecurityException and IllegaArgumentException are unchecked so don't need them
in the throws declaration.
src/java.base/share/classes/jdk/internal/event/ThrowableTracer.java line 44:
> 42:
> 43: public static void traceError(Class clazz, String message) {
> 44: if (OutOfMemoryError.class.isAssignableFrom(clazz)) {
StackOverflowError is likely problematic too, maybe it should be
VirtualMachineError.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1383552825
PR Review Comment: https://git.openjdk.org/jdk/pull/16493#discussion_r1383552352
RFR: 8319374: JFR: Remove instrumentation for exception events
Could I have a review of a PR that removes the bytecode instrumentation for the exception events. Testing: jdk/jdk/jfr + tier1 + tier2 - Commit messages: - Remove Throwable and Error from instrumentation list - Initial Changes: https://git.openjdk.org/jdk/pull/16493/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16493=00 Issue: https://bugs.openjdk.org/browse/JDK-8319374 Stats: 560 lines in 19 files changed: 268 ins; 281 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/16493.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16493/head:pull/16493 PR: https://git.openjdk.org/jdk/pull/16493
