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

[David Holmes]

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

David
-----


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