RFR(M) JDK-8218994: Consolidate indy and condy JVM information within a BootstrapInfo data structure
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Mon Apr 22 18:30:06 UTC 2019
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 ?
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;
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 ?? :)
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