8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses #1966

jsikstro · 2026-01-26T08:43:40Z

Hello,

Please refer to the JBS issue for a more detailed description of the background of this change. In summary, I suggest we only keep the array of InlineLayoutInfo for InstanceKlasses which need it, which are Klasses that have fields that have been inlined.

To make the transition to this easier, I suggest we change the following properties in FieldLayoutBuilder:

_has_inline_type_fields
_has_flattening_information

to

_has_inlineable_fields
_has_inlined_fields

The _has_inlineable_fields property is only used for printing and _has_inlined_fields is the property we expose out to the ClassFileParser, telling us that this class has inlined fields, so the array of InlineLayoutInfo must be "preserved" and is possible to read from. Hence, the array is now only safe to access if InstanceKlass::has_inlined_fields is true, or simply if the actual field being accessed is flat (fieldDescriptor::is_flat).

I only found one place (in ciReplay.cpp) where we access the array of InlineLayoutInfo even though we might not have any inlined fields and only fields that are inlineable. I've changed this to use the normal "reference" path for fields that aren't flat.

Testing:

Oracle's tier1-5, hotspot_valhalla and jdk_valhalla

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-8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses (Bug - P4)

Reviewers

Paul Hübner (@Arraying - Committer)
Frederic Parain (@fparain - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1966

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…anceKlasses

bridgekeeper · 2026-01-26T08:44:49Z

👋 Welcome back jsikstro! 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-01-26T08:46:34Z

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

8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses

Reviewed-by: phubner, fparain

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

5878735: Merge
6165daf: 8375458: Check legal folder of JDK image for unwanted files
03038d8: 8375978: G1: Convert G1Policy to use Atomic
... and 112 more: https://git.openjdk.org/valhalla/compare/3eab1e005f08044ac0eb9fc84f3a309e9ab3a018...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 (@stefank, @xmas92, @Arraying, @fparain) 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).

mlbridge · 2026-01-26T08:49:56Z

Webrevs

stefank

I'm leaving the proper review to Valhalla-devs, but I added a few comments that could be considered.

stefank · 2026-01-26T10:10:07Z

src/hotspot/share/classfile/classFileParser.cpp

          if (klass != nullptr) {
            if (klass->is_inline_klass()) {
-              _inline_layout_info_array->adr_at(fieldinfo.index())->set_klass(InlineKlass::cast(klass));
+              set_inline_layout_info_klass(fieldinfo.index(), klass, CHECK);


I see that this can throw a Metaspace OOME (hence the CHECK) and that we later have this comment:

// Loads triggered by the LoadableDescriptors attribute are speculative, failures must not impact loading of current class if (HAS_PENDING_EXCEPTION) { CLEAR_PENDING_EXCEPTION; }

Can you verify that using CHECK here is really appropriate? The resolve_super_or_fail uses THREAD, and I think that's what the comment refer to. But with the current patch there's a "returning" call in-between those two parts of the function.

No, I didn't consider any pending exceptions when I added the CHECK. I see that we have an assert(false, ...) down in the Metaspace allocation if there's a pending exception, so we really shouldn't try to allocate the array of InlineLayoutInfo if there's a pending exception, which makes sense.

Looking at the code, I think we should re-arrange it so that we clear any pending exception before calling set_inline_layout_info_klass, while keeping the CHECK so that we exit from ClassFileParser::post_process_parsed_stream if we fail to allocate in Metaspace. This seems to me to be consistent with the old behavior, but it would be nice for someone to fill in here.

After looking at this for a bit longer I argue that we should never get a resolved klass (non-nullptr) with a pending exception, so the code can be simplified a bit further.

Correct, if SystemDictionary::resolve_super_or_fail() returns a non-null pointer, no exception should have been set by this method, and the methods call before resolve_super_or_fail() have a CHECK argument, ensuring the call to SystemDictionary::resolve_super_or_fail() cannot be reached with a pending exception.

stefank · 2026-01-26T10:20:53Z

src/hotspot/share/classfile/classFileParser.cpp

      _must_be_atomic, _layout_info, _inline_layout_info_array);
  lb.build_layout();
-  _has_inline_type_fields = _layout_info->_has_inline_fields;
+  _has_inlined_fields = _layout_info->_has_inlined_fields;


More of a questions to the long-time Valhalla devs: I'm a little curious why the parser has booth of these fields containing the same value?

Historical reasons. The _layout_info structure was added late, and the previous ways to handle inline fields information was not properly refactored.
The ClassFilerParser gets the _layout_info instance before creating the InstanceKlass instance and keeps it until the InstanceKlass instance has been fully initialized, so it is safe to refactor the code to remove ClassFileParser::_has_inline_fields and refactor ClassFilerParser::has_inline_fields() to return _layout_info->_has_inlined_fields.
Note: currently, the only client of ClassFilerParser::has_inline_fields() is the InstanceKlass constructor.

src/hotspot/share/classfile/classFileParser.cpp

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

stefank · 2026-01-26T10:30:57Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

    {
      bool use_atomic_flat = _must_be_atomic; // flatten atomic fields only if the container is itself atomic
      LayoutKind lk = field_layout_selection(fieldinfo, _inline_layout_info_array, use_atomic_flat);
+      const int field_index = (int)fieldinfo.index();


Are idx and field_index different here?

I don't think they ever are. I added an assert and ran tier1-3, which never hit. Maybe @fparain can shed some light on this? Otherwise I think we should use field_info.index() in favor of idx and maybe replace the GrowableArrayIterator with a range-based for loop, like this:

for (FieldInfo fi : *_field_info) { ... }

xmas92

Looks good. Just one comment on using a more precis type signature.

A pre-existing issue which I wonder if we can do better. And a general question of how the failure paths are suppose to work for these new pre-loaded classes.

src/hotspot/share/classfile/classFileParser.cpp

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

xmas92 · 2026-01-26T10:34:42Z

src/hotspot/share/classfile/classFileParser.cpp

          if (klass != nullptr) {
            if (klass->is_inline_klass()) {
-              _inline_layout_info_array->adr_at(fieldinfo.index())->set_klass(InlineKlass::cast(klass));
+              set_inline_layout_info_klass(fieldinfo.index(), klass, CHECK);


I think this is a property already precent in the code but this change made me reflect on this.

What are the expected interactions for pre-loaded classes if we fail to load parse the class file which contains them. For example here if we have just resolved the pre-loaded class, and then get metaspace out of memory and stop loading this class file.

Is this the correct (specification wise) interaction for partially failed class file parsing?

There's nothing from the specification point of view that enforces that a class and its pre-loaded (via LoadableDescriptor) classes must be all-or-nothing.

Classes and interfaces mentioned by LoadableDescriptors may optionally be loaded when the referencing class is derived and created (5.3.5), or during early stages of linking (5.4).

This is the same situation as when the JVM is loading a class A, and during the loading of A, it successfully loads A's super-class B, but then encounter an issue while continuing the loading of A (OOM in Metaspace, or B is an interface, or B is final etc.). The loading of A fails, but class B remains in the system dictionary, until its class loader is unloaded.

Arraying

Thanks a lot for doing this, the new names are much better and less memory usage is always good. I left some comments, the most pressing one I'd like to resolve is the notion of having flat fields when a value object is buffered.

src/hotspot/share/classfile/classFileParser.cpp

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

…ation

Arraying

I think this looks good, thanks again for doing it. Maybe it'd be good if @fparain also takes a look at this PR.

fparain · 2026-01-29T21:33:57Z

src/hotspot/share/classfile/fieldLayoutBuilder.cpp

+  if (!fieldinfo.field_flags().is_injected() &&
+      ili != nullptr &&
+      ili->adr_at((int)fieldinfo.index())->klass() != nullptr &&
+      !ili->adr_at((int)fieldinfo.index())->klass()->is_identity_class()) {


To be inlineable, the class should not be abstract.

!ili->adr_at((int)fieldinfo.index())->klass()->is_abstract()

would exclude both abstract value classes and interfaces.

Yes! I'll append that to the checks.

fparain · 2026-01-29T22:00:25Z

src/hotspot/share/classfile/classFileParser.cpp

  _klass = klass;
 }

+void ClassFileParser::set_inline_layout_info_klass(int field_index, InstanceKlass* klass, TRAPS) {


This method could take an InlineKlass* argument instead of an InstanceKlass* argument, because InlineLayoutInfo can only store information about flat value fields.

Sure, I'll move the InlineKlass::cast() from insdide the method to the callers instead.

fparain

Looks good to me.
Thank you for this cleanup.

jsikstro · 2026-02-03T11:41:05Z

Thank you for the reviews everyone! I merged with the tip of lworld and reran tier1-4 which is green. The failing test in GHA is due to an unrelated issue (solved in JDK-8376358).

/integrate

openjdk · 2026-02-03T11:43:46Z

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

Arraying · 2026-02-03T12:19:29Z

/sponsor

openjdk · 2026-02-03T12:21:02Z

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

5878735: Merge
6165daf: 8375458: Check legal folder of JDK image for unwanted files
03038d8: 8375978: G1: Convert G1Policy to use Atomic
... and 112 more: https://git.openjdk.org/valhalla/compare/3eab1e005f08044ac0eb9fc84f3a309e9ab3a018...lworld

Your commit was automatically rebased without conflicts.

openjdk · 2026-02-03T12:21:12Z

@Arraying @jsikstro Pushed as commit 53c5531.

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

8376221: [lworld] Do not store array of InlineLayoutInfo for all Inst…

ae50201

…anceKlasses

openjdk bot added the rfr Pull request is ready for review label Jan 26, 2026

stefank reviewed Jan 26, 2026

View reviewed changes

xmas92 reviewed Jan 26, 2026

View reviewed changes

jsikstro added 7 commits January 26, 2026 11:56

InstanceKlass instead of Klass for set_inline_layout_info_klass

fc2a73c

IOOB assert message

77c16d0

Split assert in field sorting methods

ea25676

Clear exception before setting inline klass

033b7a0

Comment for second CLEAR_PENDING_EXCEPTION

1c10105

Move inlineable check to static helper

edde34d

Exception check should really be an assert

e28ad2a

Arraying reviewed Jan 28, 2026

View reviewed changes

jsikstro added 3 commits January 28, 2026 19:30

Remove mention of 'warning' in comment

a8c8c45

LayoutKind assertion for flat fields

bc697f0

Merge branch 'lworld' into JDK-8376221_inlinelayoutinfo_array_optimiz…

b934da6

…ation

Arraying approved these changes Jan 29, 2026

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 29, 2026

fparain reviewed Jan 29, 2026

View reviewed changes

jsikstro added 2 commits February 2, 2026 10:36

Add abstract check to field_is_inlineable

b1374bc

Move InlineKlass::cast() to callers

cb80bac

fparain approved these changes Feb 2, 2026

View reviewed changes

Arraying approved these changes Feb 3, 2026

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Feb 3, 2026

openjdk bot added the integrated Pull request has been integrated label Feb 3, 2026

openjdk bot closed this Feb 3, 2026

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 Feb 3, 2026

8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses #1966

8376221: [lworld] Do not store array of InlineLayoutInfo for all InstanceKlasses #1966

Uh oh!

Conversation

jsikstro commented Jan 26, 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 Jan 26, 2026

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

stefank 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xmas92 left a comment

Choose a reason for hiding this comment

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!

Arraying left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Arraying 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!

fparain left a comment

Choose a reason for hiding this comment

Uh oh!

jsikstro commented Feb 3, 2026

Uh oh!

openjdk bot commented Feb 3, 2026

Uh oh!

Arraying commented Feb 3, 2026

Uh oh!

jsikstro commented Jan 26, 2026 •

edited by openjdk bot

Loading

openjdk bot commented Jan 26, 2026 •

edited

Loading

mlbridge bot commented Jan 26, 2026 •

edited

Loading