8299518: HotSpotVirtualMachine shared code across different platforms #11823

y1yang0 · 2023-01-03T09:34:55Z

harmless refactor to share code across different platforms of VirtualMachineImpl:

Shared code to process command response after requesting a command execution
Read functionality in SocketInputStream can be reused

Progress

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

Issue

JDK-8299518: HotSpotVirtualMachine shared code across different platforms

Reviewers

Chris Plummer (@plummercj - Reviewer) ⚠️ Review applies to 6033fcc4
David Holmes (@dholmes-ora - Reviewer) ⚠️ Review applies to 6033fcc4

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11823/head:pull/11823
$ git checkout pull/11823

Update a local copy of the PR:
$ git checkout pull/11823
$ git pull https://git.openjdk.org/jdk pull/11823/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11823

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11823.diff

bridgekeeper · 2023-01-03T09:36:44Z

👋 Welcome back yyang! 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 · 2023-01-03T09:40:19Z

@y1yang0 The following label will be automatically applied to this pull request:

serviceability

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

mlbridge · 2023-01-03T09:43:37Z

Webrevs

dholmes-ora · 2023-01-04T02:04:45Z

There are a lot of stylistic changes here that have nothing to do with refactoring or code sharing and they make the real changes very hard to see. At a minimum things like variable renaming should be done in a separate commit, so the refactoring is more obvious, but it may be better to use a separate cleanup RFE.

dholmes-ora · 2023-01-04T02:05:38Z

src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java

-            }
-        } catch (NumberFormatException x) {
-            throw new AttachNotSupportedException("Invalid process identifier: " + vmid);
+        int pid = Integer.parseInt(vmid);


This will now throw NumberFormatException instead of the expected AttachNotSupportedException.

The same check is performed in its superclass HotSpotVirtualMachine. So we can be sure that it will not throw NumberFormatException and AttachNotSupportedException here

jdk/src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

Lines 69 to 74 in 38cfc59

int pid;

try {

pid = Integer.parseInt(id);

} catch (NumberFormatException e) {

throw new AttachNotSupportedException("Invalid process identifier");

}

In that case the int pid determined by the superclass should be made available to the subclasses so they can use it and not need to re-parse the vmid.

In that case the int pid determined by the superclass should be made available to the subclasses so they can use it and not need to re-parse the vmid.

Hi @dholmes-ora, this is feasible, but in fact, the super class of VirtualMachine has already saved an unresolved pid(String) and corresponding getter method, so if we want to avoid re-parsing pid, we should save this parsed result in the parent class of the parent class of VirtualMachineImpl, and then modify the corresponding method, which seems to be beyond the scope of simple refactoring, so I hope avoiding re-parsing can be done by another PR

It is well within scope of improving code sharing in this arera, but I'm okay with leaving that for a next step.

openjdk · 2023-01-04T09:18:01Z

@y1yang0 Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

dholmes-ora · 2023-01-05T02:35:54Z

src/jdk.attach/linux/classes/sun/tools/attach/VirtualMachineImpl.java

-
-        public SocketInputStream(int s) {
-            this.s = s;
+    private static class SocketInputStreamImpl extends SocketInputStream {


Can this class definition also be shared by making it a protected nested class in the superclass?

Sorry I see it already is, but I think we can do better.

This(SocketInputStreamImpl) is implementation-specific, it calls platform-dependent VirtualMachine.xxx.

dholmes-ora · 2023-01-05T02:36:59Z

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

@@ -183,6 +184,8 @@ public void loadAgent(String agent, String options)
    private static final int ATTACH_ERROR_NOTONCP       = 101;
    private static final int ATTACH_ERROR_STARTFAIL     = 102;

+    // known error
+    private static final int ATTACH_ERROR_BADVERSION = 101;


It doesn't look right that this has the same value as ATTACH_ERROR_NOTONCP

They are located in different error families. The former is to deal with agent_load and the latter is to deal with command execution version errors.

dholmes-ora · 2023-01-05T02:37:45Z

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

@@ -366,6 +369,87 @@ String readErrorMessage(InputStream in) throws IOException {
        return message.toString();
    }

+    void processCompletionStatus(IOException ioe, String cmd, InputStream sis) throws AgentLoadException, IOException {


Some doc comments for this method would be good.

I was thinking more about describing what the parameters are - in particular it is unclear why the actual IOE can be replaced by a passed in one. If we wanted a custom message then I would at least expect to chain the actual IOE to the replacement.

I was thinking more about describing what the parameters are - in particular it is unclear why the actual IOE can be replaced by a passed in one. If we wanted a custom message then I would at least expect to chain the actual IOE to the replacement.

Done. Parameter ioe means if we get IOE during previous command execution, delay throwing it until command execution completion status has been read. Since all other parameters are straightforward and this is a private method, I don't write strandard @param descriptions for them.

dholmes-ora · 2023-01-05T02:42:48Z

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

+        protected abstract int readImpl(long fd, byte[] bs, int off, int len) throws IOException;
+        protected abstract void closeImpl(long fd) throws IOException;


If the subclasses all override these in exactly the same way then these do not need to be abstract and can simply delegate to VirtualMachineImpl.xxx

All Posix OS platforms call the same VirtualMachineImpl.read/write/etc, but Windows is an unusual guy, it calls VirtualMachineImpl.readPipe/writePipe/etc

That is a shame, though perhaps we could just rename those methods on Windows?

This also raises the obvious question can we in fact reduce this to just a Windows and Posix version, or are there other differences between Linux, macOS etc that have to be accounted for? Though in that case we could introduce a shared superclass for the Posix platforms. Next steps perhaps?

The remaining code for the POSIX system is exactly the same, except that Linux will make an extra attempt to find java_pid in cgroup environment.

sspitsyn · 2023-01-05T03:01:22Z

I like the approach in general.
Also, I agree with David on his comments, especially on the renaming.
The abstract methods readImpl() and closeImpl() is better to name as read() and close().

dholmes-ora · 2023-01-05T04:31:16Z

src/jdk.attach/windows/classes/sun/tools/attach/VirtualMachineImpl.java

+        protected int read(long fd, byte[] bs, int off, int len) throws IOException {
            return VirtualMachineImpl.readPipe(fd, bs, off, len);


If you add read/close methods for Windows that then call readPipe/closePipe, then we don't need to subclass and override.

Good suggestion.
However, I wonder why the fd is casted from long to int on Unix. The Unix versions of SocketInputStream had int fd.
So, the following code also needs to cast fd to int:

+ public synchronized int read(byte[] bs, int off, int len) throws IOException { + if ((off < 0) || (off > bs.length) || (len < 0) || + ((off + len) > bs.length) || ((off + len) < 0)) { + throw new IndexOutOfBoundsException(); + } else if (len == 0) { + return 0; + } + return read(fd, bs, off, len); <== ??? + } + + public synchronized void close() throws IOException { + if (fd != -1) { + close(fd); <== ??? + }

Looks like Windows needs a long fd so the shared API takes long and we then cast to int for the Unix native methods. A little messy but fixable.

If you add read/close methods for Windows that then call readPipe/closePipe, then we don't need to subclass and override.

Sorry, I don't understand what this means. VirtualMachineImpl is platform-dependent while HotSpotVirtualMachine is shared, we always need to sub-class socket input stream in order to call platform-depend read/close.

As long as all the platform-specific VirtualMachineImpl classes define a read and close method then we can simply have the shared socket stream class call those methods. Windows will need an extra level of indirection because its read method will have to call readPipe.

As long as all the platform-specific VirtualMachineImpl classes define a read and close method then we can simply have the shared socket stream class call those methods. Windows will need an extra level of indirection because its read method will have to call readPipe.

/path/src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java:444: error: incompatible types: possible lossy conversion from long to int return VirtualMachineImpl.read(fd, bs, off, len); ^ /path/src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java:449: error: incompatible types: possible lossy conversion from long to int VirtualMachineImpl.close(fd);

Unfortunately, this causes compilation error because we have long fd in shard socket stream and read(int fd...)/readPipe(long fd) in VirtualMachineImpl, we need subclasses to determine whether a type cast is necessary

That is unfortunate.

plummercj · 2023-01-06T22:01:45Z

I would also like to request that the variable renames be omitted from this PR. They create too much noise and would best left to a PR that focuses on just the renames and possibly other style changes.

This reverts commit cb70000.

y1yang0 · 2023-01-09T03:25:08Z

I would also like to request that the variable renames be omitted from this PR. They create too much noise and would best left to a PR that focuses on just the renames and possibly other style changes.

Okay, reverted.

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

…chine.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

plummercj · 2023-01-09T20:40:58Z

src/jdk.attach/aix/classes/sun/tools/attach/VirtualMachineImpl.java

+        if (pid < 1) {
+            throw new AttachNotSupportedException("Invalid process identifier -1");


Why the change to using -1 instead of vmid? It seems presumptuous that it is -1, and also seems less useful than printing vmid.

…jdk_virtualmachienimpl

openjdk · 2023-01-10T10:44:33Z

⚠️ @y1yang0 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

plummercj · 2023-01-11T19:13:20Z

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

+            if (fd != -1) {
+                close(fd);
+            }


There used to be logic to set fd (previously called s) to -1 during the close operation. Is that no longer needed?

Hi @plummercj , sorry for late reply.

I don't see why we need it because this method is protected by synchronized. fd is only set by ctor.

You will end up leaving fd set even after the close completes. A subsequent close (which I assume should never happen) would end up with an IOException. Previously close would end up not doing anything when called again, and simply return with no error. I'm not sure which is the best behavior in this case, but it is a change in behavior.

bridgekeeper · 2023-02-09T01:04:16Z

@y1yang0 This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2023-02-28T19:19:17Z

@y1yang0 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:

8299518: HotSpotVirtualMachine shared code across different platforms

Reviewed-by: cjplummer, dholmes

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 1283 new commits pushed to the master branch:

c961a91: 8143900: OptimizeStringConcat has an opaque dependency on Integer.sizeTable field
35003b5: 8302830: AArch64: Fix the mismatch between cas.m4 and aarch64.ad
f3abc40: 8302783: Improve CRC32C intrinsic with crypto pmull on AArch64
45d8a17: 8303405: fix @returnss typo in ReflectionFactory
e7113dc: 8302495: update for deprecated sprintf for java.desktop
843d632: 8303442: Clean up w2k_lsa_auth linker parameters
d4dcba0: 8303267: Prefer ArrayList to LinkedList in ConcurrentLocksPrinter
2c7d2c0: 8303411: JFR problem list entry for JDK-8247776 should be removed
0b63557: 8303039: Utilize coverageLevels.txt
32247c3: 8303476: Add the runtime version in the release file of a JDK image
... and 1273 more: https://git.openjdk.org/jdk/compare/11ba7591dfd3f7ca58e2e1ac6d1b3e81391f5bfb...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.

y1yang0 · 2023-03-03T02:08:49Z

@sspitsyn @dholmes-ora @turbanoff May I ask your help to review this patch? Thanks.

dholmes-ora

Nothing further from me.

Thanks.

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java

y1yang0 · 2023-03-06T02:01:22Z

/integrate

openjdk · 2023-03-06T02:02:37Z

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

148900c: 8303562: Remove obsolete comments in os::pd_attempt_reserve_memory_at
1bb39a9: 8302027: Port fdlibm trig functions (sin, cos, tan) to Java
9fdbf3c: 8303588: [JVMCI] make JVMCI source directories conform with standard layout
629a905: 8303242: ThreadMXBean issues with virtual threads
5b2e2e4: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
cd4b88d: 8292269: Replace FileMapInfo::fail_continue() with Unified Logging
ae29054: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal
a04b104: 8303413: (fs) Ignore polling interval sensitivity modifiers in PollingWatchService
9944314: 8303587: Remove VMOutOfMemoryError001 test from the problem list after 8303198
a50dc67: 8303527: update for deprecated sprintf for jdk.hotspot.agent
... and 1299 more: https://git.openjdk.org/jdk/compare/11ba7591dfd3f7ca58e2e1ac6d1b3e81391f5bfb...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-03-06T02:02:47Z

@y1yang0 Pushed as commit 10d6a8e.

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

openjdk bot added the rfr Pull request is ready for review label Jan 3, 2023

openjdk bot added the serviceability serviceability-dev@openjdk.org label Jan 3, 2023

dholmes-ora reviewed Jan 4, 2023

View reviewed changes

y1yang0 added 2 commits January 4, 2023 17:06

8299518: HotSpotVirtualMachine shared code across different platforms

cfa2701

separate renaming

cb70000

y1yang0 force-pushed the jdk_virtualmachienimpl branch from 293cccd to cb70000 Compare January 4, 2023 09:16

dholmes-ora reviewed Jan 5, 2023

View reviewed changes

add doc; readImpl/closeImpl to read/close

0580ab2

dholmes-ora reviewed Jan 5, 2023

View reviewed changes

y1yang0 added 2 commits January 9, 2023 11:17

Revert "separate renaming"

24af50a

This reverts commit cb70000.

describe parameter

e9d8c36

turbanoff reviewed Jan 9, 2023

View reviewed changes

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java Outdated Show resolved Hide resolved

Update src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMa…

9d6aa99

…chine.java Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>

plummercj reviewed Jan 9, 2023

View reviewed changes

y1yang0 added 2 commits January 10, 2023 18:42

-1 to vmid

08250f9

Merge branch 'jdk_virtualmachienimpl' of github.com:y1yang0/jdk into …

42934c0

…jdk_virtualmachienimpl

format

6077ae5

plummercj reviewed Jan 11, 2023

View reviewed changes

fd set -1

6033fcc

plummercj approved these changes Feb 28, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 28, 2023

y1yang0 requested review from dholmes-ora and sspitsyn and removed request for dholmes-ora March 3, 2023 02:07

dholmes-ora approved these changes Mar 3, 2023

View reviewed changes

src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java Outdated Show resolved Hide resolved

have/has

5dabc62

openjdk bot added the integrated Pull request has been integrated label Mar 6, 2023

openjdk bot closed this Mar 6, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 6, 2023

	int pid;
	try {
	pid = Integer.parseInt(id);
	} catch (NumberFormatException e) {
	throw new AttachNotSupportedException("Invalid process identifier");
	}

		protected abstract int readImpl(long fd, byte[] bs, int off, int len) throws IOException;
		protected abstract void closeImpl(long fd) throws IOException;

		protected int read(long fd, byte[] bs, int off, int len) throws IOException {
		return VirtualMachineImpl.readPipe(fd, bs, off, len);

		if (pid < 1) {
		throw new AttachNotSupportedException("Invalid process identifier -1");

8299518: HotSpotVirtualMachine shared code across different platforms #11823

8299518: HotSpotVirtualMachine shared code across different platforms #11823

Conversation

y1yang0 commented Jan 3, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 3, 2023

openjdk bot commented Jan 3, 2023

mlbridge bot commented Jan 3, 2023 • edited Loading

Webrevs

dholmes-ora commented Jan 4, 2023

Choose a reason for hiding this comment

y1yang0 Jan 4, 2023 • edited Loading

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 Jan 4, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sspitsyn commented Jan 5, 2023

Choose a reason for hiding this comment

sspitsyn Jan 5, 2023 • edited Loading

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

y1yang0 Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plummercj commented Jan 6, 2023

y1yang0 commented Jan 9, 2023

Choose a reason for hiding this comment

openjdk bot commented Jan 10, 2023

Choose a reason for hiding this comment

y1yang0 Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridgekeeper bot commented Feb 9, 2023

openjdk bot commented Feb 28, 2023 • edited Loading

y1yang0 commented Mar 3, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

y1yang0 commented Mar 6, 2023

openjdk bot commented Mar 6, 2023

openjdk bot commented Mar 6, 2023

y1yang0 commented Jan 3, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 3, 2023 •

edited

Loading

y1yang0 Jan 4, 2023 •

edited

Loading

sspitsyn Jan 5, 2023 •

edited

Loading

y1yang0 Jan 6, 2023 •

edited

Loading

y1yang0 Feb 15, 2023 •

edited

Loading

openjdk bot commented Feb 28, 2023 •

edited

Loading