[10] RFR(L): 8132547: [AOT] support invokedynamic instructions

[dean.long at oracle.com]

New hotspot webrev is here:

http://cr.openjdk.java.net/~dlong/8132547/hs.2/

Comments inlined below...


On 10/16/17 11:06 AM, Vladimir Kozlov wrote:

> I looked more on changes.
>
> First, please, run RBT testing. May be ask SQE to run testing with 
> AOTed java.base as they did before.
>
> I did not look on Graal assuming Labs reviewed it already.
> JVMCI changes looks fine to me.
>
> JAOTC. In addition to previous comment about change in 
> DataPatchProcessor.java.
> ------
>
> AOTCompiledClass.java - I wish metadataName() was defined in 
> corresponding classes instead of manual checking type. 

To do that I would probably need to wrap the JVMCI types in new JAOTC 
types.  I don't want to pollute the JVMCI types.

> It is fine for now but I would add assert for 'else' case that only 
> HotSpotResolvedObjectType ref is expected there.
>

Sure, thanks for catching that.  The cast will fail with an exception 
without the assert, but the assert can give a more informative error 
message.

> Should we also unify how we generate method name?
> We use JavaMethodInfo.uniqueMethodName() in few places.
> Then we have AOTHotSpotResolvedJavaMethod.getNameAndSignature().
> And now you added new metadataName().
>

OK, I filed RFE 8189411.

> Hotspot AOT code.
> -----------------
>
> aotLoader.hpp - you don't need 2 methods. Move UseAOT check into .cpp 
> code and in .hpp you can do:
>
> static bool reconcile_dynamic_invoke(InstanceKlass* holder, int index, 
> Method* adapter_method, Klass *appendix_klass) NOT_AOT({ return true; });
>
> aotCodeHeap.* - I don't like that you have separate 
> reconcile_dynamic_klass() method only for one use. Instead of passin 
> [2] array pas it as separate parameter so you can pass NULL when it is 
> not defined.
>

OK.

> Hotspot fingerprint.
> -------------------
>
> I am concern that you changed logic when and how klass's fingerprint 
> is generated. With your changes it become more expensive:
>
>    if (UseAOT && ik->supers_have_passed_fingerprint_checks()) {
> +    uint64_t str_fp = _stream->compute_fingerprint();
>

You are right, I will revert these changes that were left over from an 
earlier version.

>  Why removing !result->is_anonymous() check is not enough?:
>
>  if (InstanceKlass::should_store_fingerprint()) {
>    result->store_fingerprint(stream->compute_fingerprint());
>

Because InstanceKlass::should_store_fingerprint() will return false for 
an anonymous class.

dl


> Thanks,
> Vladimir
>
> On 10/6/17 12:52 PM, dean.long at oracle.com wrote:
>> On 10/6/17 12:37 PM, dean.long at oracle.com wrote:
>>
>>> On 10/6/17 10:03 AM, Igor Veresov wrote:
>>>
>>>>
>>>>
>>>>> On Oct 6, 2017, at 9:52 AM, Vladimir Kozlov 
>>>>> <vladimir.kozlov at oracle.com <mailto:vladimir.kozlov at oracle.com>> 
>>>>> wrote:
>>>>>
>>>>> On 10/5/17 11:16 PM, Igor Veresov wrote:
>>>>>>> On Oct 5, 2017, at 10:57 AM, dean.long at oracle.com 
>>>>>>> <mailto:dean.long at oracle.com> wrote:
>>>>>>>
>>>>>>> On 10/4/17 6:27 PM, Vladimir Kozlov wrote:
>>>>>>>
>>>>>>>> Yes, I start looking on it.
>>>>>>>>
>>>>>>>> In DataPatchProcessor.java why you removed 
>>>>>>>> addDependentKlassData() call?:
>>>>>>>>
>>>>>>>> + AOTCompiledClass.addFingerprintKlassData(binaryContainer, type);
>>>>>>>> +                targetSymbol = 
>>>>>>>> AOTCompiledClass.metadataName(type);
>>>>>>>>                  gotName = ((action == 
>>>>>>>> HotSpotConstantLoadAction.INITIALIZE) ? "got.init." : "got.") + 
>>>>>>>> targetSymbol;
>>>>>>>> - 
>>>>>>>>                methodInfo.addDependentKlassData(binaryContainer, 
>>>>>>>> type);
>>>>>>>>              } else if 
>>>>>>>> (metaspaceConstant.asResolvedJavaMethod() != null && action == 
>>>>>>>> HotSpotConstantLoadAction.LOAD_COUNTERS) {
>>>>>>>>
>>>>>>>
>>>>>>> It is supposed to be an optimization, to prevent adding 
>>>>>>> dependencies when we don't need them.  We add dependencies 
>>>>>>> elsewhere if we inline a method or reference a field, etc.  I 
>>>>>>> don't think we need a dependency just because we reference a 
>>>>>>> constant.
>>>>>>> Igor, do you agree?
>>>>>>>
>>>>>> I suppose you’re right. Field offset seems to be the only place 
>>>>>> where a dependency would be required and we should get it 
>>>>>> covered. Perhaps this was added before we had field access 
>>>>>> recording. But I’d test it in case something pops up (although 
>>>>>> nothing come to mind right now).
>>>>>
>>>>> What about allocations and runtime guard checks (class checks)?
>>>>
>>>>
>>>> Yes, good point. Allocation will have the size of the object as a 
>>>> constant, which is definitely something we need a dependency for. 
>>>> So either we need to collect all types allocated in the parser, or 
>>>> leave that statement as it were. Perhaps we need a followup RFE to 
>>>> clean this up.
>>>>
>>>
>>> OK let me see if I can simply revert that change.  There may be an 
>>> ordering problem that I was trying to fix at the same time.
>>>
>>
>> I forgot to mention, if it's in the metadata, then there should be a 
>> dependency, and we have an assert that checks for that.  Do all 
>> allocations and class checks generate a metadata entry?  But I agree 
>> that a followup RFE is safer.
>>
>> dl
>>
>>
>>> dl
>>>
>>>> igor
>>>>
>>>>>
>>>>> Vladimir
>>>>>
>>>>>> igor
>>>>>>>> Is HotSpotConstantPoolObject is real oop (Java object)? oop_got 
>>>>>>>> array is scanned for oops only.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, it's the appendix object.
>>>>>>>
>>>>>>>> Can you explain why the same class can have several Metaspace 
>>>>>>>> Names? Are all of them correspond to one class (and its 
>>>>>>>> methods)? Should we do more in load_klass_data() in such case.
>>>>>>>>
>>>>>>>
>>>>>>> It is a many to one mapping (aliases) for anonymous classes 
>>>>>>> because we can't rely on the temporary name that the JVM 
>>>>>>> creates. Regular classes use load_klass_data but anonymous 
>>>>>>> classes don't.  I have a TODO in 
>>>>>>> AOTCodeHeap::reconcile_dynamic_klass() for loading code for 
>>>>>>> anonymous classes, but it is disabled because I haven't 
>>>>>>> implemented AOT of anonymous classes yet:
>>>>>>>
>>>>>>>   // TODO: hook up any AOT code
>>>>>>>   // load_klass_data(dyno_data, thread);
>>>>>>>
>>>>>>>> Please, check consumption of Java heap since you are passing 
>>>>>>>> oops for metadata generation instead of strings through JVMCI.
>>>>>>>>
>>>>>>>
>>>>>>> OK.
>>>>>>>
>>>>>>> dl
>>>>>>>
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 10/4/17 2:50 PM, dean.long at oracle.com 
>>>>>>>> <mailto:dean.long at oracle.com> wrote:
>>>>>>>>> Hi Vladimir, do you have time to review this?
>>>>>>>>>
>>>>>>>>> thanks,
>>>>>>>>>
>>>>>>>>> dl
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/11/17 7:21 PM, Dean Long wrote:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8132547
>>>>>>>>>>
>>>>>>>>>> http://cr.openjdk.java.net/~dlong/8132547/
>>>>>>>>>>
>>>>>>>>>> This enhancement is a first step in supporting invokedynamic 
>>>>>>>>>> instructions in AOT.  Previously, when we saw an 
>>>>>>>>>> invokedynamic instruction, or any anonymous class, we would 
>>>>>>>>>> generate code to bail out and deoptimize.  With this 
>>>>>>>>>> changeset we go a little further and call into the runtime to 
>>>>>>>>>> resolve the dynamic constant pool entry, running the 
>>>>>>>>>> bootstrap method, and returning the adapter method and 
>>>>>>>>>> appendix object.  Like class initialization in AOT, we only 
>>>>>>>>>> do this the first time through. Because AOT double-checks 
>>>>>>>>>> classes using fingerprints and symbolic names, special care 
>>>>>>>>>> was required to handle anonymous class names.  The solution I 
>>>>>>>>>> chose was to name anonymous types with aliases based on their 
>>>>>>>>>> constant pool location ("adapter<classid:cpi>" and 
>>>>>>>>>> appendix<classid:cpi>").
>>>>>>>>>>
>>>>>>>>>> Future work is needed to AOT-compile the anonymous classes 
>>>>>>>>>> and/or inline through them, so this change is not expected to 
>>>>>>>>>> affect AOT performance.  In my tests I was not able to 
>>>>>>>>>> measure any difference.
>>>>>>>>>>
>>>>>>>>>> Upstream Graal changes have already been pushed.  I broke the 
>>>>>>>>>> JVMCI and hotspot changes into separate webrevs.
>>>>>>>>>>
>>>>>>>>>> dl
>>>>
>>>
>>