RFR [L][4/7]: 8180415: Rebuild remembered sets during the concurrent cycle (original) (raw)

Thomas Schatzl thomas.schatzl at oracle.com
Thu Mar 8 11:49:20 UTC 2018

Hi Stefan,

On Thu, 2018-03-08 at 10:15 +0100, Stefan Johansson wrote:

Thanks for addressing my initial comments Thomas,

On 2018-03-07 09:59, Thomas Schatzl wrote: > Hi all, > > Stefan had some comments about the HeapRegionType parameter in > the G1RemSetTrackingPolicy::updateatallocate() method - that one > is not really required when placing the call to this method > correctly and just using the HeapRegion's type directly. > > Changing this removes another 40 LOC of changes. > > There has been another bug introduced by me during cleaning up for > final review: G1RemSetTrackingPolicy::updateatfree() is empty in > this change, which is wrong for this change - in this change we > still do not free the remembered sets during the cleanup pause, > only in the Concurrent Cleanup phase. When doing this concurrently, > an assert triggers when setting the remembered set state to empty > outside a safepoint. > The fix is to make the remembered set untracked during the Cleanup > phase still. > > New webrevs: > http://cr.openjdk.java.net/~tschatzl/8180415/webrev.0to1 (diff) > http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1 (full) The change looks good. Just some additional comments: src/hotspot/share/gc/g1/g1ConcurrentMark.cpp 1255 GCTraceTime(Debug, gc)("Complete Remembered Set Tracking"); ... 1291 GCTraceTime(Debug, gc)("Finalize Concurrent Mark Cleanup"); Please add the "phases" tag to the above logging to be more consistent with the remark and other pauses. --- src/hotspot/share/gc/g1/g1FullGCOopClosures.inline.hpp 52 template inline void G1AdjustClosure::adjustpointer(T* p) In this method the comments should be updated since we no longer return anything. --- src/hotspot/share/gc/g1/g1RemSet.cpp 757 HeapRegion* startregion = hr->humongousstartregion(); startregion is only used to get bottom and crate an oop, so maybe instead create the oop directly, like: oop humongous = oop(hr->humongousstartregion()->bottom()); 761 // A humongous object is live (with respect to the scanning) either 762 // a) it is marked on the bitmap as such 763 // b) its TARS is larger than nTAMS, i.e. has been allocated during marking. 764 if (cm->nextmarkbitmap()->ismarked(startregion- >bottom()) || 765 (topatrebuildstart > hr->nexttopatmarkstart())) { Break this check out to get something like: islive(humongous, tars, ntams).

Fixed.

There is also the markedbytes counting in this method that makes the code a bit harder to follow. I've discussed this with Thomas offline and he has an idea of how to avoid this.

As discussed I would like to postpone this for now as it is much harder than it looks and warrants its own CR (actually two or three).

Also in G1FullCollector::phase3adjustpointers() there is a GCTraceTime that needs updating, since we no longer rebuild remember sets.

All done.

New webrevs: http://cr.openjdk.java.net/~tschatzl/8180415/webrev.1_to_2/ (diff) http://cr.openjdk.java.net/~tschatzl/8180415/webrev.2/ (full)

Thomas

More information about the hotspot-gc-dev mailing list