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

[John Cuthbertson]

Hi Tony, Ramki,

Thanks for the reviews. I'll push the change that's in the current 
webrev and add the patch which stops pushing the discovered field twice 
to the CR that Tony has filed.

Thanks,

JohnC

On 12/2/2011 2:05 AM, Tony Printezis wrote:
> 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