RFR (S) 8213137: Remove static initialization of monitor/mutex instances

[Erik Österlund]

Hi David,

On 2018-11-07 09:45, David Holmes wrote:
> Hi Erik,
>
> Thanks for looking at this ;-)
>
> On 7/11/2018 6:00 PM, Erik Österlund wrote:
>> Hi David,
>>
>> I noticed that for each lock except Decoder::shared_decoder_lock() 
>> you removed the static lock and its static getter, rerouting 
>> references from them to the new locks managed in the mutexLocker files.
>>
>> Was there a particular reason why the getter for 
>> shared_decoder_lock() in particular remains, instead of just 
>> referencing the Decoder_shared_decoder_lock directly, as you did for 
>> all other locks?
>>
>> I suppose that by removing the getter, we would miss out on the 
>> assert that the lock is not NULL. But on the other hand, 1) nobody 
>> else checks if their global lock from mutexLocker is null, 2) if it 
>> ever was null, we would immediately get a SIGSEGV upon use, so I 
>> don't think that is particularly helpful.
>
> Yes it is because of the assert that checks for NULL. My intent was to 
> move the locks but leave the existing semantics unchanged. So I kept 
> the getter for the Decoder_lock and moved the assertion there to 
> simplify the other code that repeated the NULL checks. If you really 
> don't like this (and I agree this code is the odd one out here) then I 
> suggest it gets fixed in the follow up RFE that I filed: (JDK-8213399) 
> DecoderLocker is unused

That sounds like a good plan.

>> I also noticed that all other locks managed by the mutexLocker files 
>> have a CamelCase_lock convention. These locks are the first to break 
>> that convention. On the other hand, I have no idea why we have that 
>> convention in the first place, so I'm not necessarily opposed to 
>> that. I thought I'd at least check if this was intended or accidental.
>
> Accidental. I thought the convention was (loosely) ClassName_lock_name 
> but it's more ThingBeingLocked_lock, so I have
>
> Decoder_shared_decoder_lock
>
> rather than (I suppose?)
>
> SharedDecoder_lock
>
> I can change these:
>
> NMethodSweeper_stat_lock -> NMethodSweeperStats_lock;
> ThreadsSMRSupport_delete_lock -> ThreadsSMRDelete_lock
> Decoder_shared_decoder_lock -> SharedDecoder_lock
>
> Okay?

Those names look good to me. I don't need to see another webrev.

Thanks,
/Erik

> Thanks,
> David
>
>> Thanks,
>> /Erik
>>
>> On 2018-11-05 02:43, David Holmes wrote:
>>> This impacts GC, compiler, runtime and serviceability.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8213137
>>> webrev: http://cr.openjdk.java.net/~dholmes/8213137/webrev/
>>>
>>> To facilitate changes to the Mutex/Monitor code we need to ensure 
>>> there are no statically initialized Monitor/Mutex instances in the 
>>> VM - as the constructors may rely on VM initialization that has not 
>>> happened when C++ initializers execute.
>>>
>>> There were 6 locally defined "lock" members of classes that were 
>>> statically initialized, and these are across all areas of the VM:
>>>
>>>   - Decoder::_shared_decoder_lock
>>>   - DCmdFactory::_dcmdFactory_lock
>>>   - JfrThreadSampler::_transition_block_lock
>>>   - NMethodSweeper::_stat_lock
>>>   - ThreadsSMRSupport::_delete_lock
>>>   - CodeCacheUnloadingTask::_lock
>>>
>>> The last is actually now unused and so is just deleted. The rest 
>>> join all other Monitor/Mutex instances defined in the global list in 
>>> MutexLocker and initialized in mutex_init(). I've verified that none 
>>> of the monitor/mutex instances can be used prior to mutex_init, by 
>>> code inspection - more details in the bug report.
>>>
>>> I investigated asserting that a Monitor/Mutex is only constructed 
>>> during mutex_init() but assertions don't fail cleanly during C++ 
>>> static initialization - the result is that during the build jmod 
>>> goes into an infinite loop printing out "[Too many errors, abort]".
>>>
>>> Tested using tier1-3
>>>
>>> Thanks,
>>> David