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

8273626: G1: Factor out concurrent segmented array from G1CardSetAllocator #5478

Conversation

Hamlin-Li
Copy link

@Hamlin-Li Hamlin-Li commented Sep 11, 2021

To finish https://bugs.openjdk.java.net/browse/JDK-8254739, we need a segmented array to store a growing regions index array, in the initial version of that patch, a newly home made segmented array was used, but the memory efficiency is not as good as expected, G1CardSetAllocator is a potential candidate to fullfill the requirement, but need some enhancement.

This is a try to enhance G1CardSetAllocator(and G1CardSetBuffer, G1CardSetBufferList) to support element size less pointer size, and strip this basic function as a more generic segmented array (G1SegmentedArray).


Progress

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

Issue

  • JDK-8273626: G1: Factor out concurrent segmented array from G1CardSetAllocator

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5478

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Sep 11, 2021

👋 Welcome back mli! 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 Pull request is ready for review label Sep 11, 2021
@openjdk
Copy link

openjdk bot commented Sep 11, 2021

@Hamlin-Li 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.

@mlbridge
Copy link

mlbridge bot commented Sep 11, 2021

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Sep 15, 2021

Seems the failed test "java/util/regex/NegativeArraySize.java " is not related to this patch.

@tschatzl
Copy link
Contributor

tschatzl commented Sep 16, 2021

Hi Hamlin,

just fyi, I am aware that this CR is out, but currently I am a too busy looking into nontrivial PRs with some pre/post-JDK17 GA work. Initial look seems good though.

Thomas

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Sep 16, 2021

Thanks for the information, Thomas.
Sure, no hurry, please take your time.

Copy link
Contributor

@tschatzl tschatzl left a comment

Sorry for the late reply. Coincidentally around release there's also some housekeeping to do.

My main question about this change is: Would it be possible to give a sneak-peek on the use of G1SegmentedArray in the JDK-8254739 code? It is impossible to determine if the change fits the intended purpose, and I want to avoid changing G1SegmentedArray again later.

What I thought was that every region gets a set of elements using G1SegmentedArray attached to it, in whatever form. Multiple threads add to that at the same time.

This means that multiple threads might add new segments to it at the same time, meaning that they may try to append new segments to it. What happens with the thread and the segment that fails? Shouldn't this allocation be reused somehow (maybe for other segmented arrays?).

Also, this seems to drop the concurrent freeing of these segments after GC, which is nice too.

The remembered set uses the memory provided by the segmented arrays in chunks too, i.e. the G1CardSetArray container. Not sure if something like this would be more appropriate here instead of directly using the chunks (but may well be as good).

What are your plans about all this?

Depending on your design choices (which I do not know) this change may be reasonable or not. Please detail this a bit.

Also note that the CR title currently is a bit misleading imho, indicating a small change supporting smaller pointer sizes. I would prefer if it would be named something like "Factor out concurrent segmented array from G1CardSetAllocator".

src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
@Hamlin-Li Hamlin-Li changed the title 8273626: G1: refactor G1CardSetAllocator to support element size less pointer size 8273626: G1: Factor out concurrent segmented array from G1CardSetAllocator Sep 24, 2021
@Hamlin-Li
Copy link
Author

Hamlin-Li commented Sep 24, 2021

Sorry for the late reply. Coincidentally around release there's also some housekeeping to do.

Thanks Thomas, it's OK, this is not that urgent.

My main question about this change is: Would it be possible to give a sneak-peek on the use of G1SegmentedArray in the JDK-8254739 code? It is impossible to determine if the change fits the intended purpose, and I want to avoid changing G1SegmentedArray again later.

Sure, I've put the merged code in a temporary branch, https://github.com/Hamlin-Li/jdk/tree/tmp-merge.segmented-array.speed-remove-self. "speed-remove-self" part is still the old one, I did not change it yet, including the suggestions you gave in #5181 and old home made segmented array implementation; I just use the new G1SegmentedArray in G1EvacuationFailureObjsInHR.

