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

[Kim Barrett]

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

You're call.

------------------------------------------------------------------------------