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

[Stefan Johansson]

On Wed, 18 Nov 2020 09:17:42 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Stefan Johansson has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits:
>> 
>>  - Merge branch 'master' into 8236926-ccu
>>  - Zoom feedback
>>  - Albert review 2
>>  - Albert review
>>  - Merge branch 'master' into 8236926-ccu
>>  - Lock for small mapper and use BitMap parallel operations.
>>  - Self review
>>  - Simplified task
>>  - Improved logging
>>  - Test improvement
>>  - ... and 5 more: https://git.openjdk.java.net/jdk/compare/3675653c...c354b1d8
>
> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 747:
> 
>> 745:   HeapRegion* prev_last_region = NULL;
>> 746:   size_t size_used = 0;
>> 747:   size_t shrink_count = 0;
> 
> The code may as well define `shrink_count` as `uint` as the only use seems to cast it to `uint` anyway.

I agree, I left it `size_t` since everything else in this function uses `size_t`, but `uint` is a better fit.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 806:
> 
>> 804:                               HeapRegion::GrainWords * HeapWordSize * shrink_count);
>> 805:     // Explicit uncommit.
>> 806:     _hrm.uncommit_inactive_regions((uint) shrink_count);
> 
> Please let the code use the `G1CollectedHeap::uncommit_regions()` helper here to limit the references to `_hrm`.

