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

8204686: Dynamic parallel reference processing support for Parallel GC #4253

Closed
wants to merge 5 commits into from

Conversation

lkorinth
Copy link
Contributor

@lkorinth lkorinth commented May 28, 2021

The first part:

  1. sets ParallelRefProcEnabled to true if we have multiple threads (in Parallel)
  2. removes the field _adjust_no_of_processing_threads so that Parallel and G1 handles dynamic resizing the same
  3. removes an early return --- ergo_proc_thread_count() already adjusts for ReferencesPerThread == 0

The second part tries something else. It makes ParallelRefProcEnabled default to true for all GCs and does not take into consideration the amount of gc threads. I have talked to Thomas Schatzl before posting this and we will probably not use part two. Reasons include that we can then no longer read the flag ParallelRefProcEnabled to see if we are going to do parallel ref processing as it will be true even if we only have one thread (or are running Serial GC). I still want to show the possibility.

Part three shows the impact on test cases when part two is applied. I will need to adjust part three if we remove Part 2.

Please take an extra look at the removal of the early return, it looks valid to me and hotspot_gc passes.

I will try to get this into 17, but if review and CSR takes time I will postpone it to 18. I suggest dropping part 2 and will do so if no one objects.


Progress

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

Issue

  • JDK-8204686: Dynamic parallel reference processing support for Parallel GC

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4253

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 28, 2021

👋 Welcome back lkorinth! 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 added the rfr label May 28, 2021
@openjdk
Copy link

openjdk bot commented May 28, 2021

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

  • hotspot-gc

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 hotspot-gc label May 28, 2021
@mlbridge
Copy link

mlbridge bot commented May 28, 2021

Webrevs

@albertnetymk
Copy link
Member

albertnetymk commented May 28, 2021

we can then no longer read the flag ParallelRefProcEnabled to see if we are going to do parallel ref processing

Given the semantics of ParallelRefProcEnabled is "Enable parallel reference processing whenever possible", I think it's fine to forgo that guarantee, ParallelRefProcEnabled == true iff parallel ref processing.

That being said, I think a possibly better direction is to remove the ambiguity in the semantics, "Perform parallel reference processing", and keep the check for ParallelGCThreads > 1 before setting ParallelRefProcEnabled to true.

PS: the current patch looks fine to me as well.

@tschatzl
Copy link
Contributor

tschatzl commented May 31, 2021

I think part 2 should be dropped: it is always a help for debugging performance issues to see what ParallelRefProcEnabled is (with -XX:+PrintFlagsFinal) if it can be determined at startup whether parallel reference processing is (not) active.

I.e. whether there is absolutely no chance that the VM will use parallel gc threads for reference processing is already useful information. Particularly for serial gc it might mislead people if ParallelRefProcEnabled were true for it. Not sure if changing the description of the flag is necessary, but I see that there might be some ambiguity.

This is not a hard no for keeping part2, but unless there are good reasons to keep it I would suggest dropping this change.

@lkorinth
Copy link
Contributor Author

lkorinth commented May 31, 2021

Hi, and sorry for showing the alternative with the global default set to true. In the CSR I have said I want to go for the option of setting only the Parallel case to true (and only if more than one thread). I will update the code to reflect this as I think it is less of a change.

This reverts commit 7535ede.
Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm. Thanks!

@openjdk
Copy link

openjdk bot commented Jun 1, 2021

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

8204686: Dynamic parallel reference processing support for Parallel GC

Reviewed-by: ayang, tschatzl, kbarrett

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

  • 3025f05: 8264305: Create implementation for native accessibility peer for Statusbar java role
  • e2d5ff9: 8268214: Use system zlib and disable dtrace when building linux-aarch64 at Oracle
  • 1b4378e: 8268142: Switch to jdk-17+24 for macosx-aarch64 at Oracle
  • edca245: 8267917: mark hotspot containers tests which ignore external VM flags
  • 05df172: 8268224: Cleanup references to "strictfp" in core lib comments
  • 516e60a: 8268095: CDS MethodHandle tests should add -XX:-VerifyDependencies
  • c1f3094: 8267939: Clarify the specification of iterator and spliterator forEachRemaining
  • 460ce55: 8266019: StreamResult(File) writes to incorrect file path if # is part of the file path
  • b955865: 8267995: Add reference to JVMS class file format in Lookup::defineHiddenClass
  • 9f05c41: 8265783: Create a separate library for x86 Intel SVML assembly intrinsics
  • ... and 200 more: https://git.openjdk.java.net/jdk/compare/459abd561accc9f10456a9d63f7fa19c7f8e020e...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 label Jun 1, 2021
@kimbarrett
Copy link

kimbarrett commented Jun 2, 2021

Looks good.

@lkorinth
Copy link
Contributor Author

lkorinth commented Jun 7, 2021

/integrate

@openjdk openjdk bot closed this Jun 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jun 7, 2021
@openjdk
Copy link

openjdk bot commented Jun 7, 2021

@lkorinth Since your change was applied there have been 238 commits pushed to the master branch:

  • 908aca2: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
  • 3e48244: 8268301: Closed test: compiler/c2/6371167/Test.java fails after JDK-8267904
  • 204b492: 8267703: runtime/cds/appcds/cacheObject/HeapFragmentationTest.java crashed with OutOfMemory
  • 2aeeeb4: 8268279: gc/shenandoah/compiler/TestLinkToNativeRBP.java fails after LibraryLookup is gone
  • b05fa02: 8267904: C2 crash when compile negative Arrays.copyOf length after loop
  • 95ddf7d: 8267839: trivial mem leak in numa
  • 52d88ee: 8268292: compiler/intrinsics/VectorizedMismatchTest.java fails with release VMs
  • 042f0bd: 8256465: [macos] Java frame and dialog presented full screen freeze application
  • 8abf36c: 8268289: build failure due to missing signed flag in x86 evcmpb instruction
  • b05c40c: 8266951: Partial in-lining for vectorized mismatch operation using AVX512 masked instructions
  • ... and 228 more: https://git.openjdk.java.net/jdk/compare/459abd561accc9f10456a9d63f7fa19c7f8e020e...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9fc914b.

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

@lkorinth lkorinth deleted the _8204686 branch Mar 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc integrated
4 participants