Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8305895: Implementation: JEP 450: Compact Object Headers (Experimental) #13961

Closed
wants to merge 112 commits into from

Conversation

rkennke
Copy link
Contributor

@rkennke rkennke commented May 12, 2023

This is the main body of the JEP 450: Compact Object Headers (Experimental).

Main changes:

  • Introduction of the (experimental) flag UseCompactObjectHeaders. All changes in this PR are protected by this flag.
  • The compressed Klass* can now be stored in the mark-word of objects. In order to be able to do this, we are building on 8291555: Implement alternative fast-locking scheme #10907, 8305896: Alternative full GC forwarding #13582 and 8305898: Alternative self-forwarding mechanism #13779 to protect the relevant (upper 32) bits of the mark-word. Significant parts of this PR deal with loading the compressed Klass* from the mark-word, and dealing with (monitor-)locked objects. When the object is monitor-locked, we load the displaced mark-word from the monitor, and load the compressed Klass* from there. This PR also changes some code paths (mostly in GCs) to be more careful when accessing Klass* (or mark-word or size) to be able to fetch it from the forwardee in case the object is forwarded, and/or reach through to the monitor when the object is locked by a monitor.
  • The identity hash-code is narrowed to 25 bits.
  • Instances can now have their base-offset (the offset where the field layouter starts to place fields) at offset 8 (instead of 12 or 16).
  • Arrays will can now store their length at offset 8. Due to alignment restrictions, array elements will still start at offset 16. 8139457: Relax alignment of array elements #11044 will resolve that restriction and allow array elements to start at offset 12 (except for long, double and uncompressed oops, which are still required to start at an element-aligned offset).
  • CDS can now write and read archives with the compressed header. However, it is not possible to read an archive that has been written with an opposite setting of UseCompactObjectHeaders.

Testing:
(+UseCompactObjectHeaders tests are run with the flag hard-patched into the build, to also catch @flagless tests, and to avoid mismatches with CDS - see above.)

  • tier1 (x86_64)
  • tier2 (x86_64)
  • tier3 (x86_64)
  • tier4 (x86_64)
  • tier1 (aarch64)
  • tier2 (aarch64)
  • tier3 (aarch64)
  • tier4 (aarch64)
  • tier1 (x86_64) +UseCompactObjectHeaders
  • tier2 (x86_64) +UseCompactObjectHeaders
  • tier3 (x86_64) +UseCompactObjectHeaders
  • tier4 (x86_64) +UseCompactObjectHeaders
  • tier1 (aarch64) +UseCompactObjectHeaders
  • tier2 (aarch64) +UseCompactObjectHeaders
  • tier3 (aarch64) +UseCompactObjectHeaders
  • tier4 (aarch64) +UseCompactObjectHeaders

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
  • Change requires CSR request JDK-8306000 to be approved

Integration blockers

 ⚠️ Dependency #13582 must be integrated first
 ⚠️ Too few reviewers with at least role reviewer found (have 0, need at least 1) (failed with updated jcheck configuration in pull request)
 ⚠️ Whitespace errors (failed with updated jcheck configuration in pull request)

Issues

  • JDK-8305895: Implementation: JEP 450: Compact Object Headers (Experimental) (Enhancement - P4)
  • JDK-8306000: Add experimental -XX:+UseCompactObjectHeaders flag (CSR)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13961

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 16, 2023

@rkennke This pull request has been inactive for more than 16 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Nov 16, 2023
@rkennke
Copy link
Contributor Author

rkennke commented Nov 16, 2023

/open

@openjdk openjdk bot reopened this Nov 16, 2023
@openjdk
Copy link

openjdk bot commented Nov 16, 2023

@rkennke This pull request is now open

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/13779 to pr/13582 December 8, 2023 03:29
@openjdk-notifier
Copy link

The parent pull request that this pull request depends on has been closed without being integrated and the target branch of this pull request has been updated as the previous branch was deleted. This means that changes from the parent pull request will start to show up in this pull request. If closing the parent pull request was done in error, it will need to be re-opened and this pull request will need to manually be retargeted again.

@rkennke rkennke changed the base branch from pr/13582 to pr/13779 December 11, 2023 12:35
@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 11, 2023
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 11, 2023
@mmyxym
Copy link

mmyxym commented Jan 8, 2024

Hi Roman,

I ran into a crash of G1ConcurrentMark::scan_root_regions in JDK11 with compact header that the object iteration in root region scanning encountered an NULL mark from an object monitor because of monitor deflation which cleared the remote header on monitor while the concurrent mark thread still hold the monitor to get the mark header. There's a very special point in scan_root_regions that the concurrent mark thread cannot join STS so the race could happen.

The problem should be very rare in tip or JDK17+ because the monitor deflation is moved into a concurrent thread and there're handshake and VM_RendezvousGCThreads to seperate deflation and monitor deletion. But theoratically the concurrent mark thread in scan_root_regions can still have a monitor which might be deleted at anytime.

