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

[Srinivas Ramakrishna]

Hi Joe --

On Wed, Apr 8, 2015 at 4:27 PM, Joseph Provino <joseph.provino at oracle.com>
wrote:

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

That would imply (along with the use of the scoped join/leave abstraction)
that nested joins/leaves are allowed.
I'd suggest then, that a thread keep track of the number of times it joined
and left by using a full-fledged
counter instead of a boolean bit.
Then your asserts would check that it wasn't trying to leave when its count
was 0.

I'll look at yr new webrev tomorrow.

thanks.
-- ramki


>
> 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
> > wrote:
>
>> CR:  https://bugs.openjdk.java.net/browse/JDK-7006810
>>
>> webrev:  http://cr.openjdk.java.net/~jprovino/7006810/webrev.00
>>
>> test:  jprt
>>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150409/a076998d/attachment.htm>