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

[Ioi Lam]

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