What I thought was that every region gets a set of elements using G1SegmentedArray attached to it, in whatever form. Multiple threads add to that at the same time.

Yes, it's implemented in this way, although later in #5181 we may refactor how G1SegmentedArray is attached to a region, but G1SegmentedArray itself should be fine at that time.

This means that multiple threads might add new segments to it at the same time, meaning that they may try to append new segments to it. What happens with the thread and the segment that fails? Shouldn't this allocation be reused somehow (maybe for other segmented arrays?).

Current(original) implementation just drop the newly allocated G1SegmentedArrayBuffer if current thread fails, seems it's fine as the race should be rare, should we add the faield one to free list? seems adding to freelist will bring some benefit.

Also, this seems to drop the concurrent freeing of these segments after GC, which is nice too.

The remembered set uses the memory provided by the segmented arrays in chunks too, i.e. the G1CardSetArray container. Not sure if something like this would be more appropriate here instead of directly using the chunks (but may well be as good).

Not quite sure, seems both have benefit, in G1CardSetArray way, it has better design benefit; in G1SegmentedArray::iterate_nodes/G1EvacuationFailureObjsInHR::visit, it might have better performance. I'm OK with both ways, please kindly let me know your decision.

What are your plans about all this?

Depending on your design choices (which I do not know) this change may be reasonable or not. Please detail this a bit.

Also note that the CR title currently is a bit misleading imho, indicating a small change supporting smaller pointer sizes. I would prefer if it would be named something like "Factor out concurrent segmented array from G1CardSetAllocator".

Modified.

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 8, 2021

Kindly reminder.

Copy link
Contributor

@tschatzl tschatzl left a comment

I think other than the given comments the change seems good, but there will be a second thorough path.

Also the direct use of G1SegmentedArrayBuffer (I will likely suggest a separate G1SegmentedArray subclass for the new features) in G1EvacuationFailureObjsInHR seems okay.

src/hotspot/share/gc/g1/g1CardSetMemory.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
@tschatzl
Copy link
Contributor

tschatzl commented Oct 8, 2021

Some additional note:

Current(original) implementation just drop the newly allocated G1SegmentedArrayBuffer if current thread fails, seems it's fine as the race should be rare, should we add the faield one to free list? seems adding to freelist will bring some benefit.

Yes, that's also what G1CardSetAllocator does. At least there, the race isn't that rare.

You need to consider that as soon there are (real, not just faked ones) evacuation failures, all subsequent allocations of potentially all threads will fail. The segmented array somewhat deals with that by increasing the buffer size, and potentially you are not always failing in the same region...

Thanks,
Thomas

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 9, 2021

Current(original) implementation just drop the newly allocated G1SegmentedArrayBuffer if current thread fails, seems it's fine as the race should be rare, should we add the faield one to free list? seems adding to freelist will bring some benefit.

Yes, that's also what G1CardSetAllocator does. At least there, the race isn't that rare.

You need to consider that as soon there are (real, not just faked ones) evacuation failures, all subsequent allocations of potentially all threads will fail. The segmented array somewhat deals with that by increasing the buffer size, and potentially you are not always failing in the same region...

Agree. I've filed https://bugs.openjdk.java.net/browse/JDK-8274987 to track the enhancement, will work on it soon after we finish refactoring.

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 9, 2021

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 9, 2021

I almost missed https://bugs.openjdk.java.net/browse/JDK-8274993, as I can not see the message in this github PR page.
( should this be a bug of the infrastrucuture? )

Copy link
Contributor

@tschatzl tschatzl left a comment

More unnecessary changes during refactoring I did not notice earlier.

src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSet.cpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSet.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.hpp Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.cpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1CardSetMemory.inline.hpp Outdated Show resolved Hide resolved
src/hotspot/share/gc/g1/g1SegmentedArray.inline.hpp Outdated Show resolved Hide resolved
Copy link
Member

@albertnetymk albertnetymk left a comment

