RFR(S): 7099824: G1: we should take the pending list lock before doing the remark pause

[John Cuthbertson]

Hi Everyone,

Thanks for all the discussion. I'll push the changes in the current form 
with the possibility of cleanups at a later date. I like the idea of 
checking that the PLL is being held on behalf of the GC before we start 
pushing the references onto the pending list. I'll submit an RFE for that.

Thanks again everyone.

JohnC

On 10/20/11 10:35, Y. S. Ramakrishna wrote:
> Tony, yes, i kind of appreciate your caution here, and I agree that
> the performance impact of the extra locking is vanishingly small.
> It just seemed a bit awkward to take a lock for no good reason.
> I do not see cleanup pauses needing to manipulate the reference handler's
> pending list even in the future, unless the pending list
> is explicitly manipulated during such a phase, something that
> we will do with some caution. Of course the question is why would
> we not make the same mistake again :-) One thing we will want to do
> to avoid such mistakes in the future (after all this was found
> because of a clean-up that stumbled upon the deficiency during
> a visual inspection of related code) is to have the GC code that
> handles the pending list check that the pending list lock is
> held on behalf of GC (i.e. either by a thread that requested this
> GC -- it is possible, albeit perhaps a bit messy, to keep that dependency
> chain -- or by the SLT thread on behalf of the VM/GC thread)
> in the ref processor's method that places the processed refs on
> the pending list.
>
> Anyway, none of these changes are needed here. I am fine with the
> code going in in the current state, but just some remarks for perhaps
> a future clean-up/mod, may be...
> -- ramki
>
> On 10/20/11 09:25, Tony Printezis wrote:
>> Ramki,
>>
>> I discussed this with John, the motivation was to avoid any phantom 
>> mistakes in the future if for some reason we really need to take the 
>> PLL before we do the cleanup pause and we forget to do so. Given that 
>> any effect I'd guess would be marginal / non-existent (cleanup pauses 
>> are really not very frequent) I'd like to play it safe here.
>>
>> Tony
>>
>> On 10/19/2011 08:23 PM, Ramki Ramakrishna wrote:
>>> Hi John, I don't think it would make any performance (latency) 
>>> difference (given how
>>> the reference handler thread takes the lock today). It just seemed
>>> unnecessary, that's all.... OK to take it for all CGC ops, if it helps
>>> keep the code from being messy... your subjective choice (anyone
>>> else have an opinion on code maintainability/robustness here because
>>> of the extra option/attribute in the CGC op c'tor?)
>>>
>>> -- ramki
>>>
>>> On 10/19/2011 5:09 PM, John Cuthbertson wrote:
>>>> Hi Ramki,
>>>>
>>>> Thanks for the review. Do you think that waiting for the pending 
>>>> list lock will significantly delay cleanups? If so then adding an 
>>>> extra "needs_pll" flag to VM_CGC_Operation is an easy change.
>>>>
>>>> Thanks,
>>>>
>>>> JohnC
>>>>
>>>> On 10/19/11 16:50, Ramki Ramakrishna wrote:
>>>>> Hi John -- This looks good to me, although I didn't see any 
>>>>> obvious reason to make clean up pauses take the
>>>>> PLL lock given they do not mess with the pending list in any 
>>>>> manner. Otherwise looks good to me.
>>>>>
>>>>> -- ramki
>>>>>
>>>>> On 10/17/2011 2:36 PM, John Cuthbertson wrote:
>>>>>> Hi Everyone,
>>>>>>
>>>>>> Can I have a couple of volunteers review these changes? The 
>>>>>> webrev can be found at: 
>>>>>> http://cr.openjdk.java.net/~johnc/7099824/wevrev.0/
>>>>>>
>>>>>> Summary: During a G1 remark pause, the JVM may enqueue some 
>>>>>> discovered references on to the pending list. Whenever the JVM 
>>>>>> modifies the pending list, it is supposed to synchronize with the 
>>>>>> ReferenceHandler thread and acquire the pending list lock but 
>>>>>> G1's concurrent GC operations were not doing so.
>>>>>>
>>>>>> Testing: the GC test suite with both CMS and G1, and jprt.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> JohnC
>>>>