RFR(S): 7147724: G1: hang in SurrogateLockerThread::manipulatePLL

[Bengt Rutisson]

Hi John,

Looks good! You are of course right about the negation for the 
safepoint_check_flag. :-)

Copyright year in vm_operations_g1.hpp needs an update. ;-)

Ship it!
Bengt

On 2012-03-10 00:54, John Cuthbertson wrote:
> Hi Bengt,
>
> Thanks for looking again. Replies inline....
>
> On 03/09/12 02:25, Bengt Rutisson wrote:
>>
>> Hi John,
>>
>> I think I like this change, but either I am confused or you forgot to 
>> remove the "bool no_safepoint_check" from:
>>
>> G1CollectedHeap::humongous_obj_allocate_find_first()
>> G1CollectedHeap::humongous_obj_allocate()
>>
>> Did you mean to remove it?
>
> Yes I did mean to remove it. Done.
>
>>
>> Just a style comment on this piece of code:
>>
>>     bool safepoint_check_flag = Mutex::_no_safepoint_check_flag;
>>     if (Thread::current()->is_Java_thread()) {
>>       safepoint_check_flag = !Mutex::_no_safepoint_check_flag;
>>     }
>>
>> I understand that we want to use Mutex::_no_safepoint_check_flag 
>> since it is defined. But since there is no 
>> Mutex::_safepoint_check_flag or similar I think we break the 
>> abstraction by doing "safepoint_check_flag = 
>> !Mutex::_no_safepoint_check_flag". This kind of exposes that we are 
>> dealing with a bool. So, given this I think I would prefer to change 
>> the code to just:
>>
>> safepoint_check_flag = Thread::current()->is_Java_thread();
>
> OK. Done, but with a small difference. For Java threads we wish to 
> pass false so that we don't skip the safepoint check so I used "bool 
> no_safepoint_check_flag = !Thread::current()->is_Java_thread();" and 
> can then just pass in the value.
>
> New webrev: http://cr.openjdk.java.net/~johnc/7147724/webrev.2/
>
> Thanks,
>
> JohnC