8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer by fparain · Pull Request #2312 · openjdk/valhalla

fparain · 2026-04-08T15:22:05Z

Add acmp_maps verification to the FieldLayoutAnalyzer.
The FieldLayoutAnalyzer recomputes the memory segments used by fields from the available layout descriptions, and compare the results to the acmp_maps computed by the JVM.
The verification is automatically performed for all tests calling the check() method of the FieldLayoutAnalyzer (like the ValueRandomLayoutTest.java).

Tested repeatedly with ValueRandomLayoutTest.java on Mach5.

I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

Thanks,

Fred

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 Committer)

Issue

JDK-8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer (Bug - P4)

Reviewers

Paul Hübner (@Arraying - Committer) ⚠️ Review applies to 1ac3b7e7
Matias Saavedra Silva (@matias9927 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2312

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2026-04-08T15:23:23Z

👋 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 · 2026-04-08T15:24:56Z

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

8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer

Reviewed-by: phubner, matsaave

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

383d50e: 8379694: [lworld] Add value class test coverage to java.util.Arrays
8f95866: 8382028: [lworld] gc/stress/gcbasher/TestGCBasherWithParallel.java fails with OOME
a1cad61: 8382189: [lworld] move two ProblemList entries for tests impacted by JDK-8381268
... and 10 more: https://git.openjdk.org/valhalla/compare/2588fe3b10c45d4d35afca34f5701bbb10d9d4e1...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.

mlbridge · 2026-04-08T15:32:25Z

Webrevs

Arraying

Thanks for this change. This is a valuable thing to have in our test, and I think the logic of using a bitmap to check for overlaps makes a lot of sense. I've left some code feedback and potential future ideas.

Arraying · 2026-04-10T12:30:18Z

        }
+        // Non-oop acmp map <offset,size>: <12,1> <16,4> ...
        Asserts.assertTrue(lo.getCurrentLine().startsWith("Non-oop acmp map <offset,size>"), lo.getCurrentLine());
+        String[] acmp_nonoop_line = lo.getCurrentLine().split("\\s+");


May be worth doing a .trim() before splitting by whitespace, it's good practice so we don't end up with any surprises, especially since our output has trailing spaces at the moment.

The expression used to split the line takes care of removing consecutive whitespaces and trailing whitespaces.

Arraying · 2026-04-10T13:01:01Z

+    for (ClassLayout layout : layouts) {
+      if (!layout.isValue) continue;
+      System.out.println("Checking acmp map of class " + layout.name);
+      boolean[] acmpBitMap = new boolean[layout.instanceSize];


If I understand the format properly, it could be a nice addition to verify that the leftover false regions in the bitmap correspond to the padding BlockType.

false regions could be padding, or empty, or reserved, in fact all block types that are not real basic types or the null marker.

Good point.

Right now, we are testing that what we get from the acmp map is correct (i.e., fields do not overlap). I think it would be valuable to test (perhaps a follow-up) from the opposite angle as well: is everything that's not in the acmp map accounted for? Are there any unexpected leftover areas?

For instance, let's say that we introduce a bug which forgets to include a field in the acmp map. We end up with a bitmap of e.g. length 5, and the (faulty) acmp map says there are fields in indices 1, 2, and 4. After the test is run, the bitmap looks as follows:

[F | T | T | F | T]

We know that index 1 corresponds to padding, so we mark that as T as well. We have no empty, reserved, etc. blocks. The final bitmap looks as follows:

[T | T | T | F | T]

At this point, the bitmap should only be truthy. If we add an assertion, that will fail. It turns out, index 3 corresponds to the region that got skipped during the (faulty) acmp map creation. By looking at what's left, we ensure nothing is left out by accident.

Does any of that make sense? 😅

I don't really understand your point. The acmp_map must contain all the segments that are relevant for a substitutability test, and only the segments that are relevant, so padding areas must not be in the map. The new test reconstructs a map of the relevant segments using the field information, and then compare this reconstructed map with the VM generated map. At line 834, the test checks that at each index, the two maps are consistent, they must agree that either it is a relevant area or it is not. If the VM generated acmp_map is missing a field area, this field area will show up in the reconstructed map, and the test will detect the mismatch.

so padding areas must not be in the map.

Not in the acmp map, but I was under the impression that FieldLayoutAnalyzer has this information. What I'm suggesting combines existing generic layout tests with the new acmp bitmap test.

this field area will show up in the reconstructed map

That's a good point (or maybe my example was just bad).

To summarize, I'm suggesting something along the lines of:

Get the field layout of value class X. Save all of the FieldBlocks as blocks.

Create a bitmap that spans the whole field layout region (I think we already do this right now).

Get the acmp maps, set the bitmap indices to true and ensure no collisions (we are doing this already).

Iterate through every block b in blocks:

If we haven't seen b's type yet in the acmp maps (e.g., we already marked the null marker, so skip that:

Ensure the corresponding bitmap index is false and then set to true.

Iterate through the bitmap at every index i:

Ensure that the value at index i is true. If not, we've "lost" something in some part of our bookkeeping.

I hope that explains the idea a bit better. Thoughts?

As we discussed off-line, this is a different kind of verification unrelated to acmp_maps. This should be addressed in a different PR.

Arraying

Thanks!

matias9927

Change looks good! Just some cosmetic comments

matias9927

LGTM!

fparain · 2026-04-16T17:04:05Z

Thanks @Arraying @matias9927 for the reviews.
/integrate

openjdk · 2026-04-16T17:06:35Z

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

383d50e: 8379694: [lworld] Add value class test coverage to java.util.Arrays
8f95866: 8382028: [lworld] gc/stress/gcbasher/TestGCBasherWithParallel.java fails with OOME
a1cad61: 8382189: [lworld] move two ProblemList entries for tests impacted by JDK-8381268
... and 10 more: https://git.openjdk.org/valhalla/compare/2588fe3b10c45d4d35afca34f5701bbb10d9d4e1...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2026-04-16T17:06:50Z

@fparain Pushed as commit 8997310.

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

Add acmp_maps verification

cd1a6ad

openjdk bot added the rfr Pull request is ready for review label Apr 8, 2026

Arraying reviewed Apr 10, 2026

View reviewed changes

Minor changes from review comments

1ac3b7e

openjdk bot removed the rfr Pull request is ready for review label Apr 10, 2026

Arraying approved these changes Apr 15, 2026

View reviewed changes

matias9927 suggested changes Apr 16, 2026

View reviewed changes

Comment thread test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/FieldLayoutAnalyzer.java Outdated

Comment thread test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/FieldLayoutAnalyzer.java

fparain added 2 commits April 16, 2026 12:52

Fix typo

fa7f879

Fix blank lines

d5c14c7

matias9927 approved these changes Apr 16, 2026

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2026

openjdk bot added the integrated Pull request has been integrated label Apr 16, 2026

openjdk bot closed this Apr 16, 2026

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2026

Conversation

fparain commented Apr 8, 2026 • 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 Apr 8, 2026

Uh oh!

openjdk bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlbridge bot commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Arraying left a comment

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!

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!

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!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

matias9927 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

matias9927 left a comment

Choose a reason for hiding this comment

Uh oh!

fparain commented Apr 16, 2026

Uh oh!

openjdk bot commented Apr 16, 2026

Uh oh!

openjdk bot commented Apr 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

fparain commented Apr 8, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Apr 8, 2026 •

edited

Loading

mlbridge bot commented Apr 8, 2026 •

edited

Loading