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

8276670: G1: Move and rename G1CardSetFreePool and related classes #6289

Closed

Conversation

Hamlin-Li
Copy link

@Hamlin-Li Hamlin-Li commented Nov 8, 2021

Currently, only the card sets (remembered sets) use G1CardSetFreePool to give back memory to OS.

After JDK-8254739, this memory reclaiming mechanism could be reused by evacuation failure too. This is a preparation change to allow reuse of this code.

I plan to do this in about 3 steps to smooth the review process:

  1. move G1CardSetFreePool and related classes to new file, rename these classes
  2. refactor these classes to support freeing other freelist
  3. some necessary cleanup

This is to simply move and rename G1CardSetFreePool and related classes, as G1CardSetFreePool and related classes are going to be reused outside of the remembered set, they should be factored out and renamed.
Rename from G1CardSetXxx -> G1BufferListXxx.


Progress

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

Issue

  • JDK-8276670: G1: Move and rename G1CardSetFreePool and related classes

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6289

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 8, 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
Copy link

@openjdk openjdk bot commented Nov 8, 2021

@Hamlin-Li 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 rename-move-G1CardSetFreePool-out
git fetch https://git.openjdk.java.net/jdk 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 rfr labels Nov 8, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 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.

@openjdk openjdk bot added the hotspot-gc label Nov 8, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 8, 2021

Webrevs

@openjdk openjdk bot removed the merge-conflict label Nov 8, 2021
@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Nov 8, 2021

Seems windows x64 build failure is not related to this change.

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Nov 10, 2021

Is some one available to have a look at this change? Thanks

#include "gc/g1/g1CardSetMemory.hpp"
#include "gc/g1/heapRegionRemSet.hpp"
#include "utilities/growableArray.hpp"
#include "utilities/ticks.hpp"

