RFR(XS) 8189140 - SystemDictionaryShared::initialize() should be renamed to be more meaningful

[David Holmes]

On 15/05/2018 5:44 PM, Ioi Lam wrote:
> On 5/14/18 11:25 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 15/05/2018 4:07 PM, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8189140
>>> http://cr.openjdk.java.net/~iklam/jdk11/8189140-rename-system-dict-shared-initialize.v01/ 
>>>
>>>
>>> Summary:
>>>
>>> 1. Removed the forced initialization of a few classes used by AppCDS 
>>> at JVM start-up.
>>>     Instead, initialize these class on demand by calling 
>>> InstanceKlass::initialize, which
>>>     is a quick no-op if the class is already initialized.
>>
>> Any reason not to just add them to the set of pre-loaded and 
>> pre-initialized classes used with shared spaces in 
>> initialize_preloaded_classes()?
>>
> Because initialize_preloaded_classes(), despite its name, doesn't 
> actually call InstanceKlass::initialize(). It just resolves the klasses. 
> If you try to call InstanceKlass::initialize() here, the VM would crash.

So much from truth in advertising ;-)

> A few classes are initialized inside functions like 
> Threads::initialize_java_lang_classes(), which seems a bit like black 
> magic to me, so I didn't want to muck with them.

Yes - the initialization order is not something to be trifled with.

>> Aside: It looks a little odd to lazy-init a couple of classes but then 
>> assume/expect others (like SecureClassLoader) are already initialized.
>>
> The explicit init is only necessary before calling 
> InstanceKlass::allocate_instance_handle(), which, unlike the 'new' 
> bytecodes, does not implicitly initialize the class.

What is allocate_instance_handle() supposed to do? Is it just a raw 
allocator? The class must be initialized before any constructor is called.

> The other code in SystemDictionaryShared.cpp access the classes through 
> JavaCalls::call_{static,special,virtual}, where the class would be 
> initialized implicitly as necessary in exactly the same way as the 
> invoke{static,special,virtual} bytecodes.
> 
> Perhaps we should add a JavaCalls::new_instance() method that implicitly 
> initializes the class, just like the 'new' bytecode? Should I do that in 
> this changeset?

I'm not sure where you would do this or why you need to create an 
instance of these classes ??

>> But this seems functionally fine and a good tidy up (the class init 
>> stuff really didn't belong as a side-effect). The only thing I'd be 
>> double checking is that the same thread (presumably the main thread) 
>> is guaranteed to be the one doing the initialization.
>>
> There's no such requirement on the threads. If another thread is already 
> in the middle of initializing the class, the thread that class 
> InstanceKlass::initialize() will just wait for the class initialization 
> to complete.

I was thinking purely from the perspective of not changing anything in 
the initialization order - and making sure you can't inadvertently do 
something on a thread that isn't allowed to load/initialize klasses. 
This is intended to be a small code cleanup so I presumed you didn't 
want to perturb things too much - ideally not at all.

David

> Thanks
> - Ioi
> 
>> Thanks,
>> David
>>
>>> 2. The only initialization left is that of a global lock. So I 
>>> renamed the function
>>>     to SystemDictionaryShared::initialize_locks().
>>>
>>> 3. I moved the call of this function from 
>>> SystemDictionary::compute_java_loaders() to
>>> SystemDictionary::initialize() where it seems to fit.
>>>
>>> Testing with hs-tiers 1 and 2.
>>>
>>> Thanks
>>> - Ioi
>