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

Ioi Lam ioi.lam at oracle.com
Tue May 15 23:42:43 UTC 2018 


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(...)?

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

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

More information about the hotspot-runtime-dev mailing list