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

8318706: Implement JEP 423: Region Pinning for G1 #16342

Conversation

tschatzl
Copy link
Contributor

@tschatzl tschatzl commented Oct 24, 2023

The JEP covers the idea very well, so I'm only covering some implementation details here:

  • regions get a "pin count" (reference count). As long as it is non-zero, we conservatively never reclaim that region even if there is no reference in there. JNI code might have references to it.

  • the JNI spec only requires us to provide pinning support for typeArrays, nothing else. This implementation uses this in various ways:

    • when evacuating from a pinned region, we evacuate everything live but the typeArrays to get more empty regions to clean up later.

    • when formatting dead space within pinned regions we use filler objects. Pinned regions may be referenced by JNI code only, so we can't overwrite contents of any dead typeArray either. These dead but referenced typeArrays luckily have the same header size of our filler objects, so we can use their headers for our fillers. The problem is that previously there has been that restriction that filler objects are half a region size at most, so we can end up with the need for placing a filler object header inside a typeArray. The code could be clever and handle this situation by splitting the to be filled area so that this can't happen, but the solution taken here is allowing filler arrays to cover a whole region. They are not referenced by Java code anyway, so there is no harm in doing so (i.e. gc code never touches them anyway).

  • G1 currently only ever actually evacuates young pinned regions. Old pinned regions of any kind are never put into the collection set and automatically skipped. However assuming that the pinning is of short length, we put them into the candidates when we can.

    • there is the problem that if an applications pins a region for a long time g1 will skip evacuating that region over and over. that may lead to issues with the current policy in marking regions (only exit mixed phase when there are no marking candidates) and just waste of processing time (when the candidate stays in the retained candidates)

      The cop-out chosen here is to "age out" the regions from the candidates and wait until the next marking happens.

      I.e. pinned marking candidates are immediately moved to retained candidates, and if in total the region has been pinned for G1NumCollectionsKeepUnreclaimable collections it is dropped from the candidates. Its current value is fairly random.

  • G1 pauses got a new tag if there were pinned regions in the collection set. I.e. in addition to something like:

    GC(6) Pause Young (Normal) (Evacuation Failure) 1M->1M(22M) 36.16ms

    there is that new tag (Pinned) that indicates that one or more regions that were pinned
    were encountered during gc. E.g.

    GC(6) Pause Young (Normal) (Pinned) (Allocation Failure) 1M->1M(22M) 36.16ms

    Pinned and Allocation Failure tags are not exclusive. GC might have encountered both pinned
    regions and allocation failed regions in the same collection or even in the same region. (I am
    open to a better name for the (Pinned) tag)

