8370450: [lworld] Alternate implementation of the substitutability test method #1695

fparain · 2025-10-22T17:54:29Z

This is an alternate version of the substitutability method.
To use it, add -Xshare:off -XX:+UseAltSubstitutabilityMethod to the command line of the Valhalla VM in preview mode.

Progress

Change must not contain extraneous whitespace

Issue

JDK-8370450: [lworld] Alternate implementation of the substitutability test method (Bug - P4)

Reviewers

Coleen Phillimore (@coleenp - Committer) ⚠️ Review applies to e9e79fe6
Chen Liang (@liach - Committer) ⚠️ Review applies to e9e79fe6

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1695

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-10-22T17:55:48Z

👋 Welcome back fparain! 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-10-22T17:57:32Z

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

8370450: [lworld] Alternate implementation of the substitutability test method

Reviewed-by: coleenp, liach

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

7b5f105: 8370484: [lworld] PhaseOutput::FillLocArray asserts with Unexpected type: anyptr
88f413d: [lworld] Don't run TestMethodHandles.java with -Xcomp
a6d6cb8: [lworld] Disable assert until JDK-8350208 is fixed in mainline
... and 1 more: https://git.openjdk.org/valhalla/compare/0ff86987f59f127abece39bdb5d9007a291147e9...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.

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

openjdk · 2025-10-22T17:58:03Z

@fparain this pull request can not be integrated into lworld 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 new_acmp
git fetch https://git.openjdk.org/valhalla.git lworld
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge lworld"
git push

mlbridge · 2025-10-22T18:01:02Z

Webrevs

liach · 2025-10-22T20:46:36Z

src/hotspot/share/classfile/classFileParser.cpp

+    // Acmp map required for abstract and concrete value classes
+    FieldInfo::FieldFlags fflags2(0);
+    fflags2.update_injected(true);
+    fflags2.update_stable(true);


If c2 can model the field map read as a stable array read, this allows ValueObjectMethods to generate efficient code for specialized value types in the future. 👍

src/hotspot/share/oops/instanceKlass.hpp

src/hotspot/share/classfile/fieldLayoutBuilder.hpp

src/java.base/share/classes/java/lang/runtime/ValueObjectMethods.java

liach · 2025-10-22T21:52:14Z

src/hotspot/share/runtime/globals.hpp

          "Minimal number of elements in a sorted collection to prefer"     \
          "binary search over simple linear search." )                      \
                                                                            \
+  product(bool, UseAltSubstitutabilityMethod, false,                        \


We might need this flag on/off for core-libs tests in addition to runtime tests.

RogerRiggs · 2025-10-23T20:24:13Z

When comparing the oops, do they need some additional handling?
Supposing for example, they are buffered value objects (from different sources).
It seems necessary to recurse (effectively) with those oops/refs.

coleenp

It's really great work. I have some questions and minor suggested changes.

src/hotspot/share/classfile/classFileParser.cpp

coleenp · 2025-10-23T18:08:00Z

src/hotspot/share/classfile/classFileParser.cpp

  }
