Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Thu, 18 Apr 2024 15:14:10 GMT, Tim Prinzing wrote: >> I think it might be the cause for >> https://bugs.openjdk.org/browse/JDK-8329330 I have sent out a PR to remove >> those separately so the fix can be backported. >> https://github.com/openjdk/jdk/pull/18843 > > That's correct, it is an unrelated cleanup (other than it seems to cause > tests to fail now). @egahlin, do you want me to remove those from this PR? Yes, it would be good if you could remove those changes, so it can be handled separately. - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570979943
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Thu, 18 Apr 2024 14:32:28 GMT, Erik Gahlin wrote: >> src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: >> >>> 64: FileWriteEvent.class, >>> 65: SocketReadEvent.class, >>> 66: SocketWriteEvent.class, >> >> I'm guessing that this change which remove these two event classes is a >> drive-by-cleanup that should actually have been done with some previous fix >> in this area? >> Just wanted to double check it was intended as it doesn't seem to be related >> to file events. > > I think it might be the cause for https://bugs.openjdk.org/browse/JDK-8329330 > I have sent out a PR to remove those separately so the fix can be backported. > https://github.com/openjdk/jdk/pull/18843 That's correct, it is an unrelated cleanup (other than it seems to cause tests to fail now). @egahlin, do you want me to remove those from this PR? - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570953838
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 14:05:28 GMT, Daniel Fuchs wrote: >> Tim Prinzing has updated the pull request incrementally with one additional >> commit since the last revision: >> >> test file local to test > > src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: > >> 64: FileWriteEvent.class, >> 65: SocketReadEvent.class, >> 66: SocketWriteEvent.class, > > I'm guessing that this change which remove these two event classes is a > drive-by-cleanup that should actually have been done with some previous fix > in this area? > Just wanted to double check it was intended as it doesn't seem to be related > to file events. Yes, and I think it might be the cause for https://bugs.openjdk.org/browse/JDK-8329330 I will send out a PR to remove those separately (in 30 minutes) so a fix can be backported. - PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1570887596
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 01:34:07 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > test file local to test Sorry - just noticed this comment has been pending for a few days... src/jdk.jfr/share/classes/jdk/jfr/internal/instrument/JDKEvents.java line 66: > 64: FileWriteEvent.class, > 65: SocketReadEvent.class, > 66: SocketWriteEvent.class, I'm guessing that this change which remove these two event classes is a drive-by-cleanup that should actually have been done with some previous fix in this area? Just wanted to double check it was intended as it doesn't seem to be related to file events. - PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2006152864 PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1568907662
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
On Wed, 17 Apr 2024 01:34:07 GMT, Tim Prinzing wrote: >> Currently the JFR event FileForceEvent is generated by instrumenting the >> sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer >> mirror events with static methods. >> >> Added the event at jdk.internal.event.FileForceEvent, and changed >> jdk.jfr.events.FileForceEvent to be a mirror event. >> >> Updated FileChannelImpl to use the jdk internal event static methods, and >> removed the force() method from FileChannelImplInstrumentor. >> >> Uses the existing tests. > > Tim Prinzing has updated the pull request incrementally with one additional > commit since the last revision: > > test file local to test My comments have been addressed and I don't have any else to add. I assume @egahlin will cast an eye over this too. - Marked as reviewed by alanb (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18542#pullrequestreview-2008938324
Re: RFR: 8329138: Convert JFR FileForceEvent to static mirror event [v5]
> Currently the JFR event FileForceEvent is generated by instrumenting the > sun.nio.ch.FileChannelImpl class. This needs to be changed to use the newer > mirror events with static methods. > > Added the event at jdk.internal.event.FileForceEvent, and changed > jdk.jfr.events.FileForceEvent to be a mirror event. > > Updated FileChannelImpl to use the jdk internal event static methods, and > removed the force() method from FileChannelImplInstrumentor. > > Uses the existing tests. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: test file local to test - Changes: - all: https://git.openjdk.org/jdk/pull/18542/files - new: https://git.openjdk.org/jdk/pull/18542/files/70815943..366fca19 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18542=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18542=03-04 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/18542.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18542/head:pull/18542 PR: https://git.openjdk.org/jdk/pull/18542
