On Tue, 17 Nov 2020 10:55:19 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 > > Thomas Schatzl has updated the pull request incrementally with two additional > commits since the last revision: > > - sjohanss review > - Remove code that "activates" dormant objects as now all active objects are > reachable via the root object array Looks good! I took a sweep through the code and have some nits that you may choose to ignore. src/hotspot/share/gc/g1/g1CollectedHeap.cpp line 4538: > 4536: } else { > 4537: // We ignore free regions, we'll empty the free list afterwards. > 4538: assert(hr->is_free(), Can make this one line src/hotspot/share/memory/heapShared.cpp line 334: > 332: } > 333: > 334: // Returns an objArray that contains all the roots of the archived > objects It does..? src/hotspot/share/memory/heapShared.cpp line 413: > 411: int length = _pending_roots != NULL ? _pending_roots->length() : 0; > 412: int size = objArrayOopDesc::object_size(length); > 413: Klass *k = Universe::objectArrayKlassObj(); // already relocated to > point to archived klass `Klass* k` ------------- Marked as reviewed by redestad (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1163
