Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread 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.hol...@oracle.com>
> À: "Aleksey Shipilev" <sh...@redhat.com>, "core-libs-dev Libs" 
> <core-libs-dev@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@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@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
>> ...
>>
>> 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.
> 
> H ... that is worth thinking about.
> 
> Thanks,
> David
> 
>> Thanks,
>> -Aleksey


Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread David Holmes

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@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@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
...

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.


H ... that is worth thinking about.

Thanks,
David


Thanks,
-Aleksey



Re: Forcing initialization of string concat INDY expressions

2017-03-01 Thread Aleksey Shipilev
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.

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

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@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@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
...

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

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.

Thanks,
-Aleksey



Forcing initialization of string concat INDY expressions

2017-02-28 Thread David Holmes
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!)


I'm trying to workaround the problem by forcing some earlier string 
concat in the thread in which the callback will execute. From what I can 
see the test already does:


log(Thread.currentThread() + "some text");

but the error code is doing:

log("Agent '" + agentName + "' finished execution (finishedSuccessfully: 
" + finishedSuccessfully + ")");


where finishedSuccessfully is a boolean, and so we have a different form 
of makeConcatWithConstants call site.


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?


And yes I realize this error code could hit monitor use in numerous 
other places, but so far this is the only case that we have seen 
trigger. (Though I can't reproduce it - even if I force the error path 
to be taken.)


Thanks,
David
-

V  [jvm.dll+0x3ba688]  report_vm_error+0x48;; 
?report_vm_error@@YAXPBDH00ZZ+0x48
V  [jvm.dll+0x7608f0]  ObjectMonitor::enter+0x170;; 
?enter@ObjectMonitor@@QAEXPAVThread@@@Z+0x170
V  [jvm.dll+0x862035]  ObjectSynchronizer::slow_enter+0x1d5;; 
?slow_enter@ObjectSynchronizer@@SAXVHandle@@PAVBasicLock@@PAVThread@@@Z+0x1d5
V  [jvm.dll+0x85fe8f]  ObjectSynchronizer::fast_enter+0xff;; 
?fast_enter@ObjectSynchronizer@@SAXVHandle@@PAVBasicLock@@_NPAVThread@@@Z+0xff
V  [jvm.dll+0x2b5412]  Runtime1::monitorenter+0x1e2;; 
?monitorenter@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@9-internal (54 bytes) @ 0x0242d7da [0x0242d4c0+0x031a]
J 190 c1 
java.lang.invoke.MethodType.makeImpl(Ljava/lang/Class;[Ljava/lang/Class;Z)Ljava/lang/invoke/MethodType; 
java.base@9-internal (66 bytes) @ 0x0242ca20 [0x0242c9a0+0x0080]
J 255 c1 
java.lang.invoke.MemberName.getMethodOrFieldType()Ljava/lang/invoke/MethodType; 
java.base@9-internal (72 bytes) @ 0x02445238 [0x02444f20+0x0318]
J 239 c1 
java.lang.invoke.InvokerBytecodeGenerator.isStaticallyInvocable(Ljava/lang/invoke/MemberName;)Z 
java.base@9-internal (169 bytes) @ 0x0243dc28 [0x0243d820+0x0408]
j  java.lang.invoke.InvokerBytecodeGenerator.addMethod()V+648 
java.base@9-internal
j 
java.lang.invoke.InvokerBytecodeGenerator.generateCustomizedCodeBytes()[B+6 
java.base@9-internal
j 
java.lang.invoke.InvokerBytecodeGenerator.generateCustomizedCode(Ljava/lang/invoke/LambdaForm;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/MemberName;+25 
java.base@9-internal

j  java.lang.invoke.LambdaForm.compileToBytecode()V+69 java.base@9-internal
j  java.lang.invoke.LambdaForm.prepare()V+21 java.base@9-internal
j 
java.lang.invoke.MethodHandle.(Ljava/lang/invoke/MethodType;Ljava/lang/invoke/LambdaForm;)V+33 
java.base@9-internal
j 
java.lang.invoke.BoundMethodHandle.(Ljava/lang/invoke/MethodType;Ljava/lang/invoke/LambdaForm;)V+3 
java.base@9-internal
j 
java.lang.invoke.BoundMethodHandle$Species_L9.(Ljava/lang/invoke/MethodType;Ljava/lang/invoke/LambdaForm;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;Ljava/lang/Object;)V+3 
java.base@9-internal
j 
java.lang.invoke.BoundMethodHandle$Species_L9.copyWith(Ljava/lang/invoke/MethodType;Ljava/lang/invoke/LambdaForm;)Ljava/lang/invoke/BoundMethodHandle;+42 
java.base@9-internal
j 
java.lang.invoke.MethodHandles.dropArguments0(Ljava/lang/invoke/MethodHandle;ILjava/util/List;)Ljava/lang/invoke/MethodHandle;+105 
java.base@9-internal
j 
java.lang.invoke.MethodHandles.dropArguments(Ljava/lang/invoke/MethodHandle;I[Ljava/lang/Class;)Ljava/lang/invoke/MethodHandle;+6 
java.base@9-internal
j 
java.lang.invoke.StringConcatFactory$MethodHandleInlineCopyStrategy.generate(Ljava/lang/invoke/MethodType;Ljava/lang/invoke/StringConcatFactory$Recipe;)Ljava/lang/invoke/MethodHandle;+479 
java.base@9-internal
j 
java.lang.invoke.StringConcatFactory.generate(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/StringConcatFactory$Recipe;)Ljava/lang/invoke/MethodHandle;+101 
java.base@9-internal
j 
java.lang.invoke.StringConcatFactory.doStringConcat(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;ZLjava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;+507 
java.base@9-internal
j