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

[David Holmes]

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