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

[Calvin Cheung]

Looks good.

thanks,
Calvin

On 5/15/18, 9:06 PM, Ioi Lam wrote:
> I've updated the webrev:
>
> http://cr.openjdk.java.net/~iklam/jdk11/8189140-rename-system-dict-shared-initialize.v03/ 
>
>
> 1. Moved SharedDictionary_lock to mutexLocker.cpp.
>
> 2. There's no more need for SystemDictionaryShared::initialize. Removed.
>
> 3. Renamed to JavaCalls::construct_new_instance per Coleen.
>
> If this goes in, I'll file a separate RFE to convert some other code 
> to use JavaCalls::construct_new_instance.
>
> Thanks
>
> - Ioi
>
>
>
> On 5/15/18 6:53 PM, David Holmes wrote:
>> On 16/05/2018 7:47 AM, Ioi Lam wrote:
>>> I've updated the webrev:
>>>
>>> http://cr.openjdk.java.net/~iklam/jdk11/8189140-rename-system-dict-shared-initialize.v02/ 
>>>
>>>
>>> 1. Added JavaCalls::new_instance so we can avoid all the boiler 
>>> plate code for allocating
>>>     the instance andinvoking the constructor.
>>>
>>> JavaCalls::new_instance calls InstanceKlass->initialize. This is 
>>> just a quick op after
>>>     the class is already initialized. Also, JavaCalls::call_static 
>>> also internally call
>>>     into InstanceKlass->initialize, so I am just following the 
>>> existing pattern as Coleen
>>>     mentioned below.
>>>
>>>     Doing the initialization on demand also means for cases where 
>>> JAR manifest is not used
>>>     (all code is loaded from the system image or directories), we 
>>> get faster start-up.
>>
>> That all looks good - nice cleanup. And we should look at using the 
>> new_instance in more places if appropriate.
>>
>>> 2. I also took the time to removed a bunch of "// One oop argument" 
>>> comments which
>>>     probably meant something to the person who wrote it, but seems 
>>> useless to everyone else.
>>
>> Especially when the coding pattern is odd anyway. I mean why have:
>>
>>     JavaCallArguments args(receiver);
>>     args.push_oop(arg1);
>>     args.push_oop(arg2);
>>
>> instead of
>>
>>     JavaCallArguments args;
>>     args.push_oop(receiver);
>>     args.push_oop(arg1);
>>     args.push_oop(arg2);
>>
>> or
>>
>>     JavaCallArguments args(receiver, oop1, oop2);
>>
>> ?? (Not something I expect you to change of course!)
>>
>>>
>>> 3. As Calvin suggested, I removed the File_klass and also 
>>> ParseUtil_klass from
>>>     the system dictionary since they are not used anywhere. This 
>>> hopefully improves start-up
>>>     by a little bit, since these 2 classes are no longer resolved at 
>>> start-up.
>>
>> Ok.
>>
>>>
>>> (BTW, I changed the RFR subject line from XS to S due to the extend 
>>> of change :-)
>>
>> Please don't do that as it breaks the flow when sorting/threading by 
>> subject!
>>
>> Modulo Colleen's query on the lock initialization placement this all 
>> seems fine to me.
>>
>> Thanks,
>> David
>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>>
>>>
>>> On 5/15/18 2:00 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk11/8189140-rename-system-dict-shared-initialize.v01/src/hotspot/share/classfile/systemDictionaryShared.cpp.udiff.html 
>>>>
>>>>
>>>> This looks good.  This is a pattern that's used in other places, 
>>>> and it would be better to not initialize these at startup in 
>>>> thread.cpp.
>>>>
>>>> Coleen
>>>>
>>>> On 5/15/18 2:07 AM, 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.
>>>>>
>>>>> 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
>>>>
>>>
>