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

Tue May 15 23:50:15 UTC 2018

On 5/15/18 7:42 PM, Ioi Lam wrote:
>
>
> On 5/15/18 4:34 PM, coleen.phillimore at oracle.com wrote:
>>
>>
>> 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.
>>
>
> There is a catch-all version, just like the JavaCalls::call_static() 
> variants:
>
>   // Allocate instance + invoke constructor. This is equivalent to 
> "new Klass(args ...)" expression in Java code.
>   static Handle new_instance(InstanceKlass* klass, Symbol* 
> constructor_signature, JavaCallArguments* args, TRAPS);
>
>   static Handle new_instance(InstanceKlass* klass, Symbol* 
> constructor_signature, TRAPS);
>   static Handle new_instance(InstanceKlass* klass, Symbol* 
> constructor_signature, Handle arg1, TRAPS);
>   static Handle new_instance(InstanceKlass* klass, Symbol* 
> constructor_signature, Handle arg1, Handle arg2, TRAPS);
>
>
>>>
>>> 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.
>>
> How about JavaCalls::construct_instance(...)?

JavaCalls::construct_new_instance.  Sold.

>
>> 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.
>>
> It's done here:
>
> Handle JavaCalls::new_instance(InstanceKlass* klass, Symbol* 
> constructor_signature, JavaCallArguments* args, TRAPS) {
>   klass->initialize(CHECK_NH); // Quick no-op if already initialized.
>   Handle obj = klass->allocate_instance_handle(CHECK_NH);
>   JavaValue void_result(T_VOID);
>   args->set_receiver(obj); // inserts <obj> as the first argument. <<<<<
>   JavaCalls::call_special(&void_result, klass,
>                           vmSymbols::object_initializer_name(),
>                           constructor_signature, args, CHECK_NH);
>   return obj;
> }
>
> set_receiver does what you mentioned, It's ugly but I didn't write it :-)

I see that now.  I was distracted by the other versions that I didn't like.

Coleen

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