Forcing initialization of string concat INDY expressions

[Remi Forax]

The worst thing here is that conceptually, for an invokedynamic, the implementation do not use the fact that MethodTypes are interned.
Only a direct call to a method handle (with invokeExact/invoke) needs MethodTypes to be interned to speedup the test that verifies that the parameter types are compatible.

One way to solve that is to not use the cache when creating a MethodType but only when invoking a method handle directly. A MethodHandle can have two fields, one storing the non-interned method type (which is final) and one storing the corresponding interned method type (which is @Stable) and computed it if the pointer check of an invokeExact check fail because 

mh.invokeExact(desc) 
  if (mh.internedMethodType == desc.methodType) {
    // fastpath
  }
  if (mh.internedMethodType == null) {
    mh.internedMethodType = intern(mh.type);
    if (mh.internedMethodType == desc.methodType) {
      // fastpath
    }
  }
  ...
   

Rémi

----- Mail original -----
> De: "David Holmes" <david.holmes at oracle.com>
> À: "Aleksey Shipilev" <shade at redhat.com>, "core-libs-dev Libs" <core-libs-dev at openjdk.java.net>
> Envoyé: Mercredi 1 Mars 2017 13:36:49
> Objet: Re: Forcing initialization of string concat INDY expressions

> Hi Aleksey,
> 
> On 1/03/2017 7:46 PM, Aleksey Shipilev wrote:
>> Hi,
>>
>> On 03/01/2017 07:36 AM, David Holmes wrote:
>>> The INDY-fication of string concatenation has triggered a problem where a JVM TI
>>> agent's monitor-wait/ed callback hits an error path that uses string concat
>>> which triggers a mass of indy related initialization, which in turn hits monitor
>>> use in MethodType$ConcurrentWeakInternSet.get, which causes the VM monitor
>>> subsystem to be re-entered (and it is not reentrant!) so we crash. (log extract
>>> below - the amount of code to process this is truly scary!)
>>
>> Ouch. This is an unexpected circularity. It is unusual to see Java thread to do
>> Java stuff when doing Object.wait.
> 
> That's the fatal flaw in JVM TI. It pretends you can invoke arbitrary
> Java code via an agent callback. You can invoke simple Java code - as
> most real callbacks do - but not arbitrary Java code. The VM is not
> reentrant in some subsystems - like monitors.
> 
>>> I assume I can cover the exact case above by replicating it? But can I
>>> generalize it to cover arbitrary string concat expressions that might arise on
>>> other error paths?
>>
>> Yes, the StringConcatFactory code is deliberately lazy. If you want to link
>> eagerly, you would need to match the concat shape exactly (number and type of
>> arguments) -- probably by using the same concat code the test failed on. We can
>> probably eagerly link some popular concat shapes early during system init, but
>> that requires JDK changes.
> 
> So I can cover the current failing case easily enough but can't
> generalize except by covering each case individually.
> 
>> Looking at stack trace, it seems to be generic failure when adding new
>> MethodType to the MethodType cache. This is not the first time that cache
>> participates in circularities (I wonder if it *always* does). But I am puzzled
>> how does this happen:
>>
>> ...
>> V  [jvm.dll+0x2b5412]  Runtime1::monitorenter+0x1e2;;
>> ?monitorenter at Runtime1@@CAXPAVJavaThread@@PAVoopDesc@@PAVBasicObjectLock@@@Z+0x1e2
>> v  ~RuntimeStub::monitorenter_nofpu Runtime1 stub
>> J 192 c1
>> java.lang.invoke.MethodType$ConcurrentWeakInternSet.get(Ljava/lang/Object;)Ljava/lang/Object;
>> java.base at 9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x0000031a]
>> ...
>>
>> It calls RuntimeStub::monitorenter_nofpu destructor? Why it ends up calling into
>> monitorenter? There are no locks in j.l.i.MT$CWIS.get (I checked the bytecode
>> too).
> 
> get() is compiled so I'm assuming it has inlined something from CHM that
> does the locking.
> 
>> If you want a nuclear option for your test, you may want to pass
>> -XDstringConcat:inline to javac to disable indy string concat to bypass that
>> circularity completely.
> 
> Hmmmm ... that is worth thinking about.
> 
> Thanks,
> David
> 
>> Thanks,
>> -Aleksey