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