Bug ID: JDK-8270841 G1: No need to check "in cset" in PSS::do_oop

Type: Enhancement
Component: hotspot
Sub-Component: gc
Affected Version: 18

Priority: P4
Status: Closed
Resolution: Not an Issue

Submitted: 2021-07-16
Updated: 2021-07-20
Resolved: 2021-07-20

It seems to me that the comments and check does not make sense any more, or I missed something?

diff --git a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
index 9eebe411936..b90d18f8bb2 100644
--- a/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
+++ b/src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
@@ -178,11 +178,6 @@ void G1ParScanThreadState::do_oop_evac(T* p) {
   // Reference should not be NULL here as such are never pushed to the task queue.
   oop obj = RawAccess<IS_NOT_NULL>::oop_load(p);
 
-  // Although we never intentionally push references outside of the collection
-  // set, due to (benign) races in the claim mechanism during RSet scanning more
-  // than one thread might claim the same card. So the same card may be
-  // processed multiple times, and so we might get references into old gen here.
-  // So we need to redo this check.
   const G1HeapRegionAttr region_attr = _g1h->region_attr(obj);
   // References pushed onto the work stack should never point to a humongous region
   // as they are not added to the collection set due to above precondition.
@@ -190,10 +185,7 @@ void G1ParScanThreadState::do_oop_evac(T* p) {
          "Obj " PTR_FORMAT " should not refer to humongous region %u from " PTR_FORMAT,
          p2i(obj), _g1h->addr_to_region(cast_from_oop<HeapWord*>(obj)), p2i(p));
 
-  if (!region_attr.is_in_cset()) {
-    // In this case somebody else already did all the work.
-    return;
-  }
+  assert(region_attr.is_in_cset(), "Must be!");
 
   markWord m = obj->mark();
   if (m.is_marked()) {

I see, thanks a lot for your detailed explanation.

20-07-2021

The requirement for this check is still there - remembered set scan might in some rare cases scan objects multiple times. Consider a regular object instance spanning multiple cards; it is marked (imprecisely) at the object header and e.g. in the tail card due to other objects needing scanning. The object spans multiple scan chunk (i.e. multiple threads process parts of it). Then it is possible that two threads scan that object at the same time: both thread A and B process it in full, pushing its oop* to the queues, allowing that oop* to be processed multiple times. Obviously once it will already have been forwarded, pointing to somewhere not in the cset. The card table scan code has been intentionally designed this way (and looking at it there is still no code preventing this situation) because guaranteeing this property resulted in slower operation of the card table scan code when trying - but it would be an interesting task to see if that could be done better. Unless there is further input from you why this is not the case, I'm going to close this suggestion soon.

19-07-2021