8336845: [lworld] Virtual threads don't support the value class calling convention #1556

pchilano · 2025-09-09T12:50:49Z

Please review the following patch which adds virtual thread support for the value class calling convention. Most of the changes are needed to handle extended frames, i.e c1 or c2 frames that use more stack space for arguments than the stack space allocated by the caller. These include changes in freeze and thaw code, plus changes in the stackChunk walking code where we now need a similar repair of the caller sp as in the current sender code. In this case though, we only adjust _unextended_sp and keep the value of _sp. This is because frames are walked without a full RegisterMap, but we still need to be able to access the saved fp in the callee for gc purposes. The remaining changes deal with saving and restoring the extra return registers when calling thaw.

Thanks to Tobias for working on the initial changes and for providing the very useful new test TestVirtualThreads.java, included in this PR, which has been great for catching many bugs. I also run the changes with the valhalla-comp-stress job in mach5. It uncovered a couple of extra failures in TestVirtualThreads.java, but I was able to reproduce them with platform threads as well (8367151 and 8367258). I also added extra testing for value classes in existing test Fuzz.java which has proven very useful too.

Thanks,
Patricio

Progress

Change must not contain extraneous whitespace

Issue

JDK-8336845: [lworld] Virtual threads don't support the value class calling convention (Bug - P2)

Reviewers

Tobias Hartmann (@TobiHartmann - Committer) ⚠️ Review applies to 39669832
Coleen Phillimore (@coleenp - Committer)

Contributors

Tobias Hartmann <[email protected]>

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1556

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1556.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-09-09T12:52:00Z

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

openjdk · 2025-09-09T12:54:03Z

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

8336845: [lworld] Virtual threads don't support the value class calling convention

Co-authored-by: Tobias Hartmann <[email protected]>
Reviewed-by: thartmann, coleenp

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 255 new commits pushed to the lworld branch:

0836ed5: 8328777: [lworld] TestSyncOnValueBasedClassEvent.java fails with Exception
446d7d5: 8294051: [lworld] runtime/AccModule/ConstModule.java fails with incorrect error message
c3bad8f: Merge jdk
... and 252 more: https://git.openjdk.org/valhalla/compare/2fddd34c37cfe7c4d4372217fa4b58bf199aa71d...lworld

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@TobiHartmann, @coleenp) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

pchilano · 2025-09-09T13:48:16Z

/contributor add @TobiHartmann

openjdk · 2025-09-09T13:50:28Z

@pchilano
Contributor Tobias Hartmann <[email protected]> successfully added.

mlbridge · 2025-09-09T15:13:30Z

Webrevs

01: Full - Incremental (e8759568)
00: Full (39669832)

TobiHartmann

I'm not an expert in this code but the changes look all good to me. I know this was a lot of effort, great work Patricio!

pchilano · 2025-09-11T13:00:26Z

Thanks Tobias!
/integrate

openjdk · 2025-09-11T13:04:13Z

@pchilano
Your change (at version 3966983) is now ready to be sponsored by a Committer.

coleenp · 2025-09-11T13:53:29Z

src/hotspot/cpu/x86/stackChunkFrameStream_x86.inline.hpp

+                        RegisterMap::ProcessFrames::skip,
+                        RegisterMap::WalkContinuation::skip);
+        frame caller = to_frame().sender(&map);
+        assert(caller.is_compiled_frame() && caller.cb()->as_nmethod()->is_compiled_by_c2(), "");


All these asserts with no messages could be precond(). Or they could have strings why these conditions are expected.

If the c2 frame needs stack repair but was not extended, then caller should be c2 compiled too, i.e. there was no need to extend it. How about adding "needs stack repair but was not extended with c1/interpreter caller"?

I think that sounds good. The strings in these asserts (there are a few that you added) might help with understanding this code.

coleenp

These changes are written generally enough to upstream to mainline so they can also be part of that code. In case mainline has any improvements in this area, this wouldn't be a complicated merge.

coleenp · 2025-09-12T11:41:17Z

/sponsor

openjdk · 2025-09-12T11:43:42Z

@coleenp The PR has been updated since the change author (@pchilano) issued the integrate command - the author must perform this command again.

pchilano · 2025-09-12T14:29:52Z

Thanks Coleen!
/integrate

openjdk · 2025-09-12T14:31:14Z

@pchilano
Your change (at version e875956) is now ready to be sponsored by a Committer.

coleenp · 2025-09-12T14:31:53Z

/sponsor

openjdk · 2025-09-12T14:33:34Z

Going to push as commit 11cee9e.
Since your change was applied there have been 255 commits pushed to the lworld branch:

0836ed5: 8328777: [lworld] TestSyncOnValueBasedClassEvent.java fails with Exception
446d7d5: 8294051: [lworld] runtime/AccModule/ConstModule.java fails with incorrect error message
c3bad8f: Merge jdk
... and 252 more: https://git.openjdk.org/valhalla/compare/2fddd34c37cfe7c4d4372217fa4b58bf199aa71d...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-12T14:33:45Z

@coleenp @pchilano Pushed as commit 11cee9e.

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

v1

3966983

pchilano marked this pull request as ready for review September 9, 2025 15:07

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 9, 2025

TobiHartmann approved these changes Sep 10, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 11, 2025

coleenp reviewed Sep 11, 2025

View reviewed changes

add string to assert in is_in_frame()

e875956

openjdk bot removed the sponsor Pull request is ready to be sponsored label Sep 11, 2025

coleenp approved these changes Sep 12, 2025

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Sep 12, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 12, 2025

openjdk bot closed this Sep 12, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Sep 12, 2025

8336845: [lworld] Virtual threads don't support the value class calling convention #1556

8336845: [lworld] Virtual threads don't support the value class calling convention #1556

Uh oh!

Conversation

pchilano commented Sep 9, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Contributors

Reviewing

Uh oh!

bridgekeeper bot commented Sep 9, 2025

Uh oh!

openjdk bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pchilano commented Sep 9, 2025

Uh oh!

openjdk bot commented Sep 9, 2025

Uh oh!

mlbridge bot commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

pchilano commented Sep 11, 2025

Uh oh!

openjdk bot commented Sep 11, 2025

Uh oh!

coleenp Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

pchilano Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

pchilano Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

coleenp commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

pchilano commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

coleenp commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

openjdk bot commented Sep 12, 2025

Uh oh!

Uh oh!

pchilano commented Sep 9, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Sep 9, 2025 •

edited

Loading

mlbridge bot commented Sep 9, 2025 •

edited

Loading