RFR(M) JDK-8218994: Consolidate indy and condy JVM information within a BootstrapInfo data structure

[Lois Foltan]

On 4/22/2019 11:05 AM, Karen Kinnear wrote:
> Looks good to go!
Great!  Thank you again for the review.
Lois

>
> thanks,
> Karen
>
>> On Apr 19, 2019, at 2:51 PM, Lois Foltan <lois.foltan at oracle.com 
>> <mailto:lois.foltan at oracle.com>> wrote:
>>
>> Hi Karen,
>>
>> Thank you so much for the thorough review!  I have addressed all your 
>> comments, see responses interspersed below.
>>
>> New updated webrev: 
>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.3/webrev/
>>
>> Thanks,
>> Lois
>>
>>>
>>> 3. resolve_bsm.
>>> Nice to have a comment for the first line - since bootstrapInfo is a 
>>> StackObj, this should only
>>> happen if the same thread has more than 1 indy/condy with the same 
>>> bsm, right?
>> Not quite.  A BootstrapInfo data structure is constructed for every 
>> indy and every condy.  It is never the case that they are shared 
>> between multiple indys who use the same bsm or for that case multiple 
>> condys that use the same bsm.  The first line just simply protects 
>> against resolve_bsm being called more than once for the same 
>> BootstrapInfo.  That should never occur, I guess the first line could 
>> be an assert but I am going to leave as is for now.
> You are right - I had confused myself.
>>
>>>
>>> 4. bootstrapInfo
>>> Handle _arg_values // array of static args; null for unresolved
>>>
>>> Could you please clarify what null means?
>>> Do you mean _arg_values is a handle to null if there are no static args?
>>> Or do you mean that elements in the array are null if unresolved? 
>>> The former?
>> Yeah, that's confusing.  If BootstrapInfo::_arg_values is null that 
>> implies either the static args have not been resolved or there are no 
>> static args, _agrc == 0. I have updated the comment.
> Thank you for clarifying that combining this with args tells the 
> difference.
>>
>>
>>>
>>> 8. constantPool.cpp
>>> Did I read this correctly that JVMS 5.4.3.6 Steps 1 and 2 are both 
>>> performed
>>> under invoke_bootstrap_method?   Could you possibly clarify that in 
>>> the comments 913-923?
>> Yes, steps 1 & 2 are done under invoke_bootstrap_method.  I have 
>> improved the comment to state that both the first and second tasks of 
>> JVMS section 5.4.3.6 occur within invoke_bootstrap_method().
> Nice!
>>
>>>
>>> 9. bootstrapInfo.cpp
>>> resolve_args:
>>> if (!use_BSCI)
>>> I am confused about _arg_values setting:
>>>   1) looks like you set _arg_values = Handle(THREAD< arg_oop)
>>>   and then overwrite with _arg_values = Handle(THREAD, args()) in 
>>> some cases
>>>   2) if args is already objArrayHandle, why do you handleize again?
>> Great catch.  That's not right, the code has been changed:
>>
>>  if (!use_BSCI) {
>>     // return {arg...}; resolution of arguments is done immediately, 
>> before JDK code is called
>>     objArrayOop args_oop = 
>> oopFactory::new_objArray(SystemDictionary::Object_klass(), _argc, CHECK);
>>     objArrayHandle args(THREAD, args_oop);
>>     _pool->copy_bootstrap_arguments_at(_bss_index, 0, _argc, args, 0, 
>> true, Handle(), CHECK);
>>     oop arg_oop = ((_argc == 1) ? args->obj_at(0) : (oop)NULL);
>>     // try to discard the singleton array
>>     if (arg_oop != NULL && !arg_oop->is_array()) {
>>       // JVM treats arrays and nulls specially in this position,
>>       // but other things are just single arguments
>>       _arg_values = Handle(THREAD, arg_oop);
>>     } else {
>>       _arg_values = args;
>>     }
>>   } else {
> Looks good.
>>
>>>
>>> 10. bootstrapInfo.cpp
>>> resolve_previously_linked_invokedynamic
>>> I know the include states what true/false mean. It would be helpful 
>>> to duplicate the comment
>>> from the include at the start of the method.
> Thank you.
>>>
>>> 12. linkResolver.cpp
>>> line 1740 - what does DTRT mean? - is that "Does The Right Thing"?
>>> could you be more specific?
>> Removed DTRT, improved the comment.
> Thank you - that is very helpful.
>>>
>>
>>>
>>> thanks,
>>> Karen
>>>
>>>> On Apr 15, 2019, at 3:09 PM, Lois Foltan <lois.foltan at oracle.com 
>>>> <mailto:lois.foltan at oracle.com>> wrote:
>>>>
>>>> Updated webrev to fix typos caught by John's review of the .1 webrev
>>>>
>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.2/webrev/
>>>>
>>>> Thanks,
>>>> Lois
>>>>
>>>> On 4/12/2019 8:17 AM, Lois Foltan wrote:
>>>>> I've updated the webrev based on preliminary feedback from Coleen.
>>>>>
>>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.1/webrev/
>>>>>
>>>>> Reviews appreciated!
>>>>> Lois
>>>>>
>>>>> On 3/14/2019 7:39 AM, Lois Foltan wrote:
>>>>>> Please review this change to consolidate indy and condy JVM 
>>>>>> information to invoke a bootstrap method into a new BootstrapInfo 
>>>>>> data structure as well as merge the condy and indy methods to 
>>>>>> invoke the bootstrap within SystemDictionary.
>>>>>>
>>>>>> open webrev at: 
>>>>>> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.0/webrev/
>>>>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8218994
>>>>>> co-contributors: Lois Foltan & John Rose
>>>>>>
>>>>>> Testing: hs-tier1-3 & jdk-tier1-3 (all platforms), hs-tier4-8 
>>>>>> (linux only), JCK vm & lang
>>>>>>
>>>>>> Thanks,
>>>>>> Lois
>>>>>
>>>>
>>>
>>
>