8253779: Amalloc may be wasting space by overaligning

[coleenp]

Thanks to @kimbarrett for noticing this. The alignment was changed to 64 bits for 32 bit platforms, but overalign for 64 bits platforms. I changed this to BytesPerLong to cover both, since the long case is why it was changed on 32 bits in the first place in JDK-4526490.
I also removed Amalloc_D since I don't know what D stands for and it's the same as Amalloc_4. That's not a great name either. I'm open to suggestions!
Tested with tier1-3.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8253779: Amalloc may be wasting space by overaligning

Reviewers

• Kim Barrett (@kimbarrett - Reviewer)
• Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to db91c5a

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4732/head:pull/4732
$ git checkout pull/4732

Update a local copy of the PR:
$ git checkout pull/4732
$ git pull https://git.openjdk.java.net/jdk pull/4732/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4732

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4732.diff

[bridgekeeper]

👋 Welcome back coleenp! 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]

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

• hotspot

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.

[mlbridge]

Webrevs

• 03: Full - Incremental (7ebc7421)
• 02: Full - Incremental (220c3cb8)
• 01: Full - Incremental (db91c5ae)
• 00: Full (fcb50228)

[kimbarrett]

I replied to some "outdated" conversation threads. I'm not sure how that will show up in the UI or in the Skara emails. GitHub seems to be aggressively hiding some of the conversations in the "Files changed" view, which seems pretty unhelpful.

[kimbarrett]

Yuck! The handling of the replies to "outdated" conversations seems pretty horrible. In "conversation" mode they remain attached to the original change, while in "files changed" mode they don't show up at all. Sorry this seems to be kind of a mess. I also have no idea why these got tagged as "outdated".

[tstuefe]

Yuck! The handling of the replies to "outdated" conversations seems pretty horrible. In "conversation" mode they remain attached to the original change, while in "files changed" mode they don't show up at all. Sorry this seems to be kind of a mess. I also have no idea why these got tagged as "outdated".

I think they get outdated once a new push is made. I agree that this is confusing. The way we work, the "outdated" tag has no meaning.

[tstuefe]

I like this fix and the suggested naming cleanups. LGTM.

Since you are talking about potential improvements:

I was always a bit unhappy with this arena code. E.g. I disliked how, instead of letting the Arena itself deal with its chunk chain, it exposed the chain internals and let the Marks modify the chunks from outside. This always seemed wrong to me.

There may be more potential improvements:

• _hwm and _max should be properties of the chunk, not the arena, and then arguably could be smaller typed offsets instead of pointers.
• I am not sure why we need to keep track of both _first and _current chunk in the arenas. I think one pointer would suffice: just holding the last added chunk, which would serve as starting point to traverse the chain. I may miss something here though, maybe traversal order matters somewhere.

I also disliked how an Arena would always create its first chunk right when constructed, instead of delaying chunk allocation to the first allocation. You always pay upfront even if you don't allocate from the Arena.

If one does all of the above, Arena could maybe shrink to just one member (the top chunk pointer), and then it could be embedded as a value object into Thread instead of having to dynamically create and destroy it. Would be slightly simpler and save one pointer dereferencing when accessing the resource area. You'd also save NMT registration of the Arenas themselves.

Cheers, Thomas

(I also had vague ideas of re-using Metaspace arena code for these hotspot arenas, which share many similarities. But I am not sure if or when I find the time to play with that idea.)

[openjdk]

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

8253779: Amalloc may be wasting space by overaligning

Reviewed-by: kbarrett, stuefe

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 48 new commits pushed to the master branch:

• 68b6e11: 8270083: -Wnonnull errors happen with GCC 11.1.1
• ec975c6: Merge
• 6889a39: 8268826: Cleanup Override in Context-Specific Deserialization Filters
• f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
• 1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
• 885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
• 62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
• 46c610c: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms
• d6c0f5f: 8270059: Remove KVHashtable
• 7bfa39f: 8270108: Update JCov version to 3.0.9
• ... and 38 more: https://git.openjdk.java.net/jdk/compare/2209e3ec655d6013adc8dd5a463235b5db4d73d4...master

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.

[tstuefe]

One thought, maybe it should keep the alignment adjustable with a diagnostic switch in debug. At least for a little while. Since we may uncover hidden overwrite issues with the reduced alignment.

[coleenp]

2 comments in this view.

[coleenp]

I'll try to answer the main comments in one message. I hope I haven't lost any.

1. I don't want to add a switch to keep the original alignment. I think we should be confident that this change is correct or not make it! It's the beginning of the release. We have time to find any bugs that are unlikely to fall out.

2. Funny that metaspace was originally inherited from Arena, as you know, but I'd rather see a simpler implementation for Arena if rewritten than using all of Metaspace. Metaspace needs to deallocate from the middle and Arena doesn't. Arena uses malloc which is better imo except for on one platform. Rewriting Arena isn't something that's on the top of our list, please only file a bug if you think you're going to do it @tstuefe :)

3. I'll file another RFE for renaming (ignore some comment above that I can't find now).

[tstuefe]

I'll try to answer the main comments in one message. I hope I haven't lost any.

1. I don't want to add a switch to keep the original alignment. I think we should be confident that this change is correct or not make it! It's the beginning of the release. We have time to find any bugs that are unlikely to fall out.

Okay, sure.

2. Funny that metaspace was originally inherited from Arena, as you know, but I'd rather see a simpler implementation for Arena if rewritten than using all of Metaspace. Metaspace needs to deallocate from the middle and Arena doesn't. Arena uses malloc which is better imo except for on one platform. Rewriting Arena isn't something that's on the top of our list, please only file a bug if you think you're going to do it @tstuefe :)

Of course, sorry, I was not suggesting someone other should do it. I was just interested in your thoughts.

3. I'll file another RFE for renaming (ignore some comment above that I can't find now).

Change looks still good to me.

..Thomas

[coleenp]

Thanks Thomas! Since Arena's haven't been a problem we haven't actually thought about them, other than this RFE and now the new RFE JDK-8270179.

[kimbarrett]

Looks good.

[coleenp]

Thanks Kim. Thank you for the suggestions on other improvements.
/integrate

[openjdk]

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

• 68b6e11: 8270083: -Wnonnull errors happen with GCC 11.1.1
• ec975c6: Merge
• 6889a39: 8268826: Cleanup Override in Context-Specific Deserialization Filters
• f791fdf: 8261147: C2: Node is wrongly marked as reduction resulting in a wrong execution due to wrong vector instructions
• 1196b35: 8270151: IncompatibleClassChangeError on empty pattern switch statement case
• 885f7b1: 8269146: Missing unreported constraints on pattern and other case label combination
• 62ff55d: 8269952: compiler/vectorapi/VectorCastShape*Test.java tests failed on avx2 machines
• 46c610c: 8269840: Update Platform.isDefaultCDSArchiveSupported() to return true for aarch64 platforms
• d6c0f5f: 8270059: Remove KVHashtable
• 7bfa39f: 8270108: Update JCov version to 3.0.9
• ... and 38 more: https://git.openjdk.java.net/jdk/compare/2209e3ec655d6013adc8dd5a463235b5db4d73d4...master

Your commit was automatically rebased without conflicts.

[openjdk]

@coleenp Pushed as commit ac75a53.

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