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

[coleen.phillimore at oracle.com]

On 5/15/18 7:02 PM, Ioi Lam wrote:
>
>
> 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.

I don't see how these would be any cleaner.  One example you sited calls 
a static function initialize_ThreadInfo_constructor_arguments which 
pushes 9 arguments to the constructor.   You would have a version of 
JavaCalls::new_instance with 1,2,3,...,9 potential arguments?   It 
wouldn't be less code or nicer.

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

Yes, so you cannot replace all calls to allocate_instance().

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

This name seems too ambiguous with allocate_instance() which I thought 
called the constructor until I just looked.  If new_instance had a 
sentence name, maybe it would indicate when one should use it vs. 
allocate_instance.   This doesn't seem like it helps.

If you want to add a local one to systemDictionaryShared for the three 
calls, that would be fine with me.  Then you can see whether you can 
cleanly replace all of the places with a function that encapsulates 
calling the klass->initialize function, allocating an object, and 
calling the constructor with a variable length argument list.   My 
initial thought was to have the encapsulating function pass JavaCallArgs 
with a NULL for the object argument which is replaced inside the call 
after it's allocated.  Still sort of ugly but better than 10 cascading 
functions.

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