RFR (S) 8251336: Shenandoah: assert "only get here when SATB active" after JDK-8244997

[Coleen Phillimore]

On 8/11/20 6:00 PM, David Holmes wrote:
> On 12/08/2020 7:02 am, Coleen Phillimore wrote:
>>
>>
>> On 8/11/20 4:45 PM, Coleen Phillimore wrote:
>>>
>>>
>>> On 8/11/20 1:48 PM, Kim Barrett wrote:
>>>>> On Aug 11, 2020, at 10:41 AM, Coleen Phillimore 
>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>
>>>>>
>>>>> Kim's suggestion for this change looks really good.  I'm 
>>>>> re-testing this now:
>>>>>
>>>>> open webrev at 
>>>>> http://cr.openjdk.java.net/~coleenp/2020/8251336.02/webrev
>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8251336
>>>> In release_oop_handles, because Service_lock is a "special" lock and
>>>> touched in lots of places, I'd prefer the deletion not happen under
>>>> the lock.  (I realize this uglifies the code a little bit.)
>>>
>>> Because there was no performance reason to use lock free code for 
>>> this, I add to and clean out the linked list under the Service_lock. 
>>> I chose a simple implementation because there was no performance or 
>>> correctness issue to do otherwise.
>>>
>>> I'd rather not change this to be ugly unless it's proven to not be 
>>> correct or performant.
>>
>> Scratch that.  I see what you mean. Like:
>>
>> http://cr.openjdk.java.net/~coleenp/2020/8251336.03.incr/webrev/index.html 
>>
>
> Yep that looks good. General principle of locking is to always try to 
> hold the lock for the minimal amount of time necessary.
>> (sorry I had some other questions about making this lock free).
>
> Given the service thread has to be notified about the presence of work 
> there's no point in trying to make the access lock free, as we're 
> going to be holding the lock anyway.

Yes, to both. Thanks David.
Coleeen

>
> Cheers,
> David
>
>> I'm re-testing this version now.
>>
>> Thanks,
>> Coleen
>>>
>>> thanks,
>>> Coleen
>>>
>>>>
>>>> Other than that, this looks good.
>>>>
>>>
>>