Skip to content

8379415: Contended classes can leave unused alignment padding#30117

Closed
caspernorrbin wants to merge 7 commits intoopenjdk:masterfrom
caspernorrbin:contended-wasted-alignment-padding-fix
Closed

8379415: Contended classes can leave unused alignment padding#30117
caspernorrbin wants to merge 7 commits intoopenjdk:masterfrom
caspernorrbin:contended-wasted-alignment-padding-fix

Conversation

@caspernorrbin
Copy link
Copy Markdown
Member

@caspernorrbin caspernorrbin commented Mar 6, 2026

Hi everyone,

When placing fields, if the first field we place needs alignment, we insert extra padding before it. If the class is a contended class, or the field is part of a contended group, this inserted alignment block can become effectively lost and cannot be reused for another field, wasting space.

This happens in FieldLayoutBuilder::compute_regular_layout when we insert a field that needs alignment inside a contended range.

First, we insert the padding. The structure then looks like this:

Byte: 0            12                                               140
      | Header      | Padding                                        | _layout->start() & _layout->last_block()
      [############][-----------------------------------------------][------>
      ^             ^                                                ^

If we then insert an 8-byte long, we need to align before inserting it. After inserting the field, the structure looks like this:

Byte: 0            12                                               140    144     152
      | Header      | Padding                                        | Algn| long  | _layout->start() & _layout->last_block()
      [############][-----------------------------------------------][####][=======][------>
      ^             ^                                                ^     ^        ^

At this point, because the start block is also the end block, we never look past it. As a result, we never see the 4-byte alignment block when continuing to place the remaining fields, even though it could be reused.

For contended classes and contended field groups, we need to ensure we search the whole contended range for empty blocks instead of stopping early. The code change is slightly different between the two cases, but both ensure we scan the full contended range so alignment padding can be reused.

Testing:

  • Oracle tiers 1-5
  • Additional class layout tests
  • Manual inspection of class layouts across different combinations before and after the change

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

Issue

  • JDK-8379415: Contended classes can leave unused alignment padding (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 30117

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

Using diff file

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

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link
Copy Markdown

bridgekeeper Bot commented Mar 6, 2026

👋 Welcome back cnorrbin! 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
Copy link
Copy Markdown

openjdk Bot commented Mar 6, 2026

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

8379415: Contended classes can leave unused alignment padding

Reviewed-by: coleenp, fparain, lfoltan, 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 20 new commits pushed to the master branch:

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 master branch, type /integrate in a new comment.

@openjdk openjdk Bot changed the title 8379415 8379415: Contended classes can leave unused alignment padding Mar 6, 2026
@openjdk openjdk Bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Mar 6, 2026
@openjdk
Copy link
Copy Markdown

openjdk Bot commented Mar 6, 2026

@caspernorrbin The following label will be automatically applied to this pull request:

  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk Bot added the rfr Pull request is ready for review label Mar 6, 2026
@mlbridge
Copy link
Copy Markdown

mlbridge Bot commented Mar 6, 2026

Webrevs

Comment thread src/hotspot/share/classfile/fieldLayoutBuilder.cpp Outdated
Copy link
Copy Markdown
Contributor

@fparain fparain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openjdk openjdk Bot added the ready Pull request is ready to be integrated label Mar 9, 2026
Copy link
Copy Markdown
Member

@lfoltan lfoltan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Lois

Copy link
Copy Markdown
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks good, but can you write a small test case?

@caspernorrbin
Copy link
Copy Markdown
Member Author

I think this looks good, but can you write a small test case?

I created a new test MixedPrimitives.java, based off of test/hotspot/jtreg/runtime/contended/Basic.java. Instead of testing contended with classes containing two ints, we instead do it with 3 primitives of different sizes.

@openjdk openjdk Bot removed the ready Pull request is ready to be integrated label Mar 26, 2026
Comment thread test/hotspot/jtreg/runtime/contended/MixedPrimitives.java Outdated
Comment thread test/hotspot/jtreg/runtime/contended/MixedPrimitives.java
Comment thread test/hotspot/jtreg/runtime/contended/MixedPrimitives.java Outdated
Comment thread test/hotspot/jtreg/runtime/contended/MixedPrimitives.java Outdated
@caspernorrbin
Copy link
Copy Markdown
Member Author

@liach Thank you for the test feedback! I've made the changes suggested.

Copy link
Copy Markdown
Member

@liach liach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tweaked test looks fine.

@openjdk openjdk Bot added the ready Pull request is ready to be integrated label Mar 26, 2026
failures += 1;
}

if (!arePaddedPairwise(Test3.class, "long1", "int1") ||
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you could move these tests into the Test class as a static method like:
failures += Test2.checkLayout();

Then they'd be with the fields themselves.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the layout checks to now be inside each class!

@openjdk openjdk Bot removed the ready Pull request is ready to be integrated label Mar 27, 2026
Copy link
Copy Markdown
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! This came out very nicely.

@openjdk openjdk Bot added the ready Pull request is ready to be integrated label Mar 27, 2026
@caspernorrbin
Copy link
Copy Markdown
Member Author

Thank you everyone for reviewing!

/integrate

@openjdk
Copy link
Copy Markdown

openjdk Bot commented Mar 30, 2026

Going to push as commit 4d7c139.
Since your change was applied there have been 20 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

@openjdk openjdk Bot added the integrated Pull request has been integrated label Mar 30, 2026
@openjdk openjdk Bot closed this Mar 30, 2026
@openjdk openjdk Bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 30, 2026
@openjdk
Copy link
Copy Markdown

openjdk Bot commented Mar 30, 2026

@caspernorrbin Pushed as commit 4d7c139.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.

5 participants