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

[Lois Foltan]

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

On 4/16/2019 6:06 PM, Karen Kinnear wrote:
> Lois,
>
> Looks really good.
>
> A couple minor questions/comments - mostly about comments
> I did not find any bugs - so I don’t need to see an updated webrev.
> Pick-and-choose any suggestions you like/don’t like.
>
> 1. resolve_metadata
> Any chance this could be renamed to something more specific? e.g. 
> resolve_bss_name_and_type?
Done.

>
> 2. bootstrapInfo:
> Why is there a field _is_method_call ? Couldn’t this be 
> is_method_call() which gets the same
> info from indy_index <0 vs. >= 0? Or is there a timing reason you need 
> both?
Removed field BootstrapInfo::_is_method_call.  Method 
BootstrapInfo::is_method_call() just checks the _indy_index.  There was 
no timing issue that warranted this field.

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

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

>
> 5. constantPool.cpp
> old line 927
> // FIXME: Cache this once per BootstrapMethods entry, not once per 
> CONSTANT_Dynamic
> -- did I read this correctly? That the resulting bootstrap_specifier 
> is still caching the result per bss_index, where bss is either 
> CONSTANT_Dynamic or CONSTANT_InvokeDynamic
Correct.  The "FIXME" comment was removed because I don't think we ever 
intend to cache this once per BootstrapMethods entry.

>
> 6. Trivial
> How do you feel about asserts with no text message? See 
> bootstrapInfo.cpp lines 55/56
> I didn't check for others
Fixed, checked others too.

>
> 7. where is has_bootstrap defined?
utilities/constantTag.hpp

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

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

>
> 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.
>
> 11. linkResolver.cpp line 1727:
> "relevent" -> "relevant"
Fixed.

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

>
> 13. name resolve_newly_linked_invokedynamic is confusing. Is it worth
> having this as a separate method? This is just a set_handle call, with
> a small assertion. If you want to keep it, could you possibly rename it?
> the bootstrap_specifier is already resolved, and the assertion belongs
> to the success path after invoke_bootstrap_method I presume.
> If others feel strongly, ignore my comment.
I'm going to leave it for now.  This will be revisited on the next 
iteration of changes for BootstrapInfo.

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