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

[Jiangli Zhou]

> On May 15, 2018, at 9:13 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> 
> 
> On 5/15/18 1:39 AM, David Holmes wrote:
>> Correcting silly comment ...
>> 
>> On 15/05/2018 6:31 PM, David Holmes wrote:
>>> 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 ??
>> 
>> Of course you are creating instances of these classes. But you're using JavaCalls::call_special to invoke the constructor, so isn't that already ensuring the class is initialized as you just described ??
>> 
> 
> I should have clarified what I said above about the invoke bytecodes: if I remember correctly, the only 4 bytecodes that implicitly initialize the operand's class are - new, invokestatic, getstatic and putstatic. invokespecial and invokevirtual do not perform initialization, because they operate on an object, which has already been new'ed, which means its class has already been initialized by the 'new' byte code.

Right, class initialization is triggered ‘implicitly’ with invokestatic, putstatic, getstatic, new, etc. Before doing invokespecial, an explicit initialize() call is made for a class. Looks like most existing JavaCalls::call_special() call sites don’t do the class initialize(). Those classes’ initialize() are done earlier however, either during VM initialization or the initialization of a component. So the original code did follow the convention. Calling initialize() in get_shared_jar_manifest() and get_protection_domain_from_classloader() would be a little less efficient. Maybe add a new API in SystemDictionaryShared for initializing the classes used by AppCDS and add it after the SystemDictionary::compute_java_loaders call (so the original initialization sequence is not changed)?

Thanks,

Jiangli

> 
> 
>> 
>>>>> 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.
> 
> For, the 3 places where the klass->initialize() is added, they all immediately execute Java code, which could have any side effect, including loading/initializing more classes. So I think it's safe to add the initialization call there.
> 
> Thanks
> - Ioi
> 
>>> 
>>> 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
>>>> 
>