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

[David Holmes]

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.

Cheers,
David

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