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

[David Holmes]

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

> 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?

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