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

[Ioi Lam]

On 5/15/18 3:22 PM, coleen.phillimore at oracle.com wrote:
>
>
>
> On 5/15/18 5:47 PM, 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.
>>
>
> I think I like your last version better.  I don't think 
> JavaCalls::new_instance() can nicely generalize the calls to object 
> init functions that I found around the vm.  I think just doing what 
> you had would be better.  It's only 3 klass->initialize calls. You 
> could make the new_instance() function local to systemDictionaryShared 
> to save code if they are all the same, but I don't see much advantage.
>
> Also a general new_instance() function should have the call to this to 
> be the same as the others (see reflection.cpp).
>
>   klass->check_valid_for_instantiation(false, CHECK_NULL);
>
javaCalls is for cases where "the VM knows what it's doing", and 
reflection.cpp is to implement java.lang.reflect. So the former doesn't 
do access checks while the latter does. You can compare 
JavaCalls::call_static() vs Reflection::invoke_method().

> If it were possible to replace all klass->allocate_instance() calls 
> with JavaCalls::new_instance(), that seems like the API would be worth 
> it, but it's not possible.   So people can still make mistakes and not 
> call the object init function, like I think someone did in jvm.cpp.
>
Why is it not possible? I think many places can be cleaned up with this 
API, e.g., in ./share/services/{management.cpp, memoryService.cpp, 
attachListener.cpp and gcNotifier.cpp). I can do the cleanup in a 
separate RFE.

Some places call klass->allocate_instance() without calling the 
constructor, on purpose, to simulate the behavior of the 'new' bytecode. 
E.g., in jvmciRuntime.cpp.

But if what you want to do is "x = new Foo(args ...)" in the C code, 
JavaCalls::new_instance() seems like a clean way to do it.

Thanks
- Ioi
>
>> 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.
>>
>> 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.
>
> Not sure what that would mean myself :)
>>
>> 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.
>>
>>
>> (BTW, I changed the RFR subject line from XS to S due to the extend 
>> of change :-)
>>
> Make it S again!
>
> Coleen
>
>> 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
>>>
>>
>