8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() #8622

iklam · 2022-05-10T04:00:29Z

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

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (1 review required, with at least 1 reviewer)

Issue

JDK-8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

Reviewers

Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to 22c22c30
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8622/head:pull/8622
$ git checkout pull/8622

Update a local copy of the PR:
$ git checkout pull/8622
$ git pull https://git.openjdk.java.net/jdk pull/8622/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8622

View PR using the GUI difftool:
$ git pr show -t 8622

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8622.diff

bridgekeeper · 2022-05-10T04:01:34Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-05-10T04:03:11Z

@iklam The following labels will be automatically applied to this pull request:

core-libs
hotspot
jmx
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-05-10T04:09:56Z

Webrevs

01: Full - Incremental (34a01f71)
00: Full (22c22c30)

AlanBateman · 2022-05-10T19:59:41Z

...nternal.jvmstat/share/classes/sun/jvmstat/perfdata/monitor/protocol/file/PerfDataBuffer.java

-            }
-
+            FileChannel fc = new RandomAccessFile(f, "r").getChannel();
+            ByteBuffer bb = fc.map(FileChannel.MapMode.READ_ONLY, 0L, (int)fc.size());
            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); }

AlanBateman · 2022-05-10T20:03:45Z

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.

cl4es

Nice cleanup!

(Some stylistic suggestions inline, feel free to ignore)

cl4es · 2022-05-10T21:39:30Z

src/jdk.jstatd/share/classes/sun/tools/jstatd/RemoteHostImpl.java

@@ -67,17 +67,13 @@ public RemoteHostImpl(int rmiPort) throws MonitorException {
        monitoredHost.addHostListener(this);
    }

-    public RemoteVm attachVm(int lvmid, String mode)
+    public RemoteVm attachVm(int lvmid)
                    throws RemoteException, MonitorException {
        Integer v = lvmid;
        RemoteVm stub = null;
        StringBuilder sb = new StringBuilder();


Suggested change

StringBuilder sb = new StringBuilder();

String vmidStr = "local://" + lvmid + "@localhost";

cl4es · 2022-05-10T21:41:47Z

src/hotspot/share/runtime/perfMemory.hpp

@@ -148,7 +143,7 @@ class PerfMemory : AllStatic {

    // methods for attaching to and detaching from the PerfData
    // memory segment of another JVM process on the same system.
-    static void attach(const char* user, int vmid, PerfMemoryMode mode,
+    static void attach(const char* user, int vmid,


cl4es · 2022-05-10T21:43:35Z

src/hotspot/share/prims/perf.cpp

  // attach to the PerfData memory region for the specified VM
-  PerfMemory::attach(user_utf, vmid, (PerfMemory::PerfMemoryMode) mode,
+  PerfMemory::attach(user_utf, vmid,


cl4es · 2022-05-10T21:43:44Z

src/hotspot/os/windows/perfMemory_windows.cpp

@@ -1796,7 +1778,7 @@ void PerfMemory::delete_memory_region() {
 // the indicated process's PerfData memory region into this JVMs
 // address space.
 //
-void PerfMemory::attach(const char* user, int vmid, PerfMemoryMode mode,
+void PerfMemory::attach(const char* user, int vmid,


openjdk · 2022-05-10T21:45:41Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

Reviewed-by: redestad, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 48 new commits pushed to the master branch:

46a775a: 8286540: Build failure caused by missing DefaultPollerProvider implementation on AIX
4ad8cfa: 8286002: Add support for intel syntax to capstone hsdis
cec23b1: 8286556: Remove EagerInitialization develop option
89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
9ac52b0: 8286392: Address possibly lossy conversions in jdk.jfr
f628966: 8286541: JFR: RecordingFile.write is missing "since 19"
f1554fc: 8285872: JFR: Remove finalize() methods
... and 38 more: https://git.openjdk.java.net/jdk/compare/54e33082105dcbcfc795839c954f6e63402edff1...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iklam · 2022-05-11T02:49:14Z

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.

iklam · 2022-05-11T19:25:27Z

Thanks to @AlanBateman and @cl4es for the review.
/integrate

openjdk · 2022-05-11T19:27:08Z

Going to push as commit fcf49f4.
Since your change was applied there have been 48 commits pushed to the master branch:

46a775a: 8286540: Build failure caused by missing DefaultPollerProvider implementation on AIX
4ad8cfa: 8286002: Add support for intel syntax to capstone hsdis
cec23b1: 8286556: Remove EagerInitialization develop option
89de756: 8286387: Remove unused FreeListAllocator::reduce_free_list
ae695d6: 8286476: x86_32: Fix crashes with non-preview mode after JDK-8284161 (Virtual Threads)
87f3d2b: 8286446: PPC64: fix crashes after JDK-8284161 (virtual threads preview)
7a2bbbb: 8286396: Address possibly lossy conversions in jdk.management.jfr
9ac52b0: 8286392: Address possibly lossy conversions in jdk.jfr
f628966: 8286541: JFR: RecordingFile.write is missing "since 19"
f1554fc: 8285872: JFR: Remove finalize() methods
... and 38 more: https://git.openjdk.java.net/jdk/compare/54e33082105dcbcfc795839c954f6e63402edff1...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-05-11T19:27:25Z

@iklam Pushed as commit fcf49f4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach()

22c22c3

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 10, 2022

iklam marked this pull request as ready for review May 10, 2022 04:04

openjdk bot added the rfr Pull request is ready for review label May 10, 2022

AlanBateman reviewed May 10, 2022

View reviewed changes

cl4es approved these changes May 10, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 10, 2022

review comments

34a01f7

AlanBateman approved these changes May 11, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 11, 2022

openjdk bot closed this May 11, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() #8622

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() #8622

iklam commented May 10, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 10, 2022

openjdk bot commented May 10, 2022

mlbridge bot commented May 10, 2022 •

edited

Loading

AlanBateman May 10, 2022

iklam May 11, 2022

AlanBateman commented May 10, 2022

cl4es left a comment

cl4es May 10, 2022

iklam May 11, 2022

cl4es May 10, 2022

iklam May 11, 2022

cl4es May 10, 2022

iklam May 11, 2022

cl4es May 10, 2022

iklam May 11, 2022

openjdk bot commented May 10, 2022 •

edited

Loading

iklam commented May 11, 2022

iklam commented May 11, 2022

openjdk bot commented May 11, 2022

openjdk bot commented May 11, 2022

	StringBuilder sb = new StringBuilder();
	String vmidStr = "local://" + lvmid + "@localhost";

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() #8622

8286441: Remove mode parameter from jdk.internal.perf.Perf.attach() #8622

Conversation

iklam commented May 10, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 10, 2022

openjdk bot commented May 10, 2022

mlbridge bot commented May 10, 2022 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanBateman commented May 10, 2022

cl4es left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented May 10, 2022 • edited Loading

iklam commented May 11, 2022

iklam commented May 11, 2022

openjdk bot commented May 11, 2022

openjdk bot commented May 11, 2022

iklam commented May 10, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented May 10, 2022 •

edited

Loading

openjdk bot commented May 10, 2022 •

edited

Loading