Testing: tier1-8


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-8318706: Implement JEP 423: Region Pinning for G1 (Enhancement - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16342

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

Using diff file

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

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 24, 2023

👋 Welcome back tschatzl! 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 openjdk bot changed the title 8318706 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 Oct 24, 2023
@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@tschatzl The following labels will be automatically applied to this pull request:

  • hotspot
  • serviceability

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

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Oct 24, 2023
@tschatzl
Copy link
Contributor Author

/label add hotspot-gc

@openjdk openjdk bot added the hotspot-gc hotspot-gc-dev@openjdk.org label Oct 24, 2023
@openjdk
Copy link

openjdk bot commented Oct 24, 2023

@tschatzl
The hotspot-gc label was successfully added.

The JEP covers the idea very well, so I'm only covering some implementation details here:

* regions get a "pin count" (reference count). As long as it is non-zero, we conservatively
  never reclaim that region even if there is no reference in there. JNI code might have
  references to it.

* the JNI spec only requires us to provide pinning support for typeArrays, nothing else.
  This implementation uses this in various ways:

  * when evacuating from a pinned region, we evacuate everything live but the typeArrays to
    get more empty regions to clean up later.

  * when formatting dead space within pinned regions we use filler objects. Pinned regions
    may be referenced by JNI code only, so we can't overwrite contents of any dead typeArray
    either.
    These dead but referenced typeArrays luckily have the same header size of our filler
    objects, so we can use their headers for our fillers. The problem is that previously
    there has been that restriction that filler objects are half a region size at most, so
    we can end up with the need for placing a filler object header inside a typeArray.
    The code could be clever and handle this situation by splitting the to be filled area
    so that this can't happen, but the solution taken here is allowing filler arrays to
    cover a whole region. They are not referenced by Java code anyway, so there is no harm
    in doing so (i.e. gc code never touches them anyway).

* G1 currently only ever actually evacuates young pinned regions. Old pinned regions of any kind
  are never put into the collection set and automatically skipped. However assuming that the
  pinning is of short length, we put them into the candidates when we can.

  * there is the problem that if an applications pins a region for a long time g1 will skip
    evacuating that region over and over. that may lead to issues with the current policy
    in marking regions (only exit mixed phase when there are no marking candidates) and
    just waste of processing time (when the candidate stays in the retained candidates)

    The cop-out chosen here is to "age out" the regions from the candidates and wait until
    the next marking happens.

    I.e. pinned marking candidates are immediately moved to retained candidates, and if
    in total the region has been pinned for `G1NumCollectionsKeepUnreclaimable` collections
    it is dropped from the candidates. Its current value is fairly random.

* G1 pauses got a new tag if there were pinned regions in the collection set. I.e. in addition
  to something like:

`GC(6) Pause Young (Normal) (Evacuation Failure) 1M->1M(22M) 36.16ms`

  there is that new tag `(Pinned)` that indicates that one or more regions that were pinned
  were encountered during gc. E.g.

`GC(6) Pause Young (Normal) (Pinned) (Evacuation Failure) 1M->1M(22M) 36.16ms`

  `Pinned` and `Evacuation Failure` tags are not exclusive. GC might have encountered both pinned
  regions and evacuation failed regions in the same collection or even in the same region.

whitespace fixes
@tschatzl tschatzl force-pushed the submit/8318706-implementation-of-region-pinning-in-g1 branch from 925edf8 to 44d430a Compare October 24, 2023 11:14
Thomas Schatzl added 3 commits October 24, 2023 15:07
…/HeapRegion.java so that resourcehogs/serviceability/sa/ClhsdbRegionDetailsScanOopsForG1.java does not fail
@tschatzl
Copy link
Contributor Author

tschatzl commented Oct 25, 2023

The new TestPinnedOldObjectsEvacuation.java test isn't stable, otherwise passes tier1-8. No perf changes.

I'm opening this PR for review even if this is the case, this is not a blocker for review, and fix it later.

@tschatzl tschatzl marked this pull request as ready for review October 25, 2023 14:15
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 25, 2023
@mlbridge
Copy link

mlbridge bot commented Oct 25, 2023

src/hotspot/share/gc/g1/g1CollectedHeap.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1ParScanThreadState.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/heapRegion.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1EvacFailureRegions.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/heapRegion.cpp Show resolved Hide resolved
@tschatzl
Copy link
Contributor Author

Had a discussion with @albertnetymk and we came to the following agreement about naming:

"allocation failure" - allocation failed in the to-space due to memory exhaustion
"pinned" - the region/object has been pinned
"evacuation failure" - either pinned or allocation failure

I will apply this new naming asap.

Thomas Schatzl added 2 commits November 2, 2023 09:29
… evacuation failure and types of it:

* evacuation failure is the general concept. It includes
  * pinned regions
  * allocation failure

One region can both be pinned and experience an allocation failure.

G1 GC messages use tags "(Pinned)" and "(Allocation Failure)" now instead of "(Evacuation Failure)"

Did not rename the G1EvacFailureInjector since this adds a lot of noise.
@tschatzl
Copy link
Contributor Author

tschatzl commented Nov 2, 2023

Had a discussion with @albertnetymk and we came to the following agreement about naming:

"allocation failure" - allocation failed in the to-space due to memory exhaustion
"pinned" - the region/object has been pinned
"evacuation failure" - either pinned or allocation failure

I will apply this new naming asap.

Done. I left out the G1EvacFailureInjector (it only injects allocation failures, not evacuation failures) related renamings as this adds lots of noise (including the debug options). I'll file a follow-up and assign it to me.

Tier1 seems to pass, will redo upper tiers again.

The only noteworthy externally visible change is that the (Evacuation Failure) tag in log messages is now (Allocation Failure). I did not want combinations of (Evacuation Failure) and additionally (Pinned) (Allocation Failure), but maybe it is fine, or just fine to keep only (Evacuation Failure) as before and assume that users enable higher level logging to find out details.

src/hotspot/share/gc/g1/g1FullGCPrepareTask.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1GCPhaseTimes.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1HeapRegionAttr.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1Policy.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1YoungCollector.cpp Show resolved Hide resolved
src/hotspot/share/gc/g1/heapRegion.cpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1GCPhaseTimes.hpp Outdated Show resolved Hide resolved
@tschatzl
Copy link
Contributor Author

tschatzl commented Nov 3, 2023

Fwiw, recent changes (without the most recent renamings) passed tier1-5

@@ -324,6 +324,10 @@
"retained region restore purposes.") \
range(1, 256) \
\
product(uint, G1NumCollectionsKeepPinned, 8, DIAGNOSTIC, \
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason this is not EXPERIMENTAL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this does not in any way enable risky/experimental code not fit for production. This knob is for helping diagnose performance issues.

G1 does have its fair share of experimental options, but all/most of these were from the initial import where G1 as a whole had been experimental (unstable) for some time.

Copy link
Member

Choose a reason for hiding this comment

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

This flag conceptually related (or similar) to G1RetainRegionLiveThresholdPercent, which is an exp, so I thought they should be the same category.

src/hotspot/share/gc/g1/g1Policy.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1Policy.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1YoungCollector.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Nov 3, 2023

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

8318706: Implement JEP 423: Region Pinning for G1

Reviewed-by: ayang, iwalulya, sjohanss

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

  • 115b074: 8319944: Remove DynamicDumpSharedSpaces
  • c0507af: 8319818: Address GCC 13.2.0 warnings (stringop-overflow and dangling-pointer)
  • 3684b4b: 8306116: Update CLDR to Version 44.0
  • 88ccd64: 8296250: Update ICU4J to Version 74.1
  • 03db828: 8319650: Improve heap dump performance with class metadata caching
  • b41b00a: 8319820: Use unnamed variables in the FFM implementation
  • 4d650fe: 8319704: LogTagSet::set_output_level() should not accept NULL as LogOutput
  • 6f863b2: 8318636: Add jcmd to print annotated process memory map
  • e035637: 8319375: test/hotspot/jtreg/serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java runs into OutOfMemoryError: Metaspace on AIX
  • 50f41d6: 8309893: Integrate ReplicateB/S/I/L/F/D nodes to Replicate node
  • ... and 47 more: https://git.openjdk.org/jdk/compare/45e68ae2079336cea45dcbc39189639c06a05e0c...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2023
Copy link
Member

@walulyai walulyai left a comment

Choose a reason for hiding this comment

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

LGTM!

Nits:

src/hotspot/share/gc/g1/g1CollectionSet.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1YoungCollector.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1FullCollector.cpp Outdated Show resolved Hide resolved
@openjdk
Copy link

openjdk bot commented Nov 7, 2023

@tschatzl this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout submit/8318706-implementation-of-region-pinning-in-g1
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Nov 7, 2023
@openjdk openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 7, 2023
@tschatzl tschatzl changed the title 8318706: Implementation of JDK-8276094: JEP 423: Region Pinning for G1 8318706: Implement JEP 423: Region Pinning for G1 Nov 8, 2023
@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 8, 2023
…Evacuation Failure" with a cause description (either "Allocation" or "Pinned")
Copy link
Contributor

@kstefanj kstefanj 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. Just a few small things.

src/hotspot/share/gc/shared/collectedHeap.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1YoungGCPostEvacuateTasks.cpp Outdated Show resolved Hide resolved
- fix counting of pinned/allocation failed regions in log
- some cleanup of evacuation failure code, removing unnecessary members
- comments
@tschatzl
Copy link
Contributor Author

Thanks @albertnetymk @kstefanj @walulyai for your reviews! Given that the JEP is now targeted, I will integrate.

This has been a fairly long journey until today... :)

