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

[Joseph Provino]

Is review complete?  Can I use Ramki and Per as reviewers?

thanks.

joe

On 4/10/2015 11:04 AM, Joseph Provino wrote:
> Per, your analysis seems to be right on the money.
>
> After initializing the variable and putting in asserts in join/leave
> and removing the asserts (which did indeed get hit) in 
> synchronize/desynchronize
> it passes jprt.
>
> Here's the latest webrev for review:
>
> http://cr.openjdk.java.net/~jprovino/7006810/webrev.03
>
> Thanks for all the reviews.
>
> joe
>
> On 4/9/2015 4:00 PM, Srinivas Ramakrishna wrote:
>>
>>
>> On Thu, Apr 9, 2015 at 2:00 AM, Per Liden <per.liden at oracle.com 
>> <mailto:per.liden at oracle.com>> wrote:
>>
>>     Hi Joe/Ramki,
>>
>>     On 2015-04-09 09:58, Srinivas Ramakrishna wrote:
>>
>>         Hi Joe --
>>
>>         On Wed, Apr 8, 2015 at 4:27 PM, Joseph Provino
>>         <joseph.provino at oracle.com <mailto:joseph.provino at oracle.com>
>>         <mailto:joseph.provino at oracle.com
>>         <mailto:joseph.provino at oracle.com>>> wrote:
>>
>>         ...
>>             On 4/6/2015 2:20 PM, Srinivas Ramakrishna wrote:
>>
>>             ...
>>
>>                 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.
>>
>>
>>     Even if the STS allows nested joins (or rather, today it doesn't
>>     check for it), to the best of my knowledge we don't have any such
>>     threads. Joe, I suggest that you look at the threads using STS
>>     (not that many) to confirm that this is the case. I don't think
>>     we should allow it, and therefor I think the thread state should
>>     be kept as a bool rather than a counter. As I mentioned in my
>>     other reply, there's a bug here. Since _suspendible_thread is
>>     never properly initialized it might look like we have already
>>     joined, when in fact we have just forgot to initialize it. I
>>     suspect that's the reason why it might look like we have nested
>>     joins.
>>
>>
>> Ah, OK. Hopefully Joe's testing after fixing the initialization 
>> confirmed that that was what was triggering the assert. I agree that 
>> it makes sense to keep it simple and disallow nested joins, if 
>> possible. (Haven't looked at the exit protocol carefully, but I will, 
>> as well as looking at the newer webrev.)
>>
>> -- ramki
>

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