RFR(M) JDK-8218994: Consolidate indy and condy JVM information within a BootstrapInfo data structure
Lois Foltan
lois.foltan at oracle.com
Mon Apr 22 19:26:36 UTC 2019
Thanks Coleen! All items have been addressed. If you would like
another webrev iteration let me know.
Lois
On 4/22/2019 2:30 PM, coleen.phillimore at oracle.com wrote:
>
> So, this is less of a thorough review:
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.3/webrev/src/hotspot/share/interpreter/linkResolver.hpp.frames.htmlhttp://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.3/webrev/src/hotspot/share/classfile/systemDictionary.hpp.frames.html
>
>
> Can these just have forward declaration of BootstrapInfo here rather
> than including bootstrapinfo.hpp ?
Yes, fixed.
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.3/webrev/src/hotspot/share/interpreter/linkResolver.cpp.frames.html
>
>
> Really tiny nit, not really worth mentioning but can you remove the
> space in:
>
> 1708 if (is_done) return;
>
> 1763 if (is_done) return;
Fixed.
>
>
> http://cr.openjdk.java.net/~lfoltan/bug_jdk8218994.3/webrev/src/hotspot/share/oops/constantPool.cpp.frames.html
>
>
> 956 if (TraceMethodHandles) {
> 957 bootstrap_specifier.print_msg_on(tty, "resolve_constant_at_impl");
> 958 }
>
> Please can you convert these to UL next ?? :)
Yes, there is an outstanding RFE for this, see
https://bugs.openjdk.java.net/browse/JDK-8210012
>
> This change looks good to me.
>
> Coleen
>
> On 4/19/19 2:51 PM, Lois Foltan 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
>>
>> 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
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list