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

[Coleen Phillimore]

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