RFR: 8221102: Allow GC threads to participate in threads claiming protocol

Roman Kennke rkennke at redhat.com
Wed Mar 27 08:37:05 UTC 2019


>>> OK.  So does that count as a "review"?  And I still need a second.
>>
>> No, not yet.
>>
>> I've tested it against my failing stress test (together with your njt_lock patch) and it seems to fix it.
>>
>> I'm also testing it against Shenandoah test suite, and so far looks good.
> 
> OK, thanks for doing the additional testing.
> 
>> I don't quite understand the resetting logic though. It first increments the global counter. If it turns 0, it resets all thread tokens to 0, using the claim_oops_do/cmpxchg protocol, but that is now expecting 0 too? Why not simply assign 0 instead? Maybe that code just isn't obvious enough…?
> 
> There isn't a setter for the per-thread token, only the claim
> function. But note that we're using the non-parallel variant (the
> first argument is false), so it effectively is just a simple
> assignment to the new claim value (when not already that value), with
> no cmpxchg involved.

Ah, ok. Well, I don't like methods that behave differently on a boolean 
flag argument for a reason.

>> Also, the resetting logic warrants a test, it is so rare, it will otherwise not get executed, but if it fails, it would be catastrophic.
> 
> I was hoping the resetting code was simple enough to not require a
> test.  I will see if I can come up with a sensible gtest.

I guess the 'design' of the claiming protocol is just making almost 
everything not-obvious. I think for this reason alone I shall go ahead 
and work on limiting/eliminating its usage outside of thread.cpp anyway.

If it's too hard to come up with a gtest, then go ahead with your 
proposed change.

Roman


More information about the hotspot-dev mailing list