RFR (M): Add missing compiler checks
zoltan.majo at oracle.com
Wed Jun 14 15:16:48 UTC 2017
thank you for the review!
On 06/14/2017 01:53 PM, David Simms wrote:
> The changes to "src/share/vm/oops/*" and
> "src/share/vm/interpreter/interpreterRuntime.cpp" look good.
> One nit-pick is the use of C cast to "ValueKlass*" rather than
> "ValueKlass::cast()" like we generally do for Klass casting.
I changed element_klass to have covariant return types (as Tobias
suggested). That removed the need for ValueKlass::cast()'s.
> The extra field in TypeArrayKlass is minimal, given the fixed number
> of types (even with multi-dim).
> Thanks for the extra klass check in vastore, btw !
I'll send the updated code in my reply to Tobias.
> /David Simms
> On 14/06/17 12:55, Zoltán Majó wrote:
>> please review the following change:
>> The change adds some (or hopefully all) checks that C2 currently does
>> not generate but will most likely be required by the MVT
>> specification. I tested the change with JPRT (x86_64), no failures
>> have appeared. I'm currently running the hotspot/compiler JTREG tests
>> A few notes about the change:
>> - Moving _element_klass from ObjArrayKlass to ArrayKlass is necessary
>> because that way the klass of the array elements is found at the same
>> offset in both ObjArrayKlass and ValueArrayKlass instances (and we
>> need the offset to implement type checks). However, _element_klass is
>> now present in TypeArrayKlass as well (where it is not needed). Maybe
>> a better way to organize the class hierarchy would be to put
>> _element_klass into a new common subclass X of [Obj|Value]ArrayClass;
>> both X and TypeArrayKlass would then be direct subclasses of
>> ArrayKlass. Maybe it's better to deal with this aspect once the we're
>> closer to the final specification/design of MVT.
>> - The change adds a type check to
>> InterpreterRuntime::value_array_store. I assumed (and expect) vastore
>> will behave similarly to aastore and requires the element type of
>> src/dst to match. Please let me know if that is an incorrect assumption.
>> - The bytecodes vdefault/vwithfield can throw an OutOfMemoryError if
>> no space is available for allocation. With the current C2
>> implementation allocations can be delayed to a later point in the
>> program (e.g., when a deoptimization happens). If no free memory is
>> available at that point to allocate memory for a value type, the OOME
>> will appear later than the vdefault/vwithfield bytecode that actually
>> caused it. Tobias and I discussed about this aspect and it's likely
>> that this is not a problem as re-allocation of scalar-replaced
>> objects is similar and is also performed at deoptimization. However,
>> more investigation is needed to confirm that.
>> Thank you!
>> Best regards,
More information about the valhalla-dev