On Mon, 13 Mar 2023 16:26:06 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: > > SA additional caching from Chris Plummer Most minor comments but one .inline.hpp still in an hpp file. I should point out that I only skimmed the SA and JVMCI changes. src/hotspot/share/jvmci/jvmciCompilerToVM.cpp line 2653: > 2651: } > 2652: InstanceKlass* iklass = InstanceKlass::cast(klass); > 2653: if (index < 0 ||index > iklass->total_fields_count()) { nit: need space after || src/hotspot/share/oops/fieldInfo.cpp line 45: > 43: } > 44: > 45: void FieldInfo::print_from_growable_array(GrowableArray<FieldInfo>* > array, outputStream* os, ConstantPool* cp) { For consistency, can you make the outputStream parameter first? src/hotspot/share/oops/instanceKlass.hpp line 32: > 30: #include "oops/constMethod.hpp" > 31: #include "oops/constantPool.hpp" > 32: #include "oops/fieldInfo.inline.hpp" This shouldn't have an inline.hpp inclusion. src/hotspot/share/runtime/vmStructs.cpp line 2304: > 2302: declare_constant(FieldInfo::FieldFlags::_ff_generic) > \ > 2303: declare_constant(FieldInfo::FieldFlags::_ff_stable) > \ > 2304: declare_constant(FieldInfo::FieldFlags::_ff_contended) > \ If there are flags that SA doesn't use, like contended, I don't think they should be included in the information that we pass to SA. ------------- Changes requested by coleenp (Reviewer). PR: https://git.openjdk.org/jdk/pull/12855