// Task handling deallocation of free card set memory.
class G1CardSetFreeMemoryTask : public G1ServiceTask {
class G1BufferListFreeMemoryTask : public G1ServiceTask {
Copy link
Contributor

@tschatzl tschatzl Nov 10, 2021

Choose a reason for hiding this comment

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

Comment needs to be adapted.

@@ -53,10 +54,10 @@ class G1CardSetFreeMemoryTask : public G1ServiceTask {
State _state;

// Current total card set memory usage.
G1CardSetMemoryStats _total_used;
G1BufferListMemoryStats _total_used;
Copy link
Contributor

@tschatzl tschatzl Nov 10, 2021

Choose a reason for hiding this comment

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

Comment mentions "card set"

_source->bulk_add(*_first, *last, keep_num, keep_size);
_first = cur;

log_trace(gc, task)("Card Set Free Memory: Returned to VM %zu buffers size %zu", keep_num, keep_size);
Copy link
Contributor

@tschatzl tschatzl Nov 10, 2021

Choose a reason for hiding this comment

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

"Card Set"

};

// A set of free lists holding memory buffers for use by G1CardSetAllocators.
template<MEMFLAGS flag>
Copy link
Contributor

@tschatzl tschatzl Nov 10, 2021

Choose a reason for hiding this comment

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

G1CardSetAllocators


// A set of free lists holding memory buffers for use by G1CardSetAllocators.
template<MEMFLAGS flag>
class G1BufferListFreePool {
Copy link
Contributor

@tschatzl tschatzl Nov 10, 2021

Choose a reason for hiding this comment

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

I would prefer G1SegmentedArrayFreePool here, seeing the segmented array as the important thing (I understand that G1SegmentedArrayBufferListFreePool is too long).
I think there is already a CR to reconsider naming of all these classes, maybe add G1SegmentedArrayBufferList to the ones that need renaming; in particular I think right now that the "BufferList" of G1SegmentedArrayBufferList is kind of redundant.
Sorry for not speaking up earlier about this, but it only came to my mind right now.

It would be great for that renaming CR to start a thread in hotspot-gc-dev before actually doing the renames. Assuming that this renaming will be a close follow-up I do not have a particular opinion about the current name chosen, but maybe it is useful to actually start this discussion now to avoid renaming this class in particular again.

Copy link
Author

@Hamlin-Li Hamlin-Li Nov 11, 2021

Choose a reason for hiding this comment

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

Thanks Thomas.

I would prefer G1SegmentedArrayFreePool here, seeing the segmented array as the important thing (I understand that G1SegmentedArrayBufferListFreePool is too long).

Sure.

I think there is already a CR to reconsider naming of all these classes,

Do you mean https://bugs.openjdk.java.net/browse/JDK-8276299 or some other issue?

maybe add G1SegmentedArrayBufferList to the ones that need renaming;

If you mean 8276299 above, sure I will add this to the renaming list of 8276299.

in particular I think right now that the "BufferList" of G1SegmentedArrayBufferList is kind of redundant.

Not quite get your point, would you mind to clarify further?

Sorry for not speaking up earlier about this, but it only came to my mind right now.

It's OK, it's never late to make necessary changes. :)

It would be great for that renaming CR to start a thread in hotspot-gc-dev before actually doing the renames.

Do you mean not send a pr but just send email to hotspot-gc-dev to discuss this idea of renaming?

Assuming that this renaming will be a close follow-up I do not have a particular opinion about the current name chosen, but maybe it is useful to actually start this discussion now to avoid renaming this class in particular again.

If you mean 8276299 above, then I think these 2 (8276299 and this one) are not quite related to each other, 8276299 is to unify the naming of "buffer", "node" and "element" (especially in card set area currently), but this one is to factor out Factor out G1CardSetFreePool and enable evac failure to reuse the freeing memory logic in current G1CardSetFreePool.
Sure, I will start a discussion for 8276299 before actually send the pr.

Copy link
Contributor

@tschatzl tschatzl Nov 11, 2021

Choose a reason for hiding this comment

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

Thanks Thomas.

I would prefer G1SegmentedArrayFreePool here, seeing the segmented array as the important thing (I understand that G1SegmentedArrayBufferListFreePool is too long).

Sure.

I think there is already a CR to reconsider naming of all these classes,

Do you mean https://bugs.openjdk.java.net/browse/JDK-8276299 or some other issue?

Yes.

maybe add G1SegmentedArrayBufferList to the ones that need renaming;

If you mean 8276299 above, sure I will add this to the renaming list of 8276299.

in particular I think right now that the "BufferList" of G1SegmentedArrayBufferList is kind of redundant.

Not quite get your point, would you mind to clarify further?

A segmented array is an array consisting of segments (as opposed to a single huge array of something); a list of buffers, where a "buffer" is nothing but an array (of some element type) is almost the same. One is a set of chunks of elements (to explicitly use other words), the other too.

It would be great for that renaming CR to start a thread in hotspot-gc-dev before actually doing the renames.

Do you mean not send a pr but just send email to hotspot-gc-dev to discuss this idea of renaming?

Not to discuss the idea of renaming, 8276299 is pretty clear about that, but discuss the actual mappings from old name to new name. Maybe this will wake up more people to the discussion than hiding it in a PR :)

Assuming that this renaming will be a close follow-up I do not have a particular opinion about the current name chosen, but maybe it is useful to actually start this discussion now to avoid renaming this class in particular again.

If you mean 8276299 above, then I think these 2 (8276299 and this one) are not quite related to each other, 8276299 is to unify the naming of "buffer", "node" and "element" (especially in card set area currently), but this one is to factor out Factor out G1CardSetFreePool and enable evac failure to reuse the freeing memory logic in current G1CardSetFreePool. Sure, I will start a discussion for 8276299 before actually send the pr.

That's true, but we are renaming something that also contains a "Buffer" in it and seemingly related.

So instead of renaming this G1BufferListFreePool again to something else in 8276299, maybe we could save the time and immediately change it to the final name now, then in 8276299 do the same for the other classes.

Then 8276299 will hopefully go through more smoothly too if we already decided on the new names beforehand as well.

Copy link
Author

@Hamlin-Li Hamlin-Li Nov 11, 2021

Choose a reason for hiding this comment

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

Thanks for clarification, I see your point, will initiate an discussion later. :)

Copy link
Author

@Hamlin-Li Hamlin-Li Nov 16, 2021

Choose a reason for hiding this comment

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

Discussion thread is here: https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2021-November/037626.html