/integrate

@openjdk
Copy link

openjdk bot commented Nov 29, 2023

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

  • e44d4b2: 8320858: Move jpackage tests to tier3
  • 5dcf3a5: 8320715: Improve the tests of test/hotspot/jtreg/compiler/intrinsics/float16
  • 78b6c2b: 8320898: exclude compiler/vectorapi/reshape/TestVectorReinterpret.java on ppc64(le) platforms
  • 9a6ca23: 8320918: Fix errors in the built-in Catalog implementation
  • 5e1b771: 8316422: TestIntegerUnsignedDivMod.java triggers "invalid layout" assert in FrameValues::validate
  • a657aa3: 8320681: [macos] Test tools/jpackage/macosx/MacAppStoreJlinkOptionsTest.java timed out on macOS
  • 3ccd02f: 8320379: C2: Sort spilling/unspilling sequence for better ld/st merging into ldp/stp on AArch64
  • 2c4c6c9: 8320049: PKCS10 would not discard the cause when throw SignatureException on invalid key
  • f93b18f: 8320932: [BACKOUT] dsymutil command leaves around temporary directories
  • ce4e6e2: 8320915: Update copyright year in build files
  • ... and 257 more: https://git.openjdk.org/jdk/compare/45e68ae2079336cea45dcbc39189639c06a05e0c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Nov 29, 2023
@openjdk openjdk bot closed this Nov 29, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 29, 2023
@openjdk
Copy link

openjdk bot commented Nov 29, 2023

@tschatzl Pushed as commit 38cfb22.

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

@tschatzl tschatzl deleted the submit/8318706-implementation-of-region-pinning-in-g1 branch January 16, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated serviceability serviceability-dev@openjdk.org
4 participants