Good point, fixed.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 1312:
> 
>> 1310:                             shrink_bytes, aligned_shrink_bytes, shrunk_bytes);
>> 1311:   if (num_regions_removed > 0) {
>> 1312:     log_debug(gc, heap)("Regions ready for uncommit: %u", num_regions_removed);
> 
> Maybe keep with existing terminology here i.e. `Uncommittable regions after shrink: %u`

Sound good.

> src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 567:
> 
>> 565: 
>> 566:   // Check if there is memory to uncommit and if so schedule a task to do it.
>> 567:   void uncommit_heap_if_necessary();
> 
> I would prefer if the method were called `uncommit_regions_if_necessary()` as this method does not uncommit the *heap* but just uncomittable regions.

Done, I agree that is more accurate.

> src/hotspot/share/gc/g1/g1CollectedHeap.hpp line 568:
> 
>> 566:   // Check if there is memory to uncommit and if so schedule a task to do it.
>> 567:   void uncommit_heap_if_necessary();
>> 568:   uint uncommit_regions(uint region_limit);
> 
> Please add a comment like "// Immediately uncommits uncommittable regions.

Done.

> src/hotspot/share/gc/g1/g1CommittedRegionMap.hpp line 46:
> 
>> 44: };
>> 45: 
>> 46: class G1CommittedRegionMap : public CHeapObj<mtGC> {
> 
> It would be nice to have a diagram of the region states and their transitions here.

Good idea.

> src/hotspot/share/gc/g1/g1CommittedRegionMap.hpp line 98:
> 
>> 96:   // Finds the next range of committable regions starting at offset.
>> 97:   // This function must only be called when no inactive regions are
>> 98:   // present and can be used to active more regions.
> 
> s/active/activate

��

> src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1172:
> 
>> 1170:     reset_at_marking_complete();
>> 1171: 
>> 1172:     _g1h->uncommit_heap_if_necessary();
> 
> I'd prefer it this call were placed below the `resize_heap_if_necessary` call unless there is a reason not to.

Fixed.

> src/hotspot/share/gc/g1/g1FullCollector.cpp line 218:
> 
>> 216:   _heap->print_heap_after_full_collection(scope()->heap_transition());
>> 217: 
>> 218:   _heap->uncommit_heap_if_necessary();
> 
> Maybe move this to the `gc_epilogue` or actually into `prepare_heap_for_mutators` where the shrinking occurs.

Moved it to `prepare_heap_for_mutators`.

> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp line 81:
> 
>> 79:   }
>> 80: 
>> 81:   bool committed_range(uint start_idx, size_t num_regions) {
> 
> I would prefer if these two methods were named more like a condition, i.e. `is_range_committed` and `is_range_uncommitted` (optionally exchanging `range` and `[un]committed`) as this makes the verification they are used in read better.

Went with `is_range_[un]committed`.

> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp line 92:
> 
>> 90: 
>> 91:   virtual void commit_regions(uint start_idx, size_t num_regions, WorkGang* pretouch_gang) {
>> 92:     guarantee(uncommitted_range(start_idx, num_regions),
> 
> Not sure this (and the one in `uncommit_regions`) should be guarantees.

I went with `guarantee` since this is what's used in `G1PageBasedVirtualSpace` for similar checks. Errors like this might be more likely in release builds.

> src/hotspot/share/gc/g1/g1RegionToSpaceMapper.cpp line 138:
> 
>> 136:   // - G1RegionToSpaceMapper::_region_commit_map;
>> 137:   // - G1PageBasedVirtualSpace::_committed (_storage.commit())
>> 138:   Mutex _lock;
> 
> Good comment! :)

Thanks!

> src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp line 83:
> 
>> 81:   _summary_duration += time;
>> 82: 
>> 83:   log_trace(gc, heap)("Concurrent Uncommit: " SIZE_FORMAT "%s, %u regions, %1.3fms",
> 
> Maybe use gc+heap+region here (and below) like other code; imho after all this task is kind of an extension of the `HeapRegionManager`. I did not look at the output though.

My idea is to use `gc+heap+region` for transitions on a region level and `gc+heap` for the actual changes to the heap. See the PR description for some examples.

> src/hotspot/share/gc/g1/g1UncommitRegionTask.cpp line 128:
> 
>> 126:     // No delay, reason to reschedule rather then to loop is to allow
>> 127:     // other tasks to run without waiting for a full uncommit cycle.
>> 128:     schedule(0);
> 
> Why not notify like in `run()`? Maybe refactor the code a bit to allow calling the same method to schedule the task and notify the service thread for immediate processing here.

Because here we know that the service thread is running and if we schedule with a 0 delay, there is no risk of it going to sleep before we run again. Another task might be up next, but this task will eventually run before the service thread can go to sleep.

There is room for improvement on how we schedule tasks from another thread. I changed `enqueue` to call a new public function on the `G1ServiceThread` called `schedule_task`, which calls `schedule(task, delay)` and then `notify()`. The function `schedule(task, delay)` is what previously was named `schedule_task`. This is what will be called when someone does `task->schedule(delay)` as well, so it is a bit more unified.

> src/hotspot/share/gc/g1/g1UncommitRegionTask.hpp line 42:
> 
>> 40:   // while running on the service thread joined with the suspendible
>> 41:   // thread set.
>> 42:   bool _active;
> 
> The information in the comment is missing context on how scheduling and suspension works and how the `_active` flag interacts with `run()` requests. The MT safety seems an implementation detail. I.e. it would be nice to know that:
> * this task does its work in chunks, i.e. does not uncommit everything at once to avoid long stalls or issues with GCs interrupting it
> * this flag prevents rescheduling the task when it has not uncommitted everything yet but another request (`run` call) comes in.

Fair point, since I moved the 256M constant into the class, the chunking info is now in a comment for this constant. For the state I added the info around usage and moved the implementation detail into `set_active()`.

> src/hotspot/share/gc/g1/g1UncommitRegionTask.hpp line 58:
> 
>> 56: 
>> 57: public:
>> 58:   static void run();
> 
> I'd prefer if the method were named `schedule` (or something like `schedule_for_later` or maybe `enqueue` to not clash with the protected `schedule(jlong)` method) instead of `run` since imho `run` implies that it is actually executed.

I like `enqueue`, changed.

> src/hotspot/share/gc/g1/g1ServiceThread.hpp line 138:
> 
>> 136:   // Notify a change to the service thread. Used to stop either
>> 137:   // stop the service or to force check for new tasks.
>> 138:   void notify();
> 
> The first "stop" is too much

Good catch.

> src/hotspot/share/gc/g1/heapRegionManager.cpp line 181:
> 
>> 179:     G1CollectedHeap::heap()->hr_printer()->commit(hr);
>> 180:   }
>> 181:   activate_regions(start, num_regions);
> 
> In this place there will be two messages from the HRPrinter for every region:
> 1) a COMMIT message and
> 2) an ACTIVATE message
> 
> This is a bit confusing as in my understanding (that's why I asked for a region state diagram in the `G1CommittedRegionMap` which may as well be put in `HeapRegionManager`) the (typical) flow of states are Uncommitted->Committed/Active->Committed/Inactive->Uncommitted.
> As mentioned, I'm not sure if it is a good idea to send two separate messages here; better rename the "Active" message to "Commit-Active" (and "Inactive" to "Commit-Inactive") instead imho, even if it's quite long (and drop `HRPrinter::commit()` completely)

It's true that it will generate two messages when committing a previously uncommitted region, but I still think it is valuable to separate them since we can also have the state change `Active->Inactive->Active`. In this case the transition from Inactive to Active will not include a commit, but rather making inactive regions active again. Just seeing a "Commit-Active" message in this case would not be as clear as seeing "Active" that is not immediately preceded by "Commit".

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

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