RFR (XS): 7114095: G1: assert(obj == oopDesc::load_decode_heap_oop(p)) failed: p should still be pointing to obj

[John Cuthbertson]

Hi Ramki,

Response inline....

On 12/01/11 01:08, Srinivas Ramakrishna wrote:
> Hi John -- I am wondering if we should simply prevent the second 
> closure application if we have already
> done it once. That is easy to set-up. Of course such a test penalizes 
> all collectors when they scan Reference objects, but perhaps the cost 
> of the test
> will be lost in the noise of all the other test-logic when scanning 
> Reference objects...

Yes this is straight forward to do - but as you said it will add an 
additional clause to the if-condition that's around the second 
application. I have verified that this does seem to fix the problem. If 
I go with this approach - do you foresee any issues with skipping the 
second application of the closure to the discovered field? I don't think 
there is but it's worth asking the original author. :) I went with the 
fix that's in the webrev because it only touches G1 code.

> But since G1 deals with multiple scans anyway, perhaps this is not 
> worth worrying over.
>
> I also have some related questions: what forces G1 to have to deal 
> with the multiple scan possibility anyway?
> I had previously incorrectly assumed that G1's remembered set scanning 
> also avoids multiple scans of any
> reference...

During RSet scanning - the closure claims a group of cards. That along 
with a couple of "is_in_cset" tests seem to be enough to prevent 
multiple scans of the same reference field (i.e. pushing the same 
reference field on to the _refs_to_scan queue more than once).

> In the scenario that you outlined, when the second thread processes 
> the reference the second
> time, how does it determine that it should do nothing? (I guess I am 
> asking for a pointer to the
> code that does the processing of the stack elements and does the 
> recursive copying.)

The failing assert indicates that reference fields should only be pushed 
on to the _refs_to_scan queue once and mostly that seems to be the case. 
After an object is evacuated, the worker that successfully installs the 
forwarding pointer iterates over the reference fields of the evacuated 
object, applying the G1ParScanClosure. Since this is done by the thread 
that claims the object - we shouldn't see multiple pushes of the same 
reference field unless the closure is applied to the reference field 
multiple times.  The discovered field of reference objects is the only 
field I've seen being pushed twice.

As reference fields are popped from the _refs_to_scan queue, a copy 
closure is applied to them. This takes place in 
G1ParScanThreadState::deal_with_reference(). I believe the call in 
question comes from G1ParScanThreadState::trim_queue(). The copy closure 
dereferences the field and first checks that the referenced object is in 
the CSet before attempting to evacuate the object. This code is in 
G1ParCopyClosure::do_oop_work() and 
G1ParCopyHelper::copy_to_survivor_space() in g1CollectedHeap.cpp.

The evacuation code has to handle multiple reference fields that point 
to the same object (an already evacuated object will not pass the 
"is_in_cset" check), and it looks like it can handle seeing the same 
reference field twice. The worst that I think can happen is that the 
reference field may be updated twice (to point to the same evacuated 
object).

A while back, the evacuation code took a fourth template parameter - a 
boolean that indicated whether the "is_in_cset" check could be skipped, 
i.e. it was assumed that the closure would only be applied to objects in 
the collection set. If we still had that code then we wouldn't be able 
to handle this scenario.

Does this answer your questions?

Thanks,

JohnC