RFR: 8236926: Concurrently uncommit memory in G1 [v4]

[Stefan Johansson]

On Thu, 12 Nov 2020 10:49:23 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Albert review
>
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 2668:
> 
>> 2666: void G1CollectedHeap::uncommit_heap_if_necessary() {
>> 2667:   if (hrm()->has_inactive_regions()) {
>> 2668:     G1UncommitRegionTask::activate();
> 
> Since `activate` is already used for regions, I would suggest another word; simple `run` is enough. Additionally, I don't think using an enum is necessary, a plain `bool is_running` should do.
> 
> enum class TaskState { active, inactive };
> TaskState _state;

Good catch, for a short while I had the state `running` as well, but now just having a simple bool is enough now. I still call the state `_active` since I think that is more accurate. Also change the name to `run()`.

> src/hotspot/share/gc/g1/g1CommittedRegionMap.hpp line 67:
> 
>> 65:   void active_clear_range(uint start, uint end);
>> 66:   void inactive_set_range(uint start, uint end);
>> 67:   void inactive_clear_range(uint start, uint end);
> 
> After reading their implementation, I see that `Uncommit_lock` must be held on call them. I think it's best to mention this precondition in the comments, and explain why this lock is needed.

Updated the comments a bit and referred to guarantee_mt_safty_* for more details.

> src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp line 107:
> 
>> 105: 
>> 106:   // Each execution is limited to uncommit at most 256M worth of regions.
>> 107:   static const uint region_limit = (uint) (256 * M / G1HeapRegionSize);
> 
> Why 256M? Better include some motivation in the comments.

256M is just a "reasonable" limit that I picked to get short enough invocations. I updated the comment a bit.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 178:
> 
>> 176: 
>> 177: void HeapRegionManager::expand(uint start, uint num_regions, WorkGang* pretouch_gang) {
>> 178:   guarantee(num_regions > 0, "No point in calling this for zero regions");
> 
> The same `guarantee` is already in `commit_regions`; I think an assert is fine.

Removed it, as you say we check it the first thing we do in `commit_regions`.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 189:
> 
>> 187:     }
>> 188: 
>> 189:     if (G1CollectedHeap::heap()->hr_printer()->is_active()) {
> 
> This `if` is not needed, right? `commit(hr)` does such check already. There are a few other cases of the same kind.

This is a bit unfortunate, but this `is_inactive()` is checking if the `G1HRPrinter` is active and should print stuff. So it is needed.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 264:
> 
>> 262:   assert(num_regions > 0, "No point in calling this for zero regions");
>> 263: 
>> 264:   clear_auxiliary_data_structures(start, num_regions);
> 
> IMO, this name is too general. Even after reading its body and the associated comments, I don't get what "data structures" are cleared.

To me the comments and implementation is pretty descriptive, but I did update the comment for `signal_mapping_changed` a bit to make it more explicit. Also added a few lines to explain what will be cleared by each mapper.

I agree that this is not a perfect name but I think the naming is in line with how we refer to these structures elsewhere in the code.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 272:
> 
>> 270: void HeapRegionManager::deactivate_regions(uint start, size_t num_regions) {
>> 271:   guarantee(num_regions >= 1, "Need to specify at least one region to uncommit, tried to uncommit zero regions at %u", start);
>> 272:   guarantee(length() >= num_regions, "pre-condition");
> 
> I am not really sure why `guarantee` here but `assert` in `reactivate_regions`. I don't see a strong reason to use `guarantee` here.

Changed to assert, the reason for them being different is that the code in `deactivate_regions` is old and just moved into this function. Also changed condition to > 0 like we have most other places.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 330:
> 
>> 328:     // No more regions available for uncommit
>> 329:     if (range.length() == 0) {
>> 330:       return uncommitted;
> 
> Is `assert(uncommitted > 0)` true here? Why or why not? Please add some comments; I think it's helpful for the readers.

`uncommitted` can be 0 here so we can't add that assert. There is a chance that between we add the uncommit-task (which calls this function) and that we grab the lock, someone else might have used the `inactive` regions to expand the heap again. Update the comment a bit, I hope it makes it easier to follow.

> src/hotspot/share/gc/g1/heapRegionManager.hpp line 81:
> 
>> 79:   G1RegionToSpaceMapper* _card_counts_mapper;
>> 80: 
>> 81:   // Map to keep track of which regions are in use.
> 
> The name of the variable suggests it's tracking what regions are committed and that's all. After reading `G1CommittedRegionMap`, I believe the class name and the var name are quite misleading; it tracks the state of mem backing up all regions, `committed+mapped`, `committed`, `to_be_uncommitted`, `committed`. I don't have any good alternatives, but the comments could surely be expanded.

Naming is hard and I agree this isn't perfect but the union of the two bitmaps in `G1CommittedRegionMap` do track what is committed, so it's not completely wrong.

I did update the comment here a bit.

> src/hotspot/share/gc/g1/heapRegionManager.hpp line 97:
> 
>> 95: 
>> 96:   // Notify other data structures about change in the heap layout.
>> 97:   void update_committed_space(HeapWord* old_end, HeapWord* new_end);
> 
> `update_committed_space` is not implemented, right?

Correct, filed [JDK-8256323](https://bugs.openjdk.java.net/browse/JDK-8256323) for this.

> src/hotspot/share/gc/g1/heapRegionManager.hpp line 137:
> 
>> 135:   void deactivate_regions(uint start, size_t num_regions);
>> 136:   void reactivate_regions(uint start, uint num_regions);
>> 137:   void uncommit_regions(uint start, uint num_regions);
> 
> Why `uint` vs `size_t`? Why `= 1` for some but not others? If such inconsistency is intentional, it should be documented.

The reason for `size_t` for `deactivate_regions` is that we have a size_t in `shrink_at` which calls it. But for consistency here I will change it to `uint` and add a cast in `shrink_at`.

For the default values, on expand it is historic but can now be removed. Good catch, and for inactive I don't have any good excuse =)

-------------

PR: https://git.openjdk.java.net/jdk/pull/1141