[lworld] RFR: lworld aaload/aastore
david.simms at oracle.com
Mon Mar 26 09:20:11 UTC 2018
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".
> 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
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
More information about the valhalla-dev