RFR 8203381 Replace InstanceKlass::allocate_instance_handle with JavaCalls::construct_new_instance

[Ioi Lam]

On 5/21/18 9:54 PM, David Holmes wrote:
> On 22/05/2018 2:24 PM, Ioi Lam wrote:
>> On 5/20/18 7:05 PM, David Holmes wrote:
>>> Hi Ioi,
>>>
>>> On 19/05/2018 2:34 AM, Ioi Lam wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8203381
>>>> http://cr.openjdk.java.net/~iklam/jdk11/8203381-construct-new-instance.v01/ 
>>>>
>>>>
>>>> This patch removes a lot of the boilerplate code for allocating a
>>>> Java object and calling its constructor. Hopefully the code is cleaner
>>>> and easier to read.
>>>
>>> That's a great cleanup! Why didn't we do this years ago! :)
>>>
>>> Only nit is that now call_special is replaced with 
>>> construct_new_instance the argument line indentation is no longer 
>>> correct.
>>>
>> Hi David,
>>
>> I noticed that there isn't a uniform style of indentation with 
>> passing function parameters. Usually people indent to the open 
>> parenthesis, but I am hesitant to do that when the open parenthesis 
>> is more than 40 chars from the beginning of a line. Otherwise I'll 
>> end up with lots of skinny lines. They would also run past 80-char 
>> width when you have "wide" parameters.
>
> Yes it is certainly not a uniform rule and often conflicts with other 
> line length guidelines. But in this case the change from aligned to 
> not-aligned was quite noticeable.
>
>>    return 
>> JavaCalls::construct_new_instance(SystemDictionary::Module_klass(),
>> vmSymbols::java_lang_module_init_signature(),
>>                            loader, module_name, CHECK_NH);
>>
>> So if you don't have a big objection, I'll just keep the indentation 
>> as is.
>
> It was only a nit so that is fine.
>
Thanks!
> Though I was almost tempted to suggest that you rename 
> construct_new_instance to do_construct, as it is the same length as 
> call_special. ;-)
>

That wouldn't help too much, because call_special is a void function so 
it gets to start at the front of the line, but construct_new_instance 
would have an assignment or a "return" in front of it :-(

Thanks
- Ioi
> Cheers,
> David
>
>
>> Thanks
>> - Ioi
>>> Great comments on the Thread construction process too! Hope it didn't 
>
>>> take you too long to figure out why you couldn't make the 
>>> replacement in those cases!
>>>
>>> Thanks,
>>> David
>>>
>>>> Also:
>>>>
>>>> Added assert(InstanceKlass::is_initialized(), ...) in case where
>>>> we cannot call JavaCalls::construct_new_instance for thread objects.
>>>>
>>>> Replaced unnecessary SystemDictionary::resolve_or_null() calls with
>>>> SystemDictionary::XXX_klass().
>>>>
>>>> Tested with hs-tier[1,2,3,4,5]
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>
>>>>
>>>>
>>>>
>>