[lworld] RFR: lworld aaload/aastore

[David Simms]

Thanks for taking a look at this...


On 23/03/18 16:27, Karen Kinnear wrote:
> Mr Simms,
>
> Thank you so much for implementing this and clearly describing the JVMS changes and behavior we agreed to
> prototype. And I am grateful for all the test cases and experiments with array behavior with
> existing classes.
>
> The code looks good. (If you already pushed it great - I didn’t see a notification).
>
> Minor questions/comments
> 1. interpreterRuntime.cpp 496 - value_array_load - you changed the assertion check, so the message no longer matches

Doh, you are correct, will adjust.

>
> 2. templateTable_x86.cpp
> There are two places that say // Move superclass into pax - but I think they are moving the element class?
> lines 1260 and 1194

Correct, 1194 hasn't changed from want was there, but 1260 verbatim copy 
isn't quite correct, since it is carrying out an exact type check with 
"ArrayKlass::element_klass()". I.e. flat aastore is not covariant. I.e. 
type check at 1194 is interested "value subclass <: array element 
superclass", 1260 should read "value class == array element class".

Will adjust.

>
> 3. So my understanding is that: all arrays with a value type element are flattenable
> and the flatten decision is made in ValueKlass::array_klass_impl, so you either get a ValueArrayKlass - always
> flattened or an ObjArrayKlass with element type value type - so keeps non-nullable semantics.
> Is that accurate?

That is correct.

  * All "ArrayKlass::element_klass().is_value()" have non-nullable
    semantics.
      o By virtue of their "value type" element type, can't support
        covariant types (templateTable_x86.cpp:1260 above).
  * Yes, "ValueKlass::array_klass_impl()" currently decides
    "objArrayOop" or "valueArrayOop" (flat)
      o Currently, said decision is static. Once determined, never changes.


> So test_flat_array_oop is a flattened test and all ValueTypeArrays set the vt_array bit.
>
> looking forward to the profiling info.

Yup, the array klass layout helper needs loading. So "aastore" already 
does load array klass, but loading klass for aaload is new. I did some 
sanity perf testing, but interpreter changes often don't show much...and 
micro benchmarking, usually the klass in question is in cache...but 
certainly something to keep an eye on as we get JIT support working

>
> thanks!
> Karen
Cheers
/D