Initial runtime support for the ValueTypes attribute (original) (raw)

Frederic Parain frederic.parain at oracle.com
Wed May 23 19:41:16 UTC 2018

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 ACCFLATTENABLE and NOT isdeclaredvaluetype? 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 ACCFLATTENABLE 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 ACCFLATTENABLE flag and the ValueTypes attribute - the pre-loading of method arguments types and return values types Fred

More information about the valhalla-dev mailing list