RFR (XXS): 7006810: G1: Introduce peace-of-mind checking in the Suspendible Thread Set

[Joseph Provino]

On 4/6/2015 2:20 PM, Srinivas Ramakrishna wrote:
> Hi Joe --
Hi Ramki, nice to hear from you and thanks for the review.
>
> I am not terribly familiar with G1 code yet, but here's a suggestion:
>
> * rename the boolean field to "in_suspendible_thread_set" (or 
> "suspendible_thread"), the corresponding setters and gettes to
> set_in_suspendible_thread_set(), clear_in_suspendible_thread_set(), 
> and is_in_suspendible_thread_set(), or preferably:
> set_suspendible_thread(), clear_suspendible_thread() and 
> is_suspendible_thread(). (I don't particularly like the set_...._set() 
> naming.)

I changed the variable and the methods as you suggested.  Much easier on 
the eyes.

DEBUG_ONLY(bool _suspendible_thread;)

>
> Do we allow threads to recursively enter the suspendible thread set in 
> nested fashion? I'm guessing we don't, in which case, you would
> use this information local to the thread to prevent/forbid such 
> recursive entries or exits when it isn't part of the set, by asserting
> that the bit isn't set when a thread joins the set and never clear 
> before a thread leaves the set. I understand that there's a scoped
> join/leave abstraction via SuspendibleThreadSetJoiner, but there also 
> seem to be a bunch of naked join/leave calls, so the
> extra checking would likely be worthwhile.
This is a good question.  If I put in checks in join / leave

void SuspendibleThreadSet::join() {
   assert(Thread::current()->is_suspendible_thread() == false,
     "Thread already joined");

void SuspendibleThreadSet::leave() {
   //assert(Thread::current()->is_suspendible_thread(),
   //  "Thread not joined");

jprt hits the assert in join.

It doesn't seem right to call join if a thread is already joined.
Would it be correct to just return if trying to rejoin or trying to 
leave when not joined?

What's there seems to be harmless but I can't be sure of that.

joe
>
> As Eric notes, you could make all the new fields/methods NOT_PRODUCT, 
> but I am not too concerned about leaving them in product mode
> as well, since the frequency of transition to and from STS mode is 
> probably fairly low, and the extra information may be useful for
> field serviceability. But I will let those more familiar with G1 opine 
> more strongly on that.
>
> Looks good otherwise.
>
> Thanks!
> -- ramki (openjdk: ysr)
>
>
>
> On Mon, Apr 6, 2015 at 10:25 AM, Joseph Provino 
> <joseph.provino at oracle.com <mailto:joseph.provino at oracle.com>> wrote:
>
>     CR: https://bugs.openjdk.java.net/browse/JDK-7006810
>
>     webrev: http://cr.openjdk.java.net/~jprovino/7006810/webrev.00
>     <http://cr.openjdk.java.net/%7Ejprovino/7006810/webrev.00>
>
>     test:  jprt
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150408/68bfa096/attachment.htm>