On Wed, 11 Nov 2020 11:39:59 GMT, Thomas Schatzl <tscha...@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
