On Fri, 3 Mar 2023 14:50:34 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. HI Fred, I've taken one pass through this but it is a huge set of changes to try and digest. At this stage just a bunch of style nits. Thanks. src/hotspot/share/classfile/classFileParser.cpp line 1632: > 1630: { > 1631: debug_only(NoSafepointVerifier nsv;) > 1632: for(int i = 0; i < _temp_field_info->length(); i++) { Nit: space after 'for' please src/hotspot/share/classfile/classFileParser.cpp line 2012: > 2010: void ClassFileParser::FieldAnnotationCollector::apply_to(FieldInfo* f) { > 2011: if (is_contended()) > 2012: // Setting the contended group also set the contended bit in field > flags Nit: s/set/sets/ src/hotspot/share/oops/fieldInfo.cpp line 30: > 28: > 29: void FieldInfo::print(outputStream* os, ConstantPool* cp) { > 30: os->print_cr("index=%d name_index=%d name=%s signature_index=%d > signature=%s offset=%d AccessFlags=%d FieldFlags=%d initval_index=%d > gen_signature_index=%d, gen_signature=%s contended_group=%d", Nit: please break up this line src/hotspot/share/oops/fieldInfo.cpp line 120: > 118: *java_fields_count = r.next_uint(); > 119: *injected_fields_count = r.next_uint(); > 120: while(r.has_next()) { Nit: space before ( src/hotspot/share/oops/fieldInfo.cpp line 135: > 133: int java_field_count = r.next_uint(); > 134: int injected_fields_count = r.next_uint(); > 135: while(r.has_next()) { Nit: space before ( src/hotspot/share/oops/fieldInfo.cpp line 140: > 138: fi.print(os, cp); > 139: } > 140: } Newline needed at EOF src/hotspot/share/oops/fieldInfo.inline.hpp line 135: > 133: new_flags = old_flags | mask; > 134: witness = Atomic::cmpxchg(&flags, old_flags, new_flags); > 135: } while(witness != old_flags); Nit: space before ( src/hotspot/share/oops/fieldInfo.inline.hpp line 155: > 153: inline void FieldStatus::update_access_watched(bool z) { > update_flag(_fs_access_watched, z); } > 154: inline void FieldStatus::update_modification_watched(bool z) { > update_flag(_fs_modification_watched, z); } > 155: inline void FieldStatus::update_initialized_final_update(bool z) > {update_flag(_initialized_final_update, z); } Nit: space after { src/hotspot/share/oops/fieldStreams.hpp line 43: > 41: protected: > 42: const Array<u1>* _fieldinfo_stream; > 43: FieldInfoReader _reader; Nit variable name alignment seems off ------------- PR: https://git.openjdk.org/jdk/pull/12855
