RFR: 8253081: G1 fails on stale objects in archived module graph in Open Archive regions [v3]

[Thomas Schatzl]

On Tue, 17 Nov 2020 10:16:28 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Thomas Schatzl has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>> 
>>  - Merge branch 'master' of https://git.openjdk.java.net/jdk into 8253081-null-narrow-klass-changes2
>>  - kbarrett review
>>  - Initial import
>
> I've been focusing on the GC-parts and it looks good in general just a few comments.

Ran tier1-4, tier5-gc with the changes from ea78aa1 (contributed by @iklam after some discussion).

> src/hotspot/share/gc/g1/g1FullGCPrepareTask.hpp line 60:
> 
>> 58:     G1FullGCCompactionPoint* _cp;
>> 59:     uint _humongous_regions_removed;
>> 60:     uint _open_archive_regions_freed;
> 
> Since we no longer use these counters to update the sets, it is enough to just have one counter to track if any region has been freed. Or use a boolean if you prefer that.

Fixed.

> src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 4458:
> 
>> 4456:   heap_region_iterate(&cl);
>> 4457: 
>> 4458:   remove_from_old_gen_sets(0, 0, cl.humongous_regions_reclaimed());
> 
> Looking at this call and now having three parameters that are "optional" for `remove_from_old_gen_sets()` I wonder if it would be cleaner to have three functions, one for each set. It would increase the number of times we take the look, but we could restructure the code in `G1ReclaimEmptyRegionsTask` to not do the updates in the worker threads and that way only take the lock when there will be no contention. 
> 
> If you fell like this is outside the scope of this change, please file an issue instead.

I am not very clear on what the problem there is and how the parameters are optional. I do not completely understand how adding an extra method just for this caller would improve the code significantly.

I'll opt to defer this cleanup to a separate CR.

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

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