Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: >> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never >> supported the value `"rw"` since the source code was imported to the openjdk >> repo more than 15 years ago. In fact HotSpot throws >> `IllegalArgumentException` when such a mode is specified. >> >> It's unlikely such a mode will be required for future enhancements. Support >> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` >> is the only supported mode. >> >> I also cleaned up related code in the JDK and HotSpot. >> >> Testing: >> - Passed tiers 1 ~ 5 > > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. Thanks to @AlanBateman and @cl4es for the review. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Wed, 11 May 2022 02:43:21 GMT, Ioi Lam wrote: >> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never >> supported the value `"rw"` since the source code was imported to the openjdk >> repo more than 15 years ago. In fact HotSpot throws >> `IllegalArgumentException` when such a mode is specified. >> >> It's unlikely such a mode will be required for future enhancements. Support >> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` >> is the only supported mode. >> >> I also cleaned up related code in the JDK and HotSpot. >> >> Testing: >> - Passed tiers 1 and 2 >> - Tiers 3, 4, 5 are in progress > > Ioi Lam has updated the pull request incrementally with one additional commit > since the last revision: > > review comments Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 20:03:45 GMT, Alan Bateman wrote: > I skimmed through the changes and I think they look okay. In the distant past > there were tools outside of the JDK that used the jvmstat API directly. It's > possible that VisualVM still does but it would only compile/run if > --add-exports is used to export the sun.jvmstat.* packages. So it might be > that dropping the parameter from a method in RemoteHost is noticed and I > think that is okay because this package is not exported and is not meant to > be used by code outside of the JDK. The APIs changed by this PR are: - sun.jvmstat.monitor.remote.RemoteHost::attachVm - sun.jvmstat.monitor.VmIdentifier::getMode - sun.jvmstat.monitor.HostIdentifier::getMode - jdk.internal.perf.Perf::attach I checked the latest VisualVM source code. Some of the above classes are referenced, but none of the affected APIs are called. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 19:59:41 GMT, Alan Bateman wrote:
>> Ioi Lam has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
> line 60:
>
>> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
>> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L,
>> (int)fc.size());
>> 60: fc.close(); // doesn't need to remain open
>
> I think you can change this to:
>
>
> try (FileChannel fc = FileChannel.open(f.toPath())) {
> ByteBuffer bb = ...
> createPerfDataBuffer(bb, 0);
> }
Fixed.
-
PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress Ioi Lam has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8622/files - new: https://git.openjdk.java.net/jdk/pull/8622/files/22c22c30..34a01f71 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8622&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8622&range=00-01 Stats: 14 lines in 5 files changed: 1 ins; 7 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8622.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622 PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() [v2]
On Tue, 10 May 2022 21:43:44 GMT, Claes Redestad wrote: >> Ioi Lam has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review comments > > src/hotspot/os/windows/perfMemory_windows.cpp line 1781: > >> 1779: // address space. >> 1780: // >> 1781: void PerfMemory::attach(const char* user, int vmid, > > One line? Fixed > src/hotspot/share/prims/perf.cpp line 84: > >> 82: >> 83: // attach to the PerfData memory region for the specified VM >> 84: PerfMemory::attach(user_utf, vmid, > > One line? Fixed > src/hotspot/share/runtime/perfMemory.hpp line 146: > >> 144: // methods for attaching to and detaching from the PerfData >> 145: // memory segment of another JVM process on the same system. >> 146: static void attach(const char* user, int vmid, > > One line? Fixed > src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74: > >> 72: Integer v = lvmid; >> 73: RemoteVm stub = null; >> 74: StringBuilder sb = new StringBuilder(); > > Suggestion: > > String vmidStr = "local://" + lvmid + "@localhost"; Fixed - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress Nice cleanup! (Some stylistic suggestions inline, feel free to ignore) src/hotspot/os/windows/perfMemory_windows.cpp line 1781: > 1779: // address space. > 1780: // > 1781: void PerfMemory::attach(const char* user, int vmid, One line? src/hotspot/share/prims/perf.cpp line 84: > 82: > 83: // attach to the PerfData memory region for the specified VM > 84: PerfMemory::attach(user_utf, vmid, One line? src/hotspot/share/runtime/perfMemory.hpp line 146: > 144: // methods for attaching to and detaching from the PerfData > 145: // memory segment of another JVM process on the same system. > 146: static void attach(const char* user, int vmid, One line? src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java line 74: > 72: Integer v = lvmid; > 73: RemoteVm stub = null; > 74: StringBuilder sb = new StringBuilder(); Suggestion: String vmidStr = "local://" + lvmid + "@localhost"; - Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote: > The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never > supported the value `"rw"` since the source code was imported to the openjdk > repo more than 15 years ago. In fact HotSpot throws > `IllegalArgumentException` when such a mode is specified. > > It's unlikely such a mode will be required for future enhancements. Support > for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` > is the only supported mode. > > I also cleaned up related code in the JDK and HotSpot. > > Testing: > - Passed tiers 1 and 2 > - Tiers 3, 4, 5 are in progress I skimmed through the changes and I think they look okay. In the distant past there were tools outside of the JDK that used the jvmstat API directly. It's possible that VisualVM still does but it would only compile/run if --add-exports is used to export the sun.jvmstat.* packages. So it might be that dropping the parameter from a method in RemoteHost is noticed and I think that is okay because this package is not exported and is not meant to be used by code outside of the JDK. - PR: https://git.openjdk.java.net/jdk/pull/8622
Re: RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
On Tue, 10 May 2022 04:00:29 GMT, Ioi Lam wrote:
> The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never
> supported the value `"rw"` since the source code was imported to the openjdk
> repo more than 15 years ago. In fact HotSpot throws
> `IllegalArgumentException` when such a mode is specified.
>
> It's unlikely such a mode will be required for future enhancements. Support
> for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"`
> is the only supported mode.
>
> I also cleaned up related code in the JDK and HotSpot.
>
> Testing:
> - Passed tiers 1 and 2
> - Tiers 3, 4, 5 are in progress
src/jdk.internal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java
line 60:
> 58: FileChannel fc = new RandomAccessFile(f, "r").getChannel();
> 59: ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L,
> (int)fc.size());
> 60: fc.close(); // doesn't need to remain open
I think you can change this to:
try (FileChannel fc = FileChannel.open(f.toPath())) {
ByteBuffer bb = ...
createPerfDataBuffer(bb, 0);
}
-
PR: https://git.openjdk.java.net/jdk/pull/8622
RFR: 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()
The `mode` parameter for ` jdk.internal.perf.Perf.attach()` has never supported the value `"rw"` since the source code was imported to the openjdk repo more than 15 years ago. In fact HotSpot throws `IllegalArgumentException` when such a mode is specified. It's unlikely such a mode will be required for future enhancements. Support for `"rw"` is removed. The "mode" parameter is also removed, since now `"r"` is the only supported mode. I also cleaned up related code in the JDK and HotSpot. Testing: - Passed tiers 1 and 2 - Tiers 3, 4, 5 are in progress - Commit messages: - 8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() Changes: https://git.openjdk.java.net/jdk/pull/8622/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8622&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286441 Stats: 193 lines in 15 files changed: 0 ins; 144 del; 49 mod Patch: https://git.openjdk.java.net/jdk/pull/8622.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622 PR: https://git.openjdk.java.net/jdk/pull/8622