+  if (!access_flags().is_identity_class() && !access_flags().is_interface()
+      && _class_name != vmSymbols::java_lang_Object()) {
+    // Acmp map required for abstract and concrete value classes


Thank you for this comment. Explains why abstract classes are omitted from above.

coleenp · 2025-10-23T18:13:38Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+  // The Pair<int,int> values in the nonoop_acmp_map represent <offset,size> segments of memory
+  _nonoop_acmp_map = new GrowableArray<Pair<int,int>>();
+  _oop_acmp_map = new GrowableArray<int>();
+  if (_is_empty_inline_class) return;


Why not return first before allocating these GrowableArray in resource area?

It streamlines the code in InstanceKlass::fill_instance_klass() by not having to handle the null map case.

Okay. I can't decide if you need a comment to prevent someone from changing this to have the quick exit. I guess they'll get a crash for the null case.

src/hotspot/share/interpreter/bytecodeTracer.cpp

src/hotspot/share/prims/unsafe.cpp

src/java.base/share/classes/java/lang/runtime/ValueObjectMethods.java

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

coleenp · 2025-10-23T20:26:54Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+
+      case LayoutRawBlock::REGULAR:
+        {
+          FieldInfo* fi = _field_info->adr_at(b->field_index());


Is there an assumption that the block field indexes are in ascending order?

I'm not sure I understand the question.
LayoutRawBlocks represent the fields layout, they are chained in ascending offset order.
The field_index is the index in the FieldInfo array (from the class file) where this field is declared.

Yes, that was the question. I meant to ask if the offsets are in ascending order, so that makes the code to coalesce them simpler. Can you add a comment at 1367 that the field offsets are in ascending order and that makes reading this easier.

fparain · 2025-10-23T20:47:59Z

When comparing the oops, do they need some additional handling? Supposing for example, they are buffered value objects (from different sources). It seems necessary to recurse (effectively) with those oops/refs.

The recursion is handled in ValueObjectMethods.isSubstitutableAlt() line 1447, the oops are compare using an acmp bytecode which will recurse if both oops are non null and instances of the same value class.

coleenp

I had a couple of comments and a comment request.

coleenp · 2025-10-24T17:34:14Z

src/hotspot/share/classfile/classFileParser.cpp

+    //                           ^               ^
+    //                           |               |
+    //                           --------------------- Pair of integer describing a segment of
+    //                                                 contiguous non-oop fields


Thanks for the ascii art - very helpful.

coleenp · 2025-10-24T17:35:35Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+  // The Pair<int,int> values in the nonoop_acmp_map represent <offset,size> segments of memory
+  _nonoop_acmp_map = new GrowableArray<Pair<int,int>>();
+  _oop_acmp_map = new GrowableArray<int>();
+  if (_is_empty_inline_class) return;


Okay. I can't decide if you need a comment to prevent someone from changing this to have the quick exit. I guess they'll get a crash for the null case.

coleenp · 2025-10-24T17:41:36Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+
+      case LayoutRawBlock::REGULAR:
+        {
+          FieldInfo* fi = _field_info->adr_at(b->field_index());


Yes, that was the question. I meant to ask if the offsets are in ascending order, so that makes the code to coalesce them simpler. Can you add a comment at 1367 that the field offsets are in ascending order and that makes reading this easier.

coleenp · 2025-10-27T18:37:37Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+  return last_idx;
+}
+
+static int insert_map_at_offset(GrowableArray<Pair<int,int>>* nonoop_map, GrowableArray<int>* oop_map,


This copies the contents of the maps from an inline class that's flattened into this class or a super class into the map for this class.

coleenp · 2025-10-27T18:47:17Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+    return map->append(Pair<int,int>(offset, size));
+  }
+  last_idx = last_idx == -1 ? 0 : last_idx;
+  int start = map->adr_at(last_idx)->first > offset ? 0 : last_idx;


last_idx is an optimization to try to prevent searching through the field map to find where to insert {offset,size} for the new entry. When would it not be ascending? super classes and inlined classes are inserted in order that they're found in the LayoutBlocks so wouldn't the offsets be ascending?

You answered this in slack for me. Can you put this at the top of this function:

The insert_segment creates an ordered list of maps {offset, size} from the LayoutRawBlocks. LayoutRawBlocks are queued in ascending offset order, and are processed in this order. This allows the last_idx optimization which would be critical for classes with a huge number of fields. But, this property is not required, and, if you look at the entire set of fields being processed, they might not all be processed by ascending offset order. Inherited fields are inserted first, by copying them from the super-class, and then local fields are processed. It is possible to have local fields interleaved with super-class fields. So all fields are not necessarily processed in ascending offset order. The algorithm tries to do it, and optimizes the insertion when it is done that way, but it also handles cases where this is not the case.

Comments have been added in FieldLayoutBuilder::generate_acmp_maps().
Let me know if the explanation is sufficient to clarify how the code works.

coleenp · 2025-10-27T19:31:24Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+  // last_idx remembers the position of the last insertion in order to speed up the next insertion.
+  // Local fields are processed in ascending offset order, so an insertion is very likely be performed
+  // next to the previous insertion. However, in some cases local fields and inherited fields can be
+  // interleaved, in which case the search of the insertion position cannot depend on the previous insertion.


coleenp

At the risk of repeating myself, this is great work!

liach

The library part looks acceptable, even though ideally I wish the VM would be able to request specialized comparison methods for each value type, so core library can provide specialized bytecode.

fparain · 2025-11-04T18:57:53Z

Coleen, Chen,
Thank you for your reviews and your feedback.

Fred

fparain · 2025-11-04T18:58:00Z

/integrate

openjdk · 2025-11-04T18:58:39Z

Going to push as commit 047e232.
Since your change was applied there have been 5 commits pushed to the lworld branch:

f638741: 8370951: [lworld] Value record ClassCircularityError
7b5f105: 8370484: [lworld] PhaseOutput::FillLocArray asserts with Unexpected type: anyptr
88f413d: [lworld] Don't run TestMethodHandles.java with -Xcomp
... and 2 more: https://git.openjdk.org/valhalla/compare/0ff86987f59f127abece39bdb5d9007a291147e9...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2025-11-04T18:58:47Z

@fparain Pushed as commit 047e232.

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

Add alternate version of the substitutability method

23c2ccd

openjdk bot added merge-conflict Pull request has merge conflict with target branch rfr Pull request is ready for review labels Oct 22, 2025

Merge remote-tracking branch 'upstream/lworld' into new_acmp

893afdc

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Oct 22, 2025

liach reviewed Oct 22, 2025

View reviewed changes

Apply changes suggested in review

c4d5cf4

coleenp reviewed Oct 23, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/lworld' into new_acmp

22b7404

More fixes uggested in reviews

661ffa7

coleenp approved these changes Oct 24, 2025

View reviewed changes

fparain added 2 commits October 24, 2025 15:08

Remove useless check

cf456a9

Add comment

f5f7448

coleenp reviewed Oct 27, 2025

View reviewed changes

Add comment

e9e79fe

coleenp approved these changes Oct 27, 2025

View reviewed changes

liach approved these changes Oct 27, 2025

View reviewed changes

fparain added 3 commits October 31, 2025 09:33

Field to field map must be final

52959d0

Merge remote-tracking branch 'upstream/lworld' into new_acmp

99f2114

Fix behavior when alt method is not used

899f480

openjdk bot added the integrated Pull request has been integrated label Nov 4, 2025

openjdk bot closed this Nov 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 4, 2025

8370450: [lworld] Alternate implementation of the substitutability test method #1695

8370450: [lworld] Alternate implementation of the substitutability test method #1695

Conversation

fparain commented Oct 22, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Oct 22, 2025

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Oct 22, 2025

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RogerRiggs commented Oct 23, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fparain commented Oct 23, 2025

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

fparain commented Nov 4, 2025

fparain commented Oct 22, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Oct 22, 2025 •

edited

Loading

mlbridge bot commented Oct 22, 2025 •

edited

Loading