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

[Karen Kinnear]

Looks good to go! 

thanks,
Karen

> On Apr 19, 2019, at 2:51 PM, Lois Foltan <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/ <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/ <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/ <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/ <http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.0/webrev/>
>>>>> bug link: https://bugs.openjdk.java.net/browse/JDK-8218994 <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
>>>> 
>>> 
>> 
>