RFR: 8062036: ConcurrentMarkThread::slt may be invoked before ConcurrentMarkThread::makeSurrogateLockerThread causing intermittent crashes

[Bengt Rutisson]

Hi Kim,

The change looks good. Thanks for fixing this.

Regarding assignments in if statements; I thought we had a style where 
we use booleans for comparisons. This means that I would prefer Jon's 
suggestion since the comparison will be on a bool value rather than 
relying on that NULL is interpreted as false.

The hotspot style guidelines does not cover a lot and I guess it is open 
for interpretation whether or not this case is covered. But there is a 
short sentence about booleans:
https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#StyleGuide-Booleans

I interpret "use true & false (not 1 & 0)" as that NULL should also not 
be used as a bool value.

Thanks,
Bengt

On 2014-11-03 23:13, Kim Barrett wrote:
> On Nov 3, 2014, at 4:31 PM, Jon Masamitsu <jon.masamitsu at oracle.com> wrote:
>> Kim,
>>
>> Change looks good.  I can sponsor.  Let me know when the reviews
>> are complete.
> Thanks.
>
>> Some suggestions.
>>
>> http://cr.openjdk.java.net/~kbarrett/8062036/webrev/src/share/vm/gc_implementation/g1/vm_operations_g1.cpp.frames.html
>>
>>> 216   if (SurrogateLockerThread* slt = ConcurrentMarkThread::slt()) {
>>> 217     slt->manipulatePLL(SurrogateLockerThread::acquirePLL);
>>> 218   } else {
>>> 219     SurrogateLockerThread::report_missing_slt();
>>> 220   }
>> I have a preference for
>>
>> SurrogateLockerThread* slt = ConcurrentMarkThread::slt();
>> if (slt != NULL) {
>> slt->manipulatePLL(SurrogateLockerThread::acquirePLL);
>> } else {
>> SurrogateLockerThread::report_missing_slt();
>> }
> I like declarations in conditions (also applicable to switch, for, and while statements) because it limits the scope of the declared variable.  Though I tend to not use that form when the true case is large, because I dislike having an else clause that is a long way from the associated condition.  So I would typically write either
>
> if (T v = init()) {
> /* small amount of code using v */
> } else {
> /* code for !v */
> }
>
> or
>
> T v = init()
> if (!v) {
> /* small block of code for !v */
> } else {
> /* code using v */
> }
>
> or (pretty commonly)
>
> T v = init();
> if (!v) {
> /* small amount of code for !v, with break/return avoiding code using v */
> }
> /* larger block of code using v */
>
> And if I can’t do one of those I’d probably factor out into helpers to allow one of those.
>
> I’m willing to change my ways and drop this idiom though if folks dislike or find it too odd or difficult to read.
>
>> http://cr.openjdk.java.net/~kbarrett/8062036/webrev/src/share/vm/gc_implementation/shared/concurrentGCThread.hpp.frames.html
>>
>> 96   // Terminate VM with error message that SLT needed but not yet created.
>>
>> I think it would read better as
>>
>> "SLT needed but not yet created." => "SLT is needed but has not yet been created."
> Sure.  I guess this text *is* a bit more user-facing than an assert message, so grammar is more important.
>