This PR incorporates some cleanup/small improvement into the main refactoring, extraction of the segmented array class. Such additional and unnecessary changes make reviewing this PR much harder that it should be.

I will give a concrete example. G1CardSetAllocator<Elem>::num_buffers used to live in inline.hpp, but was moved the header and updated to use the new segmented array. It could be argued that this is more consistent together with its neighbor (mem_size and wasted_mem_size), so it's a fine change. However, embedding this change inside this relatively invasive refactoring is very distracting; what could have been a one-line change was scattered in multiple places.

// synchronization.
template <class Elem, MEMFLAGS flag>
class G1SegmentedArray {
// G1CardSetAllocOptions provides parameters for allocation buffer
Copy link
Member

@albertnetymk albertnetymk Oct 17, 2021

Choose a reason for hiding this comment

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

The name, G1CardSetAllocOptions, is incorrect.

@openjdk
Copy link

openjdk bot commented Oct 17, 2021

@Hamlin-Li 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:

8273626: G1: Factor out concurrent segmented array from G1CardSetAllocator

Reviewed-by: tschatzl, ayang

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

  • 9d191fc: 8245095: Implementation of JEP 408: Simple Web Server
  • 947d52c: 8255898: Test java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java fails on Mac OS
  • a03119c: 8275436: [BACKOUT] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect
  • bad75e6: 8275150: URLClassLoaderTable should store OopHandle instead of Handle
  • 72a976e: 8266936: Add a finalization JFR event
  • bcbe384: 8269175: [macosx-aarch64] wrong CPU speed in hs_err file
  • 426bcee: 8275360: Use @OverRide in javax.annotation.processing
  • 4d383b9: 8275298: Remove unnecessary weak_oops_do call in adjust weak roots phase
  • fb8e5cf: 8275368: Correct statement of kinds of elements Processor.process operates over
  • d548f2f: 8274346: Support for additional content in an app-image.
  • ... and 267 more: https://git.openjdk.java.net/jdk/compare/bb74ae87abee0fb550e4138242919ec791f7791c...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 Oct 17, 2021
@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 18, 2021

Thanks Albert, I agree, this is a real lesson for me.

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 19, 2021

@tschatzl Hi Thomas, Would you mind to help take another review when available? Thanks

Copy link
Contributor

@tschatzl tschatzl left a comment

Lgtm.

@Hamlin-Li
Copy link
Author

Hamlin-Li commented Oct 19, 2021

Thanks @tschatzl @albertnetymk for your reviews.

/integrate

@openjdk
Copy link

openjdk bot commented Oct 19, 2021

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

  • a4491f2: 8275413: Remove unused InstanceKlass::set_array_klasses() method
  • 9d191fc: 8245095: Implementation of JEP 408: Simple Web Server
  • 947d52c: 8255898: Test java/awt/FileDialog/FilenameFilterTest/FilenameFilterTest.java fails on Mac OS
  • a03119c: 8275436: [BACKOUT] JDK-8271949 dumppath in -XX:FlightRecorderOptions does not affect
  • bad75e6: 8275150: URLClassLoaderTable should store OopHandle instead of Handle
  • 72a976e: 8266936: Add a finalization JFR event
  • bcbe384: 8269175: [macosx-aarch64] wrong CPU speed in hs_err file
  • 426bcee: 8275360: Use @OverRide in javax.annotation.processing
  • 4d383b9: 8275298: Remove unnecessary weak_oops_do call in adjust weak roots phase
  • fb8e5cf: 8275368: Correct statement of kinds of elements Processor.process operates over
  • ... and 268 more: https://git.openjdk.java.net/jdk/compare/bb74ae87abee0fb550e4138242919ec791f7791c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 19, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 19, 2021
@openjdk
Copy link

openjdk bot commented Oct 19, 2021

@Hamlin-Li Pushed as commit d17d81a.

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

@Hamlin-Li Hamlin-Li deleted the generalize-g1CardSetBuffer-and-Allocator branch Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-gc hotspot-gc-dev@openjdk.org integrated Pull request has been integrated
3 participants