RFR (S) 8213137: Remove static initialization of monitor/mutex instances
erik.osterlund at oracle.com
Wed Nov 7 09:59:07 UTC 2018
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
> rather than (I suppose?)
> I can change these:
> NMethodSweeper_stat_lock -> NMethodSweeperStats_lock;
> ThreadsSMRSupport_delete_lock -> ThreadsSMRDelete_lock
> Decoder_shared_decoder_lock -> SharedDecoder_lock
Those names look good to me. I don't need to see another webrev.
>> 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
More information about the hotspot-dev