8303027: [Lilliput/JDK17] Correctly resolve forwarded objects in G1 heap iteration

[rkennke]

A user provided a (pretty simple) test case that makes the VM crash with Lilliput, when it should exit (somewhat more) gracefully with an OOME. The reason for the crash is that in G1, object_iterate() (or rather, the block_size() method that it calls) does not correctly resolve possibly forwarded objects, and then crashes because it accesses the Klass*, which is overridden by the forwarding pointer.

The trouble is that, we don't need (or even can't) resolve forwarded objects during full-GC, because there we are using sliding-forwarding which is designed precisely to preserve the object's Klass* while being forwarded. Outside of full-GC we don't need to preserve the Klass* because the Klass* is preserved in the forwarded copy.

This bug only exists in Lilliput/JDK17, the code path that leads to object_iterate() (restoring self-forwarded objects at evac failure) doesn't exist anymore in later versions - it uses a better way to iterate over the relevant self-forwarded objects.

The proposed fix is to do two different loops, one that resolves the forwardees when needed, and the other one that doesn't. The alternative would have to check for the condition in_full_gc() on every object being scanned, which would affect performance. I included the test case as jtreg test.

Testing:

• runtime/oom/TestOOM.java
• tier1
• tier2

---

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-8303027: [Lilliput/JDK17] Correctly resolve forwarded objects in G1 heap iteration

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 590742c5

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/lilliput-jdk17u pull/6/head:pull/6
$ git checkout pull/6

Update a local copy of the PR:
$ git checkout pull/6
$ git pull https://git.openjdk.org/lilliput-jdk17u pull/6/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/lilliput-jdk17u/pull/6.diff

[bridgekeeper]

👋 Welcome back rkennke! A progress list of the required criteria for merging this PR into lilliput will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[mlbridge]

Webrevs

• 08: Full - Incremental (aeaf7753)
• 07: Full - Incremental (b1b1051d)
• 06: Full - Incremental (8cb1b7fe)
• 05: Full - Incremental (590742c5)
• 04: Full - Incremental (38a16dae)
• 03: Full - Incremental (cb758770)
• 02: Full - Incremental (0bcdee81)
• 01: Full - Incremental (38551d5e)
• 00: Full (717ab33b)

[shipilev]

Looks okay.

But depending on the frequency of block_is_obj(p) being true, we might just put the in_full_gc() condition inside the block_size() itself?

[openjdk]

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

8303027: [Lilliput/JDK17] Correctly resolve forwarded objects in G1 heap iteration

Reviewed-by: shade

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 no new commits pushed to the lilliput branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

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

[rkennke]

But depending on the frequency of block_is_obj(p) being true, we might just put the in_full_gc() condition inside the block_size() itself?

The actual problem only arises when evacuation fails and GC tries to recover from that (specifically, when the GC is restoring self-forwarded object headers). That is not very performance critical, I think. However, block_size() is called from many other places during GC, and is potentially applied to all (live) objects, e.g. during heap scan. It may be helped a little by bitmap based iteration, but I'd rather not risk affecting performance of all the common paths, if I can avoid it.

[rkennke]

Looks okay.

But depending on the frequency of block_is_obj(p) being true, we might just put the in_full_gc() condition inside the block_size() itself?

I just pushed an attempt to use a templated loop and block_size(), that avoids the duplication AND should perform well. WDYT? (Note: we could achieve the same using constexpr bool arg, but I don't think it is allowed in 17 yet).

[shipilev]

This looks much better, thanks.

[shipilev]

There is non-templated block_size here too, just below. Is the non-templated version used? If so, do you want to drop it and say template<bool RESOLVE = true> instead?

[rkennke]

It finally passes all GHA except the x-compiles, which is expected. Let's

/integrate

[openjdk]

Going to push as commit 4e089d1.

[openjdk]

@rkennke Pushed as commit 4e089d1.

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