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

[Stefan Johansson]

On Mon, 16 Nov 2020 12:33:10 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that changes the way how archive regions are managed in general and specifically by the G1 collector, fixing the crashes caused by adding the module graph into the archive in [JDK-8244778](https://bugs.openjdk.java.net/browse/JDK-8244778)?
>> 
>> Previously before the JDK-8244778 change, archived objects could always be assumed as live, and so the G1 collector did so, not caring about the archive region's contents at all. With JDK-8244778 however, archived objects could die, and keep stale references to objects outside of the archive regions, which obviously causes crashes when walking these objects.
>> 
>> With this change, open archive region contents are basically handled as any other objects; to support that, all open archive regions are now reachable via a single object array root. This hopefully also facilitates implementation in other collectors.
>> 
>> This allows us to remove quite a bit of special handling in G1 too; the only difference is that open archive regions will generally not be collected unless they are completely empty: we do want to profit from the sharing across VMs as much as possible.
>> 
>> Testing: tier1-5, one or two 6-8 runs
>> 
>> The appcds changes were done by @iklam. These changes are described in this document: https://wiki.openjdk.java.net/display/HotSpot/CDS+Archived+Heap+Improvements
>> 
>> Thanks,
>>   Thomas
>
> 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.

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.

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp line 1266:

> 1264:     _g1h->remove_from_old_gen_sets(cl.old_regions_removed(),
> 1265:                                    cl.archive_regions_removed(),
> 1266:                                    cl.humongous_regions_removed());

If we want to go with the one call per set approach, here we could just atomically add these to a task-counter for each type and then do the calls to update the sets after the task has finished.

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.

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

Changes requested by sjohanss (Reviewer).

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