Initial runtime support for the ValueTypes attribute

[Frederic Parain]

Karen,

Thank you for the review.

> On May 23, 2018, at 15:12, Karen Kinnear <KAREN.KINNEAR at ORACLE.COM> wrote:
> 
> Frederic,
> 
> Totally agree with your discussion with Tobias - thanks for that.
> 
> Code looks good. Minor questions. I don’t need to see an updated webrev.
> 
> instanceKlass.cpp: line 617 “if this class” -> “of this class”
> lines 626-6267 “where potential” -> “to detect potential”

Both are fixed now.

> 
> classFileParser.cpp lines 4104/4105 
> not clear where you are using name and signature in the exception throwing case?
> 

A remain from the logging I used to verify the behavior of the code.
I removed it.

I’ll push the code with these modifications.

Thank you,

Fred


> 
>> On May 22, 2018, at 3:00 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>> 
>> Karen,
>> 
>> Good catch on classFileParser lines 4078/4092, the test was wrong,
>> but another bug in the way class names were retrieved from the signature
>> was hiding the problem. Both are fixed now,
>> 
>> After a discussion with Tobias, from the JIT compiler team, there’s no need
>> to pre-load argument types and return value types for all referenced methods.
>> It is sufficient to pre-load those types for the methods the class declares.
>> The current implementation is an approximation, as it preloads types for all
>> methods of the current class, but it achieves the same semantic.
>> 
>> New webrev:
>> http://cr.openjdk.java.net/~fparain/VTAttribute/webrev.02/index.html
>> 
>> Thank you,
>> 
>> Fred
>> 
>> 
>>> On May 18, 2018, at 15:43, Karen Kinnear <KAREN.KINNEAR at ORACLE.COM> wrote:
>>> 
>>> 
>>> Frederic,
>>> 
>>> Thank you for the improvements. 
>>> I agree with not need at this point to do pre-loading for arrays since the element type
>>> is already loaded before array creation.
>>> I had wondered about pre-loading fields mentioned in field descriptors and agree there
>>> is no need.
>>> 
>>> instanceKlass.cpp line 618 - partial sentence
>>> 
>>> classFileParser.cpp lines 4078/4092 - did I read these backwards?
>>> Isn’t it a ClassFormatError if ACC_FLATTENABLE and NOT is_declared_value_type?
>>> 
>>> Rest looks good.
>>> 
>>> thanks,
>>> Karen
>>> 
>>>> On May 18, 2018, at 1:50 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>>>> 
>>>> Karen,
>>>> 
>>>> Your comment made me think that I wanted too much
>>>> to rush this code out for the JIT team. It needed some
>>>> clean up first and more consistency.
>>>> 
>>>> Regarding arrays, so far, we haven’t identify any array optimization that
>>>> would require pre-loading. The element type is loading just before creating
>>>> the array, and it is sufficient to implement our current optimization (array flattening)
>>>> 
>>>> Here’s a new webrev:
>>>> http://cr.openjdk.java.net/~fparain/VTAttribute/webrev.01/index.html
>>>> 
>>>> Changes:
>>>> - pre-loading code for flattenable fields in the link phase has been removed:
>>>> this pre-loading is already performed at load time when fields layout is computed.
>>>> - error detection and handling has been improved and made consistent between
>>>> pre-loading for flattenable fields and pre-loading for method arguments
>>>> - comments have been update
>>>> 
>>>> To summarize:
>>>> - at load time: pre-loading of flattenable fields types
>>>> - at link time: pre-loading of method arguments and return types
>>>> - at execution time: regular loading of element types for array
>>>> 
>>>> Error Handling:
>>>> - If a field has the ACC_FLATTENABLE flag set but its type is not listed in
>>>> the ValueTypes attribute, an ClassFormatError is thrown
>>>> - If a class is pre-loaded but it is not a value class, an ICCE is thrown
>>>> 
>>>> Thanks,
>>>> 
>>>> Fred
>>>> 
>>>> 
>>>>> On May 18, 2018, at 10:16, Karen Kinnear <KAREN.KINNEAR at ORACLE.COM> wrote:
>>>>> 
>>>>> Frederic,
>>>>> 
>>>>> Code looks really good. Many thanks for doing this so quickly and carefully.
>>>>> Thank you for the symbol refcount handling.
>>>>> 
>>>>> Summary:
>>>>> declared fields (static and instance): preload, i.e. classfileparser loads before completing container loading
>>>>> linkage to fields and methods, we load at link time prior to creation of vtables/itables
>>>>> 
>>>>> - this all sounds correct to me.
>>>>> 
>>>>> instanceKlass.cpp: 621
>>>>> - comment is that arrays of value types are not handled
>>>>> - do we actually need to preload arrays of value types at link time for any optimizations?
>>>>> 
>>>>> Empty.java: line 40: Excepted -> Expected
>>>>> 
>>>>> thanks,
>>>>> Karen
>>>>> 
>>>>>> On May 17, 2018, at 4:04 PM, Frederic Parain <frederic.parain at oracle.com> wrote:
>>>>>> 
>>>>>> Please review this first patch related to the ValueTypes attribute:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~fparain/VTAttribute/webrev.00/
>>>>>> 
>>>>>> The patch includes:
>>>>>> - the parsing of the ValueTypes attribute
>>>>>> - the creation of meta-data from this attribute
>>>>>> - a consistency check between the ACC_FLATTENABLE flag
>>>>>> and the ValueTypes attribute
>>>>>> - the pre-loading of method arguments types and return values types
>>>>>> 
>>>>>> Fred
>>>>>> 
>>>>> 
>>>> 
>>> 
>> 
>