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

[Jiangli Zhou]

The cleanup looks really good!

Thanks,
Jiangli

> On May 15, 2018, at 9:06 PM, Ioi Lam <ioi.lam at oracle.com> 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
>>>> 
>>> 
>