[lworld] RFR: lworld aaload/aastore

[Tobias Hartmann]

Update: All tests pass with -XX:-ProfileInterpreter -Xint.


On 14.03.2018 14:17, Tobias Hartmann wrote:
> Hi David,
> 
> I've executed test/hotspot/jtreg/compiler/valhalla/valuetypes/TestArrays.java with -Xint and your
> patch but I'm seeing crashes:
> 
> #  Internal Error
> (/oracle/valhallaL/open/src/hotspot/share/interpreter/interpreterRuntime.cpp:1470), pid=8211, tid=8212
> #  assert(mdp == mdp2) failed: wrong mdp
> 
> Stack: [0x00007fa7ea72f000,0x00007fa7ea830000],  sp=0x00007fa7ea82da10,  free space=1018k
> Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native
> code)
> V  [libjvm.so+0x1990cec]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*,
> Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x18c
> V  [libjvm.so+0x1991b1f]  VMError::report_and_die(Thread*, char const*, int, char const*, char
> const*, __va_list_tag*)+0x2f
> V  [libjvm.so+0xba33a2]  report_vm_error(char const*, int, char const*, char const*, ...)+0x112
> V  [libjvm.so+0xf7a86f]  InterpreterRuntime::verify_mdp(Method*, unsigned char*, unsigned char*)+0x2df
> j  compiler.valhalla.valuetypes.TestArrays.test15()I+35
> j  compiler.valhalla.valuetypes.TestArrays.test15_verifier(Z)V+1
> v  ~StubRoutines::call_stub
> V  [libjvm.so+0xfaf6d9]  JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*,
> Thread*)+0x819
> V  [libjvm.so+0x16fade8]  invoke(InstanceKlass*, methodHandle const&, Handle, bool, objArrayHandle,
> BasicType, objArrayHandle, bool, Thread*)+0xc18
> V  [libjvm.so+0x16ffeab]  Reflection::invoke_method(oop, Handle, objArrayHandle, Thread*)+0x1cb
> V  [libjvm.so+0x1135978]  JVM_InvokeMethod+0x228
> j
> jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Ljava/lang/reflect/Method;Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+0
> java.base
> j
> jdk.internal.reflect.NativeMethodAccessorImpl.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+100
> java.base
> j
> jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+6
> java.base
> j  java.lang.reflect.Method.invoke(Ljava/lang/Object;[Ljava/lang/Object;)Ljava/lang/Object;+59 java.base
> j  compiler.valhalla.valuetypes.ValueTypeTest.run([Ljava/lang/Class;)V+237
> j  compiler.valhalla.valuetypes.ValueTypeTest.run([Ljava/lang/String;[Ljava/lang/Class;)V+14
> j  compiler.valhalla.valuetypes.TestArrays.main([Ljava/lang/String;)V+29
> v  ~StubRoutines::call_stub
> V  [libjvm.so+0xfaf6d9]  JavaCalls::call_helper(JavaValue*, methodHandle const&, JavaCallArguments*,
> Thread*)+0x819
> V  [libjvm.so+0x108cced]  jni_invoke_static(JNIEnv_*, JavaValue*, _jobject*, JNICallType,
> _jmethodID*, JNI_ArgumentPusher*, Thread*) [clone .isra.168]+0x39d
> V  [libjvm.so+0x10aacb5]  jni_CallStaticVoidMethod+0x205
> C  [libjli.so+0x4687]  JavaMain+0xa57
> 
> Thanks,
> Tobias
> 
> 
> On 13.03.2018 17:32, David Simms wrote:
>> Hi,
>>
>> Here's a patch to implement the new lworld semantics for aaload and aastore with values:
>>
>> http://cr.openjdk.java.net/~dsimms/valhalla/lworld_value_aaloadstore/webrev0/
>>
>> Hotspot implementation currently allows value to be contained in one of three types of arrays, and
>> the aaload/aastore differs slightly depending on the array type:
>>
>>  * "Flat value array": valueArrayOop, specifically for holding values
>>    of "element_klass" type (always a "value type") and nothing else
>>      o Loads and stores of null are not permitted
>>  * "Object array": objArrayOop whose "element_klass" is not a "value
>>    type", e.g. "Object", "Comparable" etc
>>      o Maintains the same behavior for both objects and values.
>>  * "Non-flat value array": objArrayOop whose "element_klass" is
>>    specifically a "value type". This is left in the code for handling
>>    special cases like "atomic value class" or "really large value
>>    class" or "many embedded oops value class", where flattening seems
>>    unsuitable. Unclear if this use-case will survive in the long run.
>>      o   Maintains the same semantics as "Flat Value Array", i.e.:
>>        Loads and stores of null are not permitted
>>
>> So semantically, there are two types, "value array" and "object array".
>>
>> Testing: "runtime/valhalla/valuetypes/ValueTypeArray.java" now passes. I was a bit concerned for
>> "aaload" performance given it needs to test for flat array oop (i.e. loads "klass->_layout_helper").
>> I added a simulation in a patch in the experiment ("exp") branch, and through it through our
>> performance "sanity" benchmarks, no regressions. Even small micro-benchmark had a hard time seeing
>> any difference. So I will avoid any premature optimization for now, until proven otherwise.
>>
>> TODO: some further test cases which are better suited to ValueOops and bytecode API testing, which
>> is a different bug. Also, for the objArrayOop containers I didn't bother passing by value, so a
>> further test for ensuring "withfield" is COW op and not affecting the reference would be a good idea.
>>
>> /D
>>