@rkennke
Copy link
Contributor Author

rkennke commented Jan 8, 2024

Hi Roman,

I ran into a crash of G1ConcurrentMark::scan_root_regions in JDK11 with compact header that the object iteration in root region scanning encountered an NULL mark from an object monitor because of monitor deflation which cleared the remote header on monitor while the concurrent mark thread still hold the monitor to get the mark header. There's a very special point in scan_root_regions that the concurrent mark thread cannot join STS so the race could happen.

The problem should be very rare in tip or JDK17+ because the monitor deflation is moved into a concurrent thread and there're handshake and VM_RendezvousGCThreads to seperate deflation and monitor deletion. But theoratically the concurrent mark thread in scan_root_regions can still have a monitor which might be deleted at anytime.

Hi Liang,

Thanks for the report! This seems to be a hard problem, indeed! On the one hand, scan_root_regions() would need to coordinate with the deflation thread, otherwise it could crash as you described, because oop_iterate() needs to access the Klass* and therefore the header, which might be displaced in a monitor. If that monitor gets deflated without coordination, the G1 thread could reach to a bad header and crash. That coordination is normally ensured by letting the concurrent GC thread join the STS. On the other hand, the concurrent GC thread must not join the STS until scan_root_regions() is finished, because of the reasons described in G1ConcurrentMarkThread::concurrent_mark_cycle_do(), otherwise a subsequent GC request could block scan_root_regions() from joining the STS.

I think that the best answer for now is to wait until external ObjectMonitor mapping arrives, which should be soon, AFAIK (@xmas92 is working on it and should know more). I am not sure if there is a good other solution to the problem that would work with monitor deflation (e.g. in Lilliput/JDK17 or 11), maybe @tschatzl has a good idea?

@mmyxym
Copy link

mmyxym commented Jan 9, 2024

Hi Roman,
I ran into a crash of G1ConcurrentMark::scan_root_regions in JDK11 with compact header that the object iteration in root region scanning encountered an NULL mark from an object monitor because of monitor deflation which cleared the remote header on monitor while the concurrent mark thread still hold the monitor to get the mark header. There's a very special point in scan_root_regions that the concurrent mark thread cannot join STS so the race could happen.
The problem should be very rare in tip or JDK17+ because the monitor deflation is moved into a concurrent thread and there're handshake and VM_RendezvousGCThreads to seperate deflation and monitor deletion. But theoratically the concurrent mark thread in scan_root_regions can still have a monitor which might be deleted at anytime.

Hi Liang,

Thanks for the report! This seems to be a hard problem, indeed! On the one hand, scan_root_regions() would need to coordinate with the deflation thread, otherwise it could crash as you described, because oop_iterate() needs to access the Klass* and therefore the header, which might be displaced in a monitor. If that monitor gets deflated without coordination, the G1 thread could reach to a bad header and crash. That coordination is normally ensured by letting the concurrent GC thread join the STS. On the other hand, the concurrent GC thread must not join the STS until scan_root_regions() is finished, because of the reasons described in G1ConcurrentMarkThread::concurrent_mark_cycle_do(), otherwise a subsequent GC request could block scan_root_regions() from joining the STS.

I think that the best answer for now is to wait until external ObjectMonitor mapping arrives, which should be soon, AFAIK (@xmas92 is working on it and should know more). I am not sure if there is a good other solution to the problem that would work with monitor deflation (e.g. in Lilliput/JDK17 or 11), maybe @tschatzl has a good idea?

So far I just skip the deflation while in scan_root_regions in 11u which is practically working but not solid theoretically. Maybe it would ok without side effects if deflation wait until scan_root_regions finish because the G1 young GC will wait for scan_root_regions in the same way.

BTW, I wonder if we could try to join STS in scan_root_regions as regular GC ops. Leaving this exception here seems a bit "evil". We can block the young GC trigger in Java thread until scan_root_regions is done as we did the same like GC locker. So the scan_root_regions can join STS and young GC still will wait for scan_root_regions finish.

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/13779 to pr/13582 February 15, 2024 11:52
@openjdk-notifier
Copy link

The parent pull request that this pull request depends on has been closed without being integrated and the target branch of this pull request has been updated as the previous branch was deleted. This means that changes from the parent pull request will start to show up in this pull request. If closing the parent pull request was done in error, it will need to be re-opened and this pull request will need to manually be retargeted again.

@openjdk openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 19, 2024
@openjdk
Copy link

openjdk bot commented Mar 13, 2024

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@rkennke
Copy link
Contributor Author

rkennke commented Apr 29, 2024

Will re-open when ready in Lilliput

@rkennke rkennke closed this Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csr Pull request needs approved CSR before integration hotspot hotspot-dev@openjdk.org merge-conflict Pull request has merge conflict with target branch serviceability serviceability-dev@openjdk.org shenandoah shenandoah-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.

9 participants