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

[Karen Kinnear]

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? 

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?

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?

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?

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

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

7. where is has_bootstrap defined?

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?

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?

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"

12. linkResolver.cpp
line 1740 - what does DTRT mean? - is that "Does The Right Thing"?
could you be more specific?

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.

thanks,
Karen

> On Apr 15, 2019, at 3:09 PM, Lois Foltan <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
>> 
>