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

[Ioi Lam]

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' bytecode.


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