On Mon, 13 Mar 2023 18:51:17 GMT, Frederic Parain <fpar...@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
