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

[Tony Printezis]

John,

The change is fine, please ship it.

Having said that, I would ultimately like to ensure that during a GC we 
guarantee to scan each field only once (IIRC, all other GCs do that). 
Right now, as John pointed out in a previous e-mail, the code is robust 
with regards to fields being scanned twice. The reason this can happen 
(apart from the scenario described here) is that when we scan cards 
pointed to by RSets (and a single card might be included in different 
RSets that are scanned separately) we don't atomically claim each card 
(we put an indicator that a thread is scanning the card but not with an 
atomic operation). The reason this was done was to cut down on atomic 
operations during GC and it assumes that the number of cards that will 
be scanned twice will probably be quite small (which is most likely 
true). Unfortunately, in order for the code to be robust wrt being able 
to scan the same field twice (and keep in mind this happens really 
infrequently) we have to pay the penalty of an extra "is in CSet" test 
for each oop* we push/pop on/from the taskqueue (and said test is a bit 
more expensive in G1 compared to the "is young" test in the other GCs). 
If we can go back to guaranteeing that each field is scanned once, we 
can get rid of one of those tests and speed up / simplify the GC 
operation a bit. We should try to do that. I'll open a CR. But, John, 
meanwhile please push this.

Tony

On 11/30/2011 02:23 PM, John Cuthbertson wrote:
> Hi Everyone,
>
> Can I have a couple of volunteers look over the fix for this CR? The 
> webrev can be found at: 
> http://http://cr.openjdk.java.net/~johnc/7114095/webrev.0/
>
> Summary:
> As a result of the changes for 4965777, we could apply the 
> G1ParScanClosure twice to the discovered field of a reference object. 
> Once because the closure itself has the apply_to_weak_discovered_field 
> attribute (as I think it should) and the other because of the changes 
> for 4965777. This causes the discovered field of an evacuated 
> reference object to be pushed to the _refs_to_scan queue twice. 
> Normally this is not a problem as the evacuation code can handle 
> seeing the same object more than once. In this case however, if the 
> result of the first push is stolen and the referenced object 
> successfully evacuated by another worker thread, then the assert is 
> too strong.  The other worker will update the discovered field to 
> point to the new location while the current thread is pushing the 
> discovered field for the second time. As a result when the current 
> thread executes the assert, the discovered field could have been 
> updated and trips the assert.
>
> The solution is to weaken the assert slightly.
>
> Testing: Both failing test cases running continuously overnight (they 
> typically fail within a few iterations); the GC test suite with a 
> delay in the offending code; jprt.
>
> Thanks,
>
> JohnC