Per above discussion, we prefer to use following naming relationship:

segmented array (1) -- segment (N) -- slot (M) -- element (1)

@openjdk openjdk bot added the merge-conflict label Nov 19, 2021
@openjdk openjdk bot removed the merge-conflict label Nov 19, 2021
@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Nov 22, 2021

kindly reminder ~

@albertnetymk
Copy link
Member

@albertnetymk albertnetymk commented Nov 22, 2021

I think splitting "move" and "rename" into two PRs (the current commits history is a bit hard to follow) will make the review process much easier and the included meaningful changes more discernible.

@@ -25,9 +25,9 @@
#ifndef SHARE_GC_G1_G1CARDSETMEMORY_HPP
#define SHARE_GC_G1_G1CARDSETMEMORY_HPP

#include "gc/g1/g1SegmentedArrayFreePool.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Include order, must be sorted alphabetically

@@ -25,10 +25,9 @@
#include "precompiled.hpp"

#include "gc/g1/g1CardSetMemory.inline.hpp"
#include "gc/g1/g1CardSetContainers.inline.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Include order, must be sorted alphabetically

@@ -33,7 +33,7 @@
#include "gc/g1/g1Arguments.hpp"
#include "gc/g1/g1BarrierSet.hpp"
#include "gc/g1/g1BatchedTask.hpp"
#include "gc/g1/g1CardSetFreeMemoryTask.hpp"
#include "gc/g1/g1SegmentedArrayFreeMemoryTask.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Include order, must be sorted alphabetically

@@ -27,7 +27,7 @@

#include "gc/g1/g1BarrierSet.hpp"
#include "gc/g1/g1BiasedArray.hpp"
#include "gc/g1/g1CardSetFreeMemoryTask.hpp"
#include "gc/g1/g1SegmentedArrayFreeMemoryTask.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Include order, must be sorted alphabetically

@@ -23,7 +23,7 @@
*/

#include "precompiled.hpp"
#include "gc/g1/g1CardSetFreeMemoryTask.hpp"
#include "gc/g1/g1SegmentedArrayFreeMemoryTask.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Include order, must be sorted alphabetically

#include "gc/g1/g1SegmentedArray.hpp"
#include "utilities/growableArray.hpp"

// Statistics for a fixed set of buffer lists. Contains the number of buffers and memory
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

s/fixed set of buffer lists/segmented array/
Contains the number of segments and memory.... Afair that's part of the other renaming change coming up.

public:

size_t _num_mem_sizes[G1CardSetConfiguration::num_mem_object_types()];
size_t _num_buffers[G1CardSetConfiguration::num_mem_object_types()];
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

The renaming of _num_buffers to _num_segments should be done in the other renaming change (which is fine).

@@ -33,7 +33,7 @@
class WorkerTask;
class G1Allocator;
class G1BatchedTask;
class G1CardSetMemoryStats;
class G1SegmentedArrayMemoryStats;
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Must be sorted alphabetically.

@@ -22,13 +22,15 @@
*/

#include "precompiled.hpp"
#include "gc/g1/g1SegmentedArrayFreePool.hpp"
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

Must be sorted alphabetically.

return _state_names[static_cast<std::underlying_type_t<State>>(value)];
}

bool G1CardSetFreeMemoryTask::deadline_exceeded(jlong deadline) {
bool G1SegmentedArrayFreeMemoryTask::deadline_exceeded(jlong deadline) {
return os::elapsed_counter() >= deadline;
Copy link
Contributor

@tschatzl tschatzl Nov 22, 2021

Choose a reason for hiding this comment

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

The include for os seems to be missing (pre-existing)

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Nov 22, 2021

I will put the "move" in a separate PR and later "rename" in another PR (including Thomas's suggestion till now)
The new pr is at #6499.

@Hamlin-Li
Copy link
Author

@Hamlin-Li Hamlin-Li commented Nov 23, 2021

I will close this pr, as #6514 is the newly created one which only "rename" (including Thomas's suggestion in this pr till now)

@Hamlin-Li Hamlin-Li closed this Nov 23, 2021
@Hamlin-Li Hamlin-Li deleted the rename-move-G1CardSetFreePool-out 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 rfr
3 participants