8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions #13484

sspitsyn · 2023-04-14T22:01:23Z

This refactoring to separate ThreadStart/ThreadEnd events posting code in the JVMTI VTMS transitions is needed for future work on JVMTI scalability and performance improvements. It is to easier put this code on slow path.

Testing: mach5 tiers 1-6 were successful.

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

Issues

JDK-8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions
JDK-8304444: Reappearance of NULL in jvmtiThreadState.cpp

Reviewers

Patricio Chilano Mateo (@pchilano - Reviewer) ⚠️ Review applies to debe49c3
Leonid Mesnik (@lmesnik - Reviewer) ⚠️ Review applies to 157f33af

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13484

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

Using diff file

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

Webrev

Link to Webrev Comment

…VTMS transitions

bridgekeeper · 2023-04-14T22:02:09Z

👋 Welcome back sspitsyn! 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-04-14T22:03:55Z

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

hotspot
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 · 2023-04-14T22:07:26Z

Webrevs

openjdk · 2023-04-15T00:23:20Z

@sspitsyn This issue is referenced in the PR title - it will now be updated.

sspitsyn · 2023-04-15T00:29:36Z

/issue add 8304444: Reappearance of NULL in jvmtiThreadState.cpp

openjdk · 2023-04-15T00:30:09Z

@sspitsyn Command syntax:

/issue [add|remove] <id>[,<id>,...]
/issue [add] <id>: <description>
`/issue create [PX] /[subcomponent]

Some examples:

/issue add JDK-1234567,4567890
/issue remove JDK-4567890
/issue 1234567: Use this exact title
`/issue create hotspot/jfr
`/issue create P4 core-libs/java.nio

If issues are specified only by their ID, the title will be automatically retrieved from JBS. The project prefix (JDK- in the above examples) is optional. Separate multiple issue IDs using either spaces or commas.

sspitsyn · 2023-04-25T03:45:08Z

/issue add 8304444

openjdk · 2023-04-25T03:46:36Z

@sspitsyn
Adding additional issue to issue list: 8304444: Reappearance of NULL in jvmtiThreadState.cpp.

openjdk · 2023-04-27T03:28:30Z

@sspitsyn this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout br29
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

sspitsyn · 2023-04-27T03:38:39Z

Added update with refactoring prepared by @pchilano .
This update includes some renaming to make function names more consistent.
The mach5 runs of tiers 1-6 are all passed.

merge with branch29

openjdk · 2023-04-27T04:36:00Z

⚠️ @sspitsyn 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).

pchilano

Hi Serguei,

Changes look good to me. Thanks for taking care of the refactoring.

Patricio

pchilano · 2023-05-01T17:02:04Z

src/hotspot/share/runtime/sharedRuntime.cpp

@@ -636,27 +636,27 @@ JRT_ENTRY(void, SharedRuntime::notify_jvmti_object_alloc(oopDesc* o, JavaThread*
  current->set_vm_result(h());
 JRT_END

-JRT_ENTRY(void, SharedRuntime::notify_jvmti_mount(oopDesc* vt, jboolean hide, jboolean first_mount, JavaThread* current))
+JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, jboolean dummy, JavaThread* current))


Maybe rename dummy to hide and just assert is false in this case and true for the vthread_end case?

Good suggestion. Thank you.

openjdk · 2023-05-01T17:07:27Z

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

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions
8304444: Reappearance of NULL in jvmtiThreadState.cpp

Reviewed-by: pchilanomate, lmesnik

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

sspitsyn · 2023-05-01T21:07:12Z

Patricio, thank you a lot for review!

lmesnik

Please update copyrights, at leas in symbols-unix.

sspitsyn · 2023-05-02T01:04:45Z

Leonid, thank you a lot for review!

sspitsyn · 2023-05-02T01:56:19Z

Please update copyrights, at leas in symbols-unix.

Done.

sspitsyn · 2023-05-02T02:39:11Z

/integrate

openjdk · 2023-05-02T02:40:59Z

Going to push as commit 1227a27.

openjdk · 2023-05-02T02:41:14Z

@sspitsyn Pushed as commit 1227a27.

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

dougxc · 2023-06-08T19:05:54Z

src/hotspot/share/runtime/sharedRuntime.cpp

@@ -636,27 +636,29 @@ JRT_ENTRY(void, SharedRuntime::notify_jvmti_object_alloc(oopDesc* o, JavaThread*
  current->set_vm_result(h());
 JRT_END

-JRT_ENTRY(void, SharedRuntime::notify_jvmti_mount(oopDesc* vt, jboolean hide, jboolean first_mount, JavaThread* current))
+JRT_ENTRY(void, SharedRuntime::notify_jvmti_vthread_start(oopDesc* vt, jboolean hide, JavaThread* current))
+  assert(hide == JNI_FALSE, "must be VTMS transition finish");
  jobject vthread = JNIHandles::make_local(const_cast<oopDesc*>(vt));


Since the current thread is in the current arg, it could be used here when creating the local handle.

That's right. Thanks.

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI …

1f042a1

…VTMS transitions

openjdk bot added the rfr Pull request is ready for review label Apr 14, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 14, 2023

8304444: Reappearance of NULL in jvmtiThreadState.cpp

7735ffa

sspitsyn changed the title ~~8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions~~ 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions, 8306028: Reappearance of NULL in jvmtiThreadState.cpp Apr 15, 2023

sspitsyn changed the title ~~8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions, 8306028: Reappearance of NULL in jvmtiThreadState.cpp~~ 8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions Apr 15, 2023

Merge

5594635

do more refactoring including VirtualThread class

77be444

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 27, 2023

Merge branch 'master' into br29

639b211

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Apr 27, 2023

sspitsyn added 2 commits April 27, 2023 04:30

move code a little bit

158c999

Merge branch 'br29' of https://github.com/sspitsyn/jdk into br29

debe49c

merge with branch29

pchilano approved these changes May 1, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 1, 2023

addressed review comment: add a couple of asserts

157f33a

lmesnik approved these changes May 2, 2023

View reviewed changes

sspitsyn added 3 commits May 2, 2023 01:45

minor correction in sharedRuntime.cpp

5fa417b

Merge

02b2760

update copyright comments

f4227c7

openjdk bot added the integrated Pull request has been integrated label May 2, 2023

openjdk bot closed this May 2, 2023

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

sspitsyn deleted the br29 branch May 2, 2023 02:45

dougxc reviewed Jun 8, 2023

View reviewed changes

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions #13484

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions #13484

sspitsyn commented Apr 14, 2023 •

edited by openjdk bot

bridgekeeper bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

mlbridge bot commented Apr 14, 2023 •

edited

openjdk bot commented Apr 15, 2023

sspitsyn commented Apr 15, 2023 •

edited

openjdk bot commented Apr 15, 2023

sspitsyn commented Apr 25, 2023

openjdk bot commented Apr 25, 2023

openjdk bot commented Apr 27, 2023

sspitsyn commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

pchilano left a comment

pchilano May 1, 2023

sspitsyn May 1, 2023

openjdk bot commented May 1, 2023 •

edited

sspitsyn commented May 1, 2023

lmesnik left a comment

sspitsyn commented May 2, 2023

sspitsyn commented May 2, 2023

sspitsyn commented May 2, 2023

openjdk bot commented May 2, 2023

openjdk bot commented May 2, 2023

dougxc Jun 8, 2023

sspitsyn Jun 9, 2023

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions #13484

8306028: separate ThreadStart/ThreadEnd events posting code in JVMTI VTMS transitions #13484

Conversation

sspitsyn commented Apr 14, 2023 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 14, 2023

openjdk bot commented Apr 14, 2023

mlbridge bot commented Apr 14, 2023 • edited

Webrevs

openjdk bot commented Apr 15, 2023

sspitsyn commented Apr 15, 2023 • edited

openjdk bot commented Apr 15, 2023

sspitsyn commented Apr 25, 2023

openjdk bot commented Apr 25, 2023

openjdk bot commented Apr 27, 2023

sspitsyn commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

pchilano left a comment

Choose a reason for hiding this comment

pchilano May 1, 2023

Choose a reason for hiding this comment

sspitsyn May 1, 2023

Choose a reason for hiding this comment

openjdk bot commented May 1, 2023 • edited

sspitsyn commented May 1, 2023

lmesnik left a comment

Choose a reason for hiding this comment

sspitsyn commented May 2, 2023

sspitsyn commented May 2, 2023

sspitsyn commented May 2, 2023

openjdk bot commented May 2, 2023

openjdk bot commented May 2, 2023

dougxc Jun 8, 2023

Choose a reason for hiding this comment

sspitsyn Jun 9, 2023

Choose a reason for hiding this comment

sspitsyn commented Apr 14, 2023 •

edited by openjdk bot

mlbridge bot commented Apr 14, 2023 •

edited

sspitsyn commented Apr 15, 2023 •

edited

openjdk bot commented May 1, 2023 •

edited