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

[Kim Barrett]

On Wed, 11 Nov 2020 11:39:59 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

I've only skimmed the non-GC changes.

src/hotspot/share/oops/klass.cpp line 207:

> 205:                            _shared_class_path_index(-1) {
> 206:   CDS_ONLY(_shared_class_flags = 0);
> 207:   CDS_JAVA_HEAP_ONLY(_archived_mirror_index = -1);

Why are the semi-colons being moved out of the macros here?  This isn't needed, and is contrary to usage elsewhere.

src/hotspot/share/memory/heapShared.hpp line 70:

> 68:   GrowableArray<int>* _subgraph_entry_fields;
> 69: 
> 70:   // Does this KlassSubGraphInfo belong to the arcived full module graph

s/arcived/archived/

src/hotspot/share/memory/heapShared.hpp line 85:

> 83:     _is_full_module_graph(is_full_module_graph),
> 84:     _has_non_early_klasses(false) {}
> 85:   ~KlassSubGraphInfo() {

Please add a blank line between the constructor and the destructor.

src/hotspot/share/gc/g1/heapRegion.hpp line 181:

> 179:   // Returns whether the given object is dead based on TAMS and bitmap.
> 180:   // An object is dead iff a) it was not allocated since the last mark, b) it
> 181:   // is not marked, and c) it is not in a closed archive region.

The first, unchanged, line isn't consistent with the additional comment.  I suggest ending it after "dead", and adding "(TAMS)" and "(bitmap)" before the ending commas of the first two alternatives.

src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 60:

> 58:       // Never free closed archive regions. This is also be the only other allowed
> 59:       // type at this point.
> 60:       assert(hr->is_closed_archive(), "Only closed archive regions can also be pinned.");

I found the assert message here very confusing.  It's really that all other pinned region cases have been covered, and closed_archive is the last remaining one.

src/hotspot/share/gc/g1/g1FullGCPrepareTask.cpp line 128:

> 126: }
> 127: 
> 128: void G1FullGCPrepareTask::G1CalculatePointersClosure::free_pinned_region(HeapRegion* hr) {

Should this be called free_archive_region (or free_open_archive_region)?  The statistics counter is `_pinned_archive_regions_removed`, so this presumably can't be used for some other kind of pinned region.

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

Changes requested by kbarrett (Reviewer).

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