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

[Vladimir Kozlov]

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. It 
is fine for now but I would add assert for 'else' case that only HotSpotResolvedObjectType ref is expected there.

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

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.

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

  Why removing !result->is_anonymous() check is not enough?:

  if (InstanceKlass::should_store_fingerprint()) {
    result->store_fingerprint(stream->compute_fingerprint());

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