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

[Coleen Phillimore]

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

New title:

8251336: OopHandle release can not be called in a safepoint
Summary: Release OopStorage oops for threadObj for exiting threads 
outside the service lock region that is marked as safe for safepoint.

Thanks,
Coleen

On 8/11/20 10:15 AM, Coleen Phillimore wrote:
>
>
> On 8/11/20 9:56 AM, Kim Barrett wrote:
>>> On Aug 11, 2020, at 7:40 AM, Coleen Phillimore 
>>> <coleen.phillimore at oracle.com> wrote:
>>>
>>> Summary: Release OopStorage oops outside a safepoint.
>>>
>>> Tested with tier1-6.
>>>
>>> open webrev at 
>>> http://cr.openjdk.java.net/~coleenp/2020/8251336.01/webrev
>>> bug link https://bugs.openjdk.java.net/browse/JDK-8251336
>>>
>>> Thank you to Zhengyu for alerting me about this issue.
>>>
>>> Coleen
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/runtime/serviceThread.cpp
>>   240       oop_handles_to_release = NULL;
>>
>> This line isn't needed.  We're going to exit the scope of that variable.
>> But see below.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/runtime/serviceThread.cpp
>>   184       // The ServiceThread is blocked here so runs during a 
>> safepoint.
>>
>> This comment doesn't make any sense to me.  This *is* the
>> ServiceThread, so to be executing this code it's clearly *not* blocked
>> in any usual sense of that word.
>>
>> I think more accurate is that it's marked as _thread_blocked, so could
>> be running during a safepoint.
>>
>> But see below.
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/runtime/serviceThread.cpp
>>    72   MutexLocker ml(Service_lock, Mutex::_no_safepoint_check_flag);
>>
>> release_oop_handles isn't manipulating any shared state, so doesn't
>> need any locking.  And that's good, because we'd rather not be holding
>> it across the list deletion.
>>
>> But see below.
>>
>> [And I see David mentioned this too.]
>>
>> ------------------------------------------------------------------------------ 
>>
>> src/hotspot/share/runtime/serviceThread.cpp
>>
>> I think more in keeping with the style elsewhere in this code would be
>> to just have a boolean flag that there are handles to process, and to
>> have release_oop_handles briefly take the Service_lock (again) to grab
>> the current list.  Doing so would eliminate the code related to the
>> first two comments above, and change things for the third.
>>
>> The cost is a brief extra grab of Service_lock in the presumably
>> uncommon situation where there are list entries to deal with.
>
> This style matches the jvmti code, but sure, I could change it like 
> this.  Then I don't need the temporary list.
>
> thanks,
> Coleen
>>
>> You're call.
>>
>> ------------------------------------------------------------------------------ 
>>
>>
>