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

[Kim Barrett]

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.