RFR: 8292818: replace 96-bit representation for field metadata with variable-sized streams [v4]

[Chris Plummer]

On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain <fparain at openjdk.org> wrote:

>> Please review this change re-implementing the FieldInfo data structure.
>> 
>> The FieldInfo array is an old data structure storing fields metadata. It has poor extension capabilities, a complex management code because of lack of strong typing and semantic overloading, and a poor memory efficiency.
>> 
>> The new implementation uses a compressed stream to store those metadata, achieving better memory density and providing flexible extensibility, while exposing a strongly typed set of data when uncompressed. The stream is compressed using the unsigned5 encoding, which alreay present in the JDK (because of pack200) and the JVM (because JIT compulers use it to comrpess debugging information).
>> 
>> More technical details are available in the CR: https://bugs.openjdk.org/browse/JDK-8292818
>> 
>> Those changes include a re-organisation of fields' flags, splitting the previous heterogeneous AccessFlags field into three distincts flag categories: immutable flags from the class file, immutable fields defined by the JVM, and finally mutable flags defined by the JVM.
>> 
>> The SA, CI, and JVMCI, which all used to access the old FieldInfo array, have been updated too to deal with the new FieldInfo format.
>> 
>> Tested with mach5, tier 1 to 7.
>> 
>> Thank you.
>
> Frederic Parain has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixes includes and style

Changes requested by cjplummer (Reviewer).

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 75:

> 73:     int initialValueIndex;
> 74:     int genericSignatureIndex;
> 75:     int contendedGroup;

It seems that these should all be shorts. All the getter methods are casting them to short.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 99:

> 97:     if (fieldIsInitialized(fieldInfoValues.fieldFlags)) fieldInfoValues.initialValueIndex = crs.readInt(); // read initial value index
> 98:     if (fieldIsGeneric(fieldInfoValues.fieldFlags))     fieldInfoValues.genericSignatureIndex = crs.readInt(); // read generic signature index
> 99:     if (fieldIsContended(fieldInfoValues.fieldFlags))   fieldInfoValues.contendedGroup = crs.readInt(); // read contended group

Column with is too wide. These lines would be easier to read if you made each one multiple lines with curly braces.

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/Field.java line 107:

> 105:     int javafieldsCount = crs.readInt(); // read num_java_fields
> 106:     int VMFieldsCount = crs.readInt(); // read num_injected_fields;
> 107:     int numFields = javafieldsCount + VMFieldsCount;

VMFieldsCount -> vmFieldsCount, or maybe just use num_java_fields and num_injected_fields

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java line 285:

> 283:   public short getFieldNameIndex(int index) {
> 284:     if (index >= getJavaFieldsCount()) throw new IndexOutOfBoundsException("not a Java field;");
> 285:     return (short)getField(index).getNameIndex();

Cast to short not needed

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java line 303:

> 301:   public short getFieldSignatureIndex(int index) {
> 302:     if (index >= getJavaFieldsCount()) throw new IndexOutOfBoundsException("not a Java field;");
> 303:     return (short)getField(index).getGenericSignatureIndex();

Cast to short is not needed

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java line 321:

> 319:   public short getFieldInitialValueIndex(int index) {
> 320:     if (index >= getJavaFieldsCount()) throw new IndexOutOfBoundsException("not a Java field;");
> 321:     return (short)getField(index).getInitialValueIndex();

cast to short is not needed

src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/InstanceKlass.java line 325:

> 323: 
> 324:   public int getFieldOffset(int index) {
> 325:     return (int)getField(index).getOffset();

Cast to int is not needed

-------------

PR: https://git.openjdk.org/jdk/pull/12855