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

[Y. S. Ramakrishna]